Central Database class

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 sur

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);

        /// 
        /// Get data from the database.
        /// 
        /// SQL command to retrieve data.
        /// Connection string for the database.
        /// IDataReader with the first result set, or null if no result was found.
        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;
        }
    }
}

StackExchange Code Review Q#113538, answer score: 2