3
\$\begingroup\$

I have created a simple DBLayer class. But I am not sure whether this class have any bug/issue using in production.

public class DBLayer
{
    private static string connectionString = ConfigurationManager.AppSettings["ConnectionString"];

    public static DataTable GetDataTable(string strSQL, CommandType type)
    {
        var objCmd = new SqlCommand(strSQL);
        objCmd.CommandType = type;
        return GetDataTable(objCmd);
    }

    public static DataTable GetDataTable(SqlCommand objCmd)
    {
        SqlConnection objConn = null;
        try
        {
            objConn = new SqlConnection(connectionString);
            objCmd.Connection = objConn;
            var objAdapter = new SqlDataAdapter(objCmd);
            var ds = new DataSet();
            objAdapter.Fill(ds);
            return ds.Tables[0];
        }
        finally
        {
            objConn.Close();
        }
    }

    public static int ExecuteNonQuery(SqlCommand objCmd)
    {
        int r = -1;
        SqlConnection objConn = null;
        try
        {
            objConn = new SqlConnection(connectionString);
            objCmd.Connection = objConn;
            objConn.Open();
            r = objCmd.ExecuteNonQuery();
        }
        finally
        {
            objConn.Close();
        }

        return r;
    }
}

Please review this code?

\$\endgroup\$
3
\$\begingroup\$

Looks good. You may also want some other things like getScalar (A single object, or string, as in 1 cell), getDataRow, just the top row, getDataRowCollection, i frequently only use datatable to get the rows, and forget about the columns, so I can iterate over it like so....

DataRowCollection drc = getDataRowCollection("SELECT * FROM tblTest");
for(DataRow r in drc){
    Console.WriteLine(r["name"]);
}

So you could have NonQuery, you may want to test these before using them....

    public DataTable getDataTable(SqlCommand objCmd)
    {
        SqlConnection objConn = null;
        try
        {
            objConn = new SqlConnection(connectionString);
            objCmd.Connection = objConn;
            var objAdapter = new SqlDataAdapter(objCmd);
            var ds = new DataSet();
            objAdapter.Fill(ds);
            return ds.Tables[0];
        }
        finally
        {
            objConn.Close();
        }
    }

    public DataRowCollection getDataRowCollection(SqlCommand objCmd)
    {
        return getDataTable(objCmd).Rows;
    }

    public DataRow getDataRow(SqlCommand objCmd){
        return getDataRowCollection(objCmd)[0];
    }

    public string getScalar(SqlCommand objCmd)
    {
        SqlConnection objConn = null;
        try
        {
            objConn = new SqlConnection(connectionString);
            objCmd.Connection = objConn;
            return objCmd.ExecuteScalar().ToString();
        }
        finally
        {
            objConn.Close();
        }
    }

    public int NonQuery(SqlCommand objCmd)
    {
        SqlConnection objConn = null;
        try
        {
            objConn = new SqlConnection(connectionString);
            objCmd.Connection = objConn;
            return objCmd.ExecuteNonQuery();
        }
        finally
        {
            objConn.Close();
        }
    }

I would recommend writing quite a few variations of the methods, and implement enums for each connection string so you don't mis-type anywhere and have them set in concrete.

Take a look at my getScalar variations....

    public static string getScalar(string query)
    {
        return getScalar(query, defaultConnection);
    }

    public static string getScalar(string query, ConnectionString cs)
    {
        return getScalar(query, new { }, cs);
    }

    public static string getScalar(string query, object param, ConnectionString cs)
    {
        return getScalar(query, param, cs, defaultTimeout);
    }
    public static string Query(string query, object param, int to)
    {
        return getScalar(query, param, defaultConnection, to);
    }

    public static string getScalar(string query, object param, ConnectionString cs, int to)
    {
        return getScalar(ConnectionStringToDb(cs), query, param, to);
    }

    public static string getScalar(MySqlConnection db, string query)
    {
        return getScalar(db, query, defaultTimeout);
    }

    public static string getScalar(MySqlConnection db, string query, int to)
    {
        return getScalar(db, query, null, to);
    }

    public static string getScalar(MySqlConnection db, string query, object param, int to)
    {
        string result = "";
        if (db.State != ConnectionState.Open)
            OpenConnection(db);
        MySqlParameter[] p = ObjectToParameters(param);
        using (db)
        {
            try
            {
                MySqlCommand command = new MySqlCommand();
                command.Connection = db;
                command.CommandText = query;
                command.CommandTimeout = to;
                foreach (MySqlParameter pm in p)
                {
                    command.Parameters.Add(pm);
                }
                result = command.ExecuteScalar().ToString();
            }
            catch { }
        }
        return result;
    }

Also if you class works as expected it should be fine... Looks okay. I use mine day to day, but I use MySql so I'm not to sure on the differences between the two classes, should be relatively the same.

\$\endgroup\$
2
\$\begingroup\$

There are several issues in the code:

  • do use using statement to dispose connections and other disposable objects
  • static methods won't allow you to use unit-testing on your business logic.
  • don't use obj prefixes or non-descriptive names like r

As a result of original code refactoring I got the following:

public interface IDBLayer
{
    DataTable GetDataTable(SqlCommand command);
    int ExecuteNonQuery(SqlCommand command);
}

public static class DBLayerExtensions
{
    public static DataTable GetDataTable(this IDBLayer dbLayer, string sql, CommandType type)
    {
        using (var command = new SqlCommand(sql))
        {
            command.CommandType = type;
            return dbLayer.GetDataTable(command);
        }
    }
}

public class DBLayer : IDBLayer
{
    private static readonly string _connectionString = ConfigurationManager.AppSettings["ConnectionString"];

    public DataTable GetDataTable(SqlCommand command)
    {
        using (var connection = new SqlConnection(_connectionString))
        using (var dataAdapter = new SqlDataAdapter(command))
        {
            command.Connection = connection;
            var result = new DataTable();
            dataAdapter.Fill(result);
            return result;
        }
    }

    public int ExecuteNonQuery(SqlCommand command)
    {
        using (var connection = new SqlConnection(_connectionString))
        {
            command.Connection = connection;
            connection.Open();
            return command.ExecuteNonQuery();
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ I like your approach. But the problem is that in the project static methods invocation are spoiled all over places. So, I can't make it instance. However, I will use using clause. Thanks. \$\endgroup\$ – user960567 Mar 13 '13 at 11:41

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.