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

Question

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!

IMO, the best way is to use one connection for one query. You want to execute another query? Set up another connection. :) — waka
– waka, Commented Mar 7, 2017 at 13:44
Put a using around cmd.ExecuteReader() or call Dispose on your DataReader when done — user743414
– user743414, Commented Mar 7, 2017 at 13:46

Rahul · Accepted Answer · 2017-03-07 13:54:30Z

2

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.

edited Mar 7, 2017 at 13:54

answered Mar 7, 2017 at 13:46

Rahul

78.1k14 gold badges80 silver badges132 bronze badges

Sign up to request clarification or add additional context in comments.

1 Comment

David Over a year ago

Thank you for your help!

Community · Accepted Answer · 2017-05-23 10:30:04Z

1

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.

edited May 23, 2017 at 10:30

CommunityBot

11 silver badge

answered Mar 7, 2017 at 13:49

granadaCoder

28.2k11 gold badges124 silver badges166 bronze badges

3 Comments

granadaCoder Over a year ago

I made an append to point you to a similar question and approach.

David Over a year ago

Thank you so much, this addressed absolutely everything I think! I appreciate your addition of samples.

granadaCoder Over a year ago

If you stick with this approach, pushing your IDataReaders into Dto/Poco objects, you'll be much better for it. It is also a much easier transition to Entity Framework (or NHibernate, or other) when you get there. Its 2017, try to avoid DataSet/DataTable solutions. You're code will be much better, much more maintainable. Take a look at this when you get a chance: msdn.microsoft.com/en-us/library/…

WereGoingOcean · Accepted Answer · 2017-03-07 13:46:12Z

1

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.

answered Mar 7, 2017 at 13:46

WereGoingOcean

1081 silver badge10 bronze badges

Comments

user743414 · Accepted Answer · 2017-03-07 13:58:51Z

1

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.

edited Mar 7, 2017 at 13:58

answered Mar 7, 2017 at 13:47

user743414

94610 silver badges25 bronze badges

1 Comment

David Over a year ago

Not closing my reader ended up being the big source of how the pool was filling up. Thanks!

Collectives™ on Stack Overflow

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

4 Answers 4

1 Comment

3 Comments

Comments

1 Comment

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

4 Answers 4

1 Comment

3 Comments

Comments

1 Comment

Your Answer

Sign up or log in

Post as a guest

Linked

Related