ADO.NET database access layer

Question

I am attempting to create a database access layer. I am looking for some improvements to this class/recommendations on best practice. It would be helpful if someone could point me to documentation on how this could be potentially done/things to consider. I have looked at using entity framework but it does not seem applicable, however, should I really be looking to move to EF? Is ADO.NET an outdated way of doing this?

public static IDbCommand GetCommandObject(string Connstring)
{
    IDbConnection conn = new SqlConnection(Connstring);
    return new SqlCommand { Connection = (SqlConnection)conn };
}

public static void AddParameter(ref IDbCommand cmd, string Name, object value, DbType ParamType)
{
    IDbDataParameter Param = cmd.CreateParameter();
    Param.DbType = ParamType;
    Param.ParameterName = (Name.StartsWith("@")) ? "" : "@"; //@ character for MS SQL database
    Param.Value = value;

    cmd.Parameters.Add(Param);
}

public static Int32 ExecuteNonQuery(string SQL, IDbCommand cmd = null)
{
    Boolean CommitTrans = true;
    Boolean CloseConn = true;
    SqlTransaction Trans = null;

    try
    {
        //IF Required - create command object if required
        if (cmd == null) { cmd = DB.GetCommandObject(""); }

        //Add the commandtext
        cmd.CommandText = SQL;

        if (cmd.Connection == null) { throw new Exception("No connection set"); }

        //IF REQUIRED - open the connection
        if (cmd.Connection.State == ConnectionState.Closed)
        {
            cmd.Connection.Open();
        }
        else
        {
            CloseConn = false;
        }

        if (cmd.Transaction != null)
        {
            //We have already been passed a Transaction so dont close it
            CommitTrans = false;
        }
        else
        {
            //Create and open a new transaction
            Trans = (SqlTransaction)cmd.Connection.BeginTransaction();
            cmd.Transaction = Trans;
        }

        Int32 rtn = cmd.ExecuteNonQuery();

        if (CommitTrans == true) { Trans.Commit(); }

        return rtn;
    }
    catch (Exception e)
    {
        if (CommitTrans == true) { Trans.Rollback(); }

        throw new Exception();
    }
    finally
    {
        if (CloseConn == true)
        {
            cmd.Connection.Close();
            cmd = null;
        }
    }

}

public static object ExecuteScalar(string SQL, IDbCommand cmd, Boolean NeedsTransaction = true)
{
    Boolean CommitTrans = true;
    Boolean CloseConn = true;
    SqlTransaction Trans = null;

    try
    {
        //IF Required - create command object if required
        if (cmd == null) { cmd = DB.GetCommandObject(""); }

        //Add the commandtext
        cmd.CommandText = SQL;

        //IF REQUIRED - create default Connection to CourseBuilder DB
        if (cmd.Connection == null) { throw new Exception("No Connection Object"); }

        //IF REQUIRED - open the connection
        if (cmd.Connection.State == ConnectionState.Closed)
        {
            cmd.Connection.Open();
        }
        else
        {
            CloseConn = false;
        }

        if (NeedsTransaction == true)
        {
            if (cmd.Transaction != null)
            {
                //We have already been passed a Transaction so dont close it
                CommitTrans = false;
            }
            else
            {
                //Create and open a new transaction
                Trans = (SqlTransaction)cmd.Connection.BeginTransaction();
                cmd.Transaction = Trans;
            }
        }

        Object rtn = cmd.ExecuteScalar();

        if (NeedsTransaction == true && CommitTrans == true) { Trans.Commit(); }

        return rtn;
    }
    catch
    {
        if (NeedsTransaction == true && Trans != null) { Trans.Rollback(); }
        throw new Exception();
    }
    finally
    {
        if (CloseConn == true) { cmd.Connection.Close(); cmd = null; }
    }

}

public static DataRow GetDataRow(string SQL, IDbCommand cmd = null, Boolean ErrorOnEmpty = true)
{
    var dt = FillDatatable(SQL, ref cmd);
    if (dt.Rows.Count > 0)
    {
        return dt.Rows[0];
    }
    else
    {
        if (ErrorOnEmpty == true) { throw new Exception(nameof(GetDataRow) + " returned no rows."); }
        return null;
    }
}

public static DataTable FillDatatable(string SQL, ref IDbCommand cmd)
{

    string newline = System.Environment.NewLine;
    var DT = new DataTable();
    Boolean LeaveConOpen = false;

    try
    {
        //Add the commandtext
        cmd.CommandText = SQL;

        //IF REQUIRED - create default Connection to CourseBuilder DB
        if (cmd?.Connection == null) { throw new Exception("No Connection Object"); }

        if (cmd.Connection.State != ConnectionState.Open)
        {
            cmd.Connection.Open();
            LeaveConOpen = false;
        }
        else
        {
            LeaveConOpen = true;
        }

        var DA = new SqlDataAdapter((SqlCommand)cmd);
        DA.Fill(DT);
    }
    catch (Exception ex)
    {
        var sbErr = new StringBuilder();

        sbErr.AppendLine("Parameters (type defaults to varchar(max)):" + newline);
        foreach (SqlParameter p in cmd.Parameters)
        {
            string s = "";
            sbErr.AppendLine("declare " + p.ParameterName + " varchar(max) = '" + (p.Value == DBNull.Value ? "Null" : p.Value + "'; ") + newline);
        }

        sbErr.AppendLine(newline + SQL + newline);

        throw new Exception("Failed to FillDataTable:" + newline + newline + sbErr.ToString(), ex);

    }
    finally
    {
        if (LeaveConOpen == false) { cmd.Connection.Close(); }

    }

    return DT;

}

