CODEWITHSUNDEEP
c#sqlasp.netsql-serverdatareader
David
David

Reputation: 593

Best way to structure multiple queries in one method c# asp.net

I have a page load method that loads asp dropdowns with sql queries to my SQL Server 2012 database. I'm new to this and basically independently learned a lot of needed to be done for the co op project I'm working on.

I've been running into problems with connections not being closed properly and having my connection pool blow up with only moderate usage of my app so I've been trying to improve how I'm executing my queries in my c# code behind. But I am not confident in my understanding of this so I'm going to post an example of my code and hope that someone much more fluent might be able to guide me a bit.

string constr = ConfigurationManager.ConnectionStrings["CurrencyDb"].ConnectionString;
                using (SqlConnection con = new SqlConnection(constr)) {
                    using (SqlCommand cmd = new SqlCommand("SELECT * FROM dbo.Category")) {
                        try {
                            cmd.CommandType = CommandType.Text;
                            cmd.Connection = con;
                            con.Open();

                            //Populate Category Dropdown
                            DDCategory.DataSource = cmd.ExecuteReader();
                            DDCategory.DataTextField = "CategoryName";
                            DDCategory.DataValueField = "CategoryId";
                            DDCategory.DataBind();
                        }
                        catch (SqlException sqlex) {
                            throw new Exception("SQL Exception loading data from database. " + sqlex.Message);
                        }

                        catch (Exception ex) {
                            throw new Exception("Error loading Category data from database. " + ex.Message);
                        }
                    }

                    using (SqlCommand cmd = new SqlCommand("SELECT * FROM dbo.SubCategory ORDER BY SubCategoryName")) {
                        try {
                            cmd.CommandType = CommandType.Text;
                            cmd.Connection = con;

                            //Populate SubCategory Dropdown
                            DDSubCategory.DataSource = cmd.ExecuteReader();
                            DDSubCategory.DataTextField = "SubCategoryName";
                            DDSubCategory.DataValueField = "SubCategoryId";
                            DDSubCategory.DataBind();
                        }
                        catch (SqlException sqlex) {
                            throw new Exception("SQL Exception loading data from database. " + sqlex.Message);
                        }

                        catch (Exception ex) {
                            throw new Exception("Error loading Subcategory data from database. " + ex.Message);
                        }
                    }
                }

The above are two queries of about 8 on my page load method.

My latest error was

There is already an open DataReader associated with this Command which must be closed first.

This prompted me to ask the question, I set MultipleActiveResultSets=true in my Web.config connection string and my app works now, but I feel as though that's a patch to cover what is likely crappy code.

What is the best practice for doing this? Thanks so much in advance!

Upvotes: 1

Views: 3034

Answers (4)

granadaCoder
granadaCoder

Reputation: 27904

To your immediate question, you are using two cmds per one connection. You could be better off using one-trip, one command, with a multiple-result.

This will allow you to properly .Close() your datareader and your connection...."returning them back to the pool"

Since you are using SqlServer, it supports multiple resultsets in one "trip".

SELECT * FROM dbo.Category;SELECT * FROM dbo.SubCategory

Use that as your select statement.

Use an .ExecuteReader() method.

And use a .NextResult() to move from one result (category), to the next one (subcategory)

You can see a more complete sample here:

https://msdn.microsoft.com/en-us/library/system.data.idatareader.nextresult(v=vs.110).aspx

Or even with Entity Framework here:

https://msdn.microsoft.com/en-us/library/jj691402%28v=vs.113%29.aspx?f=255&MSPPError=-2147217396

Now some extra stuff.

You are mixing your datalayer and your presentation layer.

You should have your datalayer populate "Dto" sometimes referred to as "Poco" objects, and return those to the Presentation layer.

public class Category
 public string CategoryKey{get;set;}
 public string CategoryName{get;set;}
public ICollection<SubCategory> SubCategories {get;set;}

.. 

 public class SubCategory
 public string SubCategoryKey{get;set;}
 public string SubCategoryName{get;set;}

.. 

public class MyDataLayerObject
    public ICollection<Category> GetAllCategories()
{
    ICollection<Category> categories;
    ICollection<SubCategory> subcats;

    // write a datareader call here, and use it to populate multiple Category and SubCategory objects
// make sure you close the datareader when done
//now "match up" the subcats to its parent category
}

Then have your presentation layer "bind" to the ICollection of Categories.

You might have a business layer between the presentation and datalayers, but at least to the presentation and datalayers.

I also answered a question at the link below...that is similar to yours. Their objects are "Question" and "Answer" where a Question has 0:N number of Answers.

Find my answer at the below question.

Return objects with populated list properties from stored procedure

Note, you can just use the 2 select queries in a string (the second line of my/this answer), aka, you don't have to create a stored procedure.

Another option is to use nuget to get Microsoft.EnterpriseLibrary.Data. This encapsulates alot of great practices for you...including closing connections. You'll still have to close your datareader, but the code is alot cleaner.

Upvotes: 1

user743414
user743414

Reputation: 936

You should call Dispose on your DataReader or either put it inside a using statement so that Dispose() is called for you.

The typical IDataReader pattern looks like this:

using (IDataReader r = query.ExecuteReader())
{
  while (r.Read())
  {
     // etc.
  }
}

When you don't call Dispose() on the IDataReader he can't be cleaned up. And I expect that your connection also can't be cleaned up even when you call Dispose() on the connection, because it's stil used by your IDataReader.

And yes per Connection you can only use one IDataReader. So as others wrote you should split this up into more than one method and use one connection per method.

Btw.: It would be better not to use "SELECT *". Just query the fields you need.

Upvotes: 1

Rahul
Rahul

Reputation: 77934

Well, there could be multiple way but I feel you should wrap all this queries in a stored procedure and call that SP in your code behind. Instead of using DataReader use a DataSet and fill it up with the different resultset.

You can also use NextResult() method of datareader instance to get the next SELECT result and do your processing.

Upvotes: 2

WereGoingOcean
WereGoingOcean

Reputation: 108

These two should be in different methods. One to get category and one to get sub-categories. Then each will have a different connection and you won't have this problem.

Upvotes: 1

Related Questions