Central Database class

Question

I have this web site I inherited as part of my job and it has some old code. I'd like to optimize it, but I don't really have the time to do it right, so I'm looking for some "low hanging fruit" to optimize. Basically I want minimal change with the greatest performance improvement.

One of the things I've come across is an old static method used to grab data from a remote database:

public static IDataReader GetRS(string sql, string connString)
{
 SqlConnection dbconn = new SqlConnection();
 if (connString.ToLower().Trim() == "shop")
 dbconn.ConnectionString = ConfigurationManager.ConnectionStrings["Shop.DbConnection"].ConnectionString;
 else
 dbconn.ConnectionString = connString;
 dbconn.Open();
 SqlCommand cmd = new SqlCommand(sql, dbconn);
 return cmd.ExecuteReader(CommandBehavior.CloseConnection);
}

I think this code is contributing to a high number of NumberOfReclaimedConnections we are seeing on the servers due to the calling code not cleaning up properly. If I rewrite the above method like the one below, will it help make things more efficient without having to rewrite all the hundreds of instances of calling code?

public static IDataReader GetRS(string sql, string connString)
{
 IDataReader ans = null;
 using (DataTable tbl = new DataTable())
 {
 using (SqlConnection dbconn = new SqlConnection())
 {
 if (connString.ToLower().Trim() == "shop")
 dbconn.ConnectionString = ConfigurationManager.ConnectionStrings["Shop.DbConnection"].ConnectionString;
 else
 dbconn.ConnectionString = connString;
 using (SqlCommand cmd = new SqlCommand(sql, dbconn))
 {
 using (SqlDataAdapter adapter = new SqlDataAdapter(cmd))
 { adapter.Fill(tbl); }
 }
 }
 if (tbl.Rows.Count > 0)
 {
 ans = tbl.CreateDataReader();
 }
 }
 return ans;
}

I'm pretty sure this will do a better job at cleaning up the connection as it will be disposed before the method returns and even if there is an error it will be disposed. Does the DataTable being disposed interfere with the caller's ability to extract data from the DataTableReader? If I do need to remove the using around the DataTable, will not cleaning up the DataTable cause similar issues to not cleaning up the SqlConnection as it is now?

I'm also open to writing this code in a completely different way as long as I'm fulfilling the following:

• I don't have to rewrite code on all the calling references to this method, I just don't have time to do that right now. But it's on the list.
• Due to the above requirement, the return signature has to be compatible with an IDataReader. I'm open to writing a custom class that inherits this interface if that is what helps.
• The code targets and will compile in a .NET 2.0 project. We're behind the technology curve.

• RubberDuck: God bless & good luck. At least it's not VB. (I might have some 2.0 VB where I'm at... might.)
• radarbob: Returning an SqlDataReader. The specific resolution for his problem is not so relevant as the discussion about the connection.

Answer 1

I would like to point some opportunities here. As you are refactoring your old code, I suggest:

• try to avoid the use of literal string inside methods. Instead, use const variables or Resources (I do prefer Resources);
• create your variables where you need them. Move your using to the nearest point of interest;
• try to use string.Equals, it will be faster;
• get your DbCommand directly from your connection, you have a method to do this;
• try to use a Factory Pattern in this class, so in the future, if you need to change your database, you won't have to search for all your SqlConnection objects, you'll just need to change your Factory creator;
• don't forget your XML comments

I made an example just to show those points. Sorry for the simplicity, but it's more difficult without knowing all your solution, but here is the idea:

using System;
using System.Configuration;
using System.Data;
using System.Data.Common;
namespace ClassLibrary1
{
 public class Class1
 {
 public const string shopStr = "shop";
 public const string shopDbConn = "Shop.DbConnection";
 public const string factoryDb = "System.Data.SqlClient";
 private static DbProviderFactory factory = DbProviderFactories.GetFactory(factoryDb);
 /// <summary>
 /// Get data from the database.
 /// </summary>
 /// <param name="sql">SQL command to retrieve data.</param>
 /// <param name="connString">Connection string for the database.</param>
 /// <returns><c>IDataReader</c> with the first result set, or <c>null</c> if no result was found.</returns>
 public static IDataReader GetRS(string sql, string connString)
 {
 IDataReader result = null;
 using (DbConnection conn = factory.CreateConnection())
 {
 if (string.Equals(connString, shopStr, StringComparison.OrdinalIgnoreCase))
 conn.ConnectionString = ConfigurationManager.ConnectionStrings[shopDbConn].ConnectionString;
 else
 conn.ConnectionString = connString;
 using (DbCommand cmd = conn.CreateCommand())
 {
 cmd.CommandText = sql;
 using (DbDataAdapter adapter = factory.CreateDataAdapter())
 {
 DataTable tbl = new DataTable(); // don't dispose it
 adapter.Fill(tbl);
 if (tbl.Rows.Count > 0) result = tbl.CreateDataReader();
 }
 }
 }
 return result;
 }
 }
}

• David VanKirk: Do you know if the DataTableReader returned will be invalid when the DataTable it is created from falls out of scope and is disposed?
• David VanKirk: I'm a little hesitant to use the factory stuff just cause I don't understand it. Do I need to have anything special in my web.config to use the factory code? And will it work in .NET 2.0?
• Anderson Rancan: @DavidVanKirk sorry, my bad! You should not Dipose() your DataTable, because you'll lost your DataReader data, see stackoverflow.com/questions/913228/…. And don't be afraid of using the Factory Pattern, it'll help you and it's not so difficult!! msdn.microsoft.com/en-us/library/dd0w4a2z(v=vs.110).aspx
• David VanKirk: @AndersonRacan thanks again. Looks like you can dispose the DataTable and still get data out of the DataReader it creates. Not sure exactly how. I tracked it through the framework code and it shouldn't work that way, but tests showed it did. Either way, I'm refactoring to leave the DataTable for GC instead of disposing it in the GetRS method as you suggested. Good news is that it looks like this refactoring did the trick. Reclaimed connection count on the servers is zero right now. Will update my question post with a full update later.

Answer 2

Yes you are on right track using the USING construct, as the objects are marked eligible for GC as soon as you fall out of scope.

One note on the datadapter.fill, it was my understanding years ago that the execute reader was the fastest because it's a one-shot top to bottom thing.

Answer 3

Your code change introduces a change in behaviour which is just as likely to adversely affect performance... A DataReader streams the data one row at a time whereas the DataTable loads all the data into memory.

The CommandBehavior.CloseConnection being passed in means that the underlying connection will be closed when the DataReader is. If the calling code isn't disposing the reader then it is wrong and you should change that.

i.e.

using (IDataReader reader = GetRS(..))
{
}

You could also clean up some of the names:

• what does RS mean?
• sql -> commandText
• connString -> connectionString

Something that's a bit terrifying is that the method takes SQL command text but has no parameters to pass in. That's a big indication to me that this sort of thing might be happening:

string commandText = "SELECT Id, Name, Price from PRODUCTS WHERE Name = '" + productName + '";

If that is the case (and it's a stab in the dark from me) that's where your refactoring effort should lie.

• David VanKirk: Thank you. I realize there is much wrong with how the code is written. But my question indicated that I don't have the time or resources to refactor the entire project the right way, so I'm looking for a stop-gap solution.