public static T CheckNull<T>(T value, T DefaultValue)
{
    if (value == null || value is System.DBNull)
    {
        return DefaultValue;

    }
    else
    {
        return value;
    }
}

Answer 1

I suggest using a light-weight ORM called Dapper. Hides all the dirty work by providing extension methods over ADO.NET. Take a look at the examples here: http://dapper-tutorial.net/execute

Answer 2

1. Alternatives

Just as the other answer suggests, you should be looking into existing framework. There's no use reinventing the wheel unless you're specifically trying to improve it (which is not the case here).

For a professional project, I prefer EF. Not just because of the framework itself, but because it's the most commonly used ORM nowadays (as far as I'm aware), and there is a vast amount of information about using EF available online.

I can't speak to the benefits of using Dapper, no personal experience with it. From a quick browse of some code examples, I'm not a fan of still using SQL in code.
However, if the queries you expect to execute are squeezed for their performance, using SQL may be a benefit to you. EF drops in performance once your queries get very complex.

Unless you're trying to account for complex-yet-performant queries, EF should notably simplify your life.

2. Reviewing the code itself

As mentioned in the earlier chapter, I suggest you move to an ORM. However, I think it's also valuable to review your coding methodology itself, regardless of the existence of ORMs.

• I'm not a fan of the static methods which return objects. Your access layer is closer to a factory than anything else. Your external callers will need to chain the static methods together in order to achieve a meaningful result. If your order of operations changes in the future, you will have to update all external callers to now use the new order of operations.

Instead, what I'd prefer to see is an access layer that functions like an object. Instead of returning an SqlCommand object, return a wrapper object (e.g. MyDataWrapper). Internally, the MyDataWrapper is allowed to use SqlCommand objects. However, your external caller should only be working directly with the MyDataWrapper (see the example below)

• Every method you have for retrieving data all contains the same copy/paste code that checks if the connection is not null, opening the connection, creating a transaction, ... This is leading to a substantial bloat of your code. This is caused by your static methods: every time you call a different method, you need to examine if all paramters have been supplied correctly. If you're using OOP principles instead, then you can have your constructor ensure that all needed objects are present; and you don't need to re-check this for every subsequent method call on the object.

• If you pass your connectionstring for every newly created SqlCommand, then you're making it possible for your data to be saved in different spots. Unless you are specifically creating an application that is intended to connect to many databases, you're better off retrieving your connection string from a singular source (e.g. your app.config file) (see the example below)

• Especially when opening external connections, you'll want to be able to control the closure of the connection at the earliest appropriate time. If you use OOP, then your MyDataWrapper object can implement IDisposable, which allows your to use using() {} statements, which keep your code clean (compared to manually calling Dispose()). This isn't possible on your static methods (it's technically possible to put the static methods in a non-static class which implements IDisposable, but that makes little sense).

Note: the below example is created specifically to showcase the difference between using static methods and using an actual object (OOP). It's not perfect in other aspects, e.g. how it returns an SqlDataReader. That can be further optimized, but I chose to omit further optimizations for clarity's sake.
For your specific case, you're already expecting an external caller to supply an SQL query, which means that expecting that external caller to handle an SqlDataReader is not really a newly introduced problem. Ideally, you'd want your external caller to not use any SQL string nor SqlDataReader, but that is a much bigger refactoring and would require strong knowledge of your business requirements.

Example:

Your data wrapper:

public class MyDataWrapper : IDisposable
{
    SqlConnection _connection;
    SqlCommand _command;

    public MyDataWrapper()
    {
        string theConnectionString = System.Configuration.ConfigurationManager.ConnectionStrings["MyConnString"].ConnectionString;

        _connection = new SqlConnection(theConnectionString);
        _command = new SqlCommand() { Connection = _connection };
    }

    public SqlDataReader Query(string query)
    {
        _connection.Open();

        _command.CommandText = query;
        _command.CommandType = CommandType.Text;

        var reader = _command.ExecuteReader();

        return reader;
    }

    //Scalar and NonQuery methods omitted.

    public void Dispose()
    {
        if (_connection != null) _connection.Close();
    }
}

An example external caller:

public class ExternalCaller
{
    private void TestMethod()
    {
        using(var myDataWrapper = new MyDataWrapper())
        {
            var myDataReader = myDataWrapper.Query("SELECT * FROM Users");
            while(myDataReader.Read())
            {
                //Process each row
            }
        }
    }
}

There are other small things that I could remark, but many of these nitpicks will already disappear (or change shape) if you move towards an OOP approach.

Answer 3

Much of the code can be eliminated if we use the ADO.DataAccessHelper library; its documentation section may help you to know if it meets your requirement.

In brief, it cuts the effort to write and manage connection, command, reader etc. In addition to that, it uses DbProviderFactory, which makes it more flexible to work with different databases.