.NET MSSQL Wrapper

Question

Back in late 2012, I wrote the following C# .NET MSSQL Wrapper, which offered the following operations:

• ExecuteScalar (for selecting single field from a row)
• ExecuteReader (for selecting one or more row)
• ExecuteNonQuery (for usual CRUD ops - uses transaction)
• Handle deadlock with retries

This wrapper was used in a Desktop application that ran in a the task bar as a sync software. I need to re-write the application as a windows service app. Before I wanted to start on the new project, I wanted to see what the stack exchange community thinks of my MSSQL DB Wrapper. Is it any good? can I improve it?

MssqlDbHelper.cs

using System;
using System.Collections.Generic;
using System.Data.SqlClient;
using System.Threading;
using System.Diagnostics;

namespace MssqlDbHelper
{
    #region Db Exception EventArg Definition

    public class DbErrorEventArg : EventArgs
    {
        private String _ErrorMsg;
        public String ErrorMsg
        {
            set { this._ErrorMsg = value; }
            get { return this._ErrorMsg; }
        }
        private String _LastQuery;
        public String LastQuery
        {
            set { this._LastQuery = value; }
            get { return this._LastQuery; }
        }
    }

    #endregion

    public class MssqlDb
    {
        #region Private Fields & Properties

        private SqlConnection MySqlConnection;
        private Int32 MyTimeout;
        private String LastQuery = String.Empty;
        private String StackInfo = String.Empty;
        public event ErrorHandler OnError;
        public delegate void ErrorHandler(MssqlDb sender, DbErrorEventArg e);

        #endregion

        #region Class Constructor

        public MssqlDb(SqlConnection P_SqlConnection, Int32 P_Timeout = 120)
        {
            MySqlConnection = P_SqlConnection;
            if (null == MySqlConnection)
                throw new Exception("Invalid MSSQL Database Conection Handle");
            if (MySqlConnection.State != System.Data.ConnectionState.Open)
                throw new Exception("MSSQL Database Connection Is Not Open");
            MyTimeout = P_Timeout;
        }

        #endregion

        #region Db Exception Error Logging Functions (Raise Class Event)

        internal void LogDbException(String ErrorMsg)
        {
            if (OnError != null)
            {
                OnError(this, new DbErrorEventArg()
                {
                    ErrorMsg = ErrorMsg,
                    LastQuery = LastQuery
                });
            }
        }

        #endregion

        #region Deadlock Handler [ Retry Deadlock & Finally Error ]

        private T Retry<T>(Func<T> MyFunc)
        {
            try
            {
                int RetryCount = 30;
                TimeSpan RetryDelay = TimeSpan.FromSeconds(1);
                while (true)
                {
                    try
                    {
                        if (MySqlConnection.State == System.Data.ConnectionState.Closed)
                        {
                            MySqlConnection.Open();
                        }
                        return MyFunc();
                    }
                    catch (SqlException e)
                    {
                        --RetryCount;
                        if (RetryCount <= 0) { throw; }
                        if (e.Number == 1205)
                        {
                            LogDbException("[ Attempts Left : " + RetryCount + " ] Failed to execute Query, SQL Deadlock occured. Retrying again in " + RetryDelay.Seconds + " seconds ...");
                        }
                        else if (e.Number == -2)
                        {
                            LogDbException("[ Attempts Left : " + RetryCount + " ] Failed to execute Query, SQL Timeout occured. Retrying again in " + RetryDelay.Seconds + " seconds ...");
                        }
                        else { throw; }
                        Thread.Sleep(RetryDelay);
                    }
                }
            }
            catch (Exception e)
            {
                LogDbException("Failed to execute SQL Query : " + e.Message);
                return default(T);
            }
        }

        private void Retry(Action MyAction)
        {
            Retry(() => { MyAction(); return true; });
        }

        #endregion

        #region Database Query Functions

        public T ExecuteScalar<T>(String MyQuery)
        {
            LastQuery = MyQuery;
            return Retry(() =>
            {
                using (SqlCommand MySqlCommand = new SqlCommand(MyQuery, MySqlConnection))
                {
                    MySqlCommand.CommandTimeout = MyTimeout;
                    object Result = MySqlCommand.ExecuteScalar();
                    if (null == Result || Result is DBNull)
                    {
                        return default(T);
                    }
                    Type CastType = typeof(T);
                    if (CastType == typeof(int))
                    {
                        try { Result = Int32.Parse(Result.ToString()); }
                        catch { Result = default(T); }
                    }
                    else if (CastType == typeof(decimal))
                    {
                        try { Result = Decimal.Parse(Result.ToString()); }
                        catch { Result = default(T); }
                    }
                    else if (CastType == typeof(string))
                    {
                        try { Result = Result.ToString(); }
                        catch { Result = default(T); }
                    }
                    return (T)Result;
                }
            });
        }
        public T ExecuteScalar<T>(String MyQuery, params object[] MyQueryParams)
        {
            return this.ExecuteScalar<T>(String.Format(MyQuery, MyQueryParams));
        }

        public List<Dictionary<String, Object>> ExecuteReader(String MyQuery)
        {
            LastQuery = MyQuery;
            return Retry(() =>
            {
                using (SqlCommand MySqlCommand = new SqlCommand(MyQuery, MySqlConnection))
                {
                    MySqlCommand.CommandTimeout = MyTimeout;
                    List<Dictionary<string, object>> Results = new List<Dictionary<string, object>>();
                    using (SqlDataReader MySqlDataReader = MySqlCommand.ExecuteReader())
                    {
                        if (null != MySqlDataReader)
                        {
                            if (MySqlDataReader.HasRows)
                            {
                                while (MySqlDataReader.Read())
                                {
                                    Dictionary<string, object> ResultData = new Dictionary<string, object>();
                                    for (int i = 0; i < MySqlDataReader.FieldCount; i++)
                                    {
                                        string ColumnName = MySqlDataReader.GetName(i);
                                        object ColumnData = MySqlDataReader[ColumnName];
                                        ResultData.Add(ColumnName, ColumnData);
                                    }
                                    Results.Add(ResultData);
                                }
                            }
                        }
                        MySqlDataReader.Close();
                    }
                    return Results;
                }
            });
        }
        public List<Dictionary<String, Object>> ExecuteReader(String MyQuery, params object[] MyQueryParams)
        {
            return this.ExecuteReader(String.Format(MyQuery, MyQueryParams));
        }

        public Int32 ExecuteNonQuery(String MyQuery)
        {
            LastQuery = MyQuery;
            return Retry(() =>
            {
                int AffectedRows = 0;
                using (SqlTransaction MySqlTransaction = MySqlConnection.BeginTransaction())
                {
                    using (SqlCommand MySqlCommand = new SqlCommand(MyQuery, MySqlConnection, MySqlTransaction))
                    {
                        MySqlCommand.CommandTimeout = MyTimeout;
                        AffectedRows = MySqlCommand.ExecuteNonQuery();
                        MySqlTransaction.Commit();
                    }
                }
                return AffectedRows;
            });
        }
        public Int32 ExecuteNonQuery(String MyQuery, params object[] MyQueryParams)
        {
            return this.ExecuteNonQuery(String.Format(MyQuery, MyQueryParams));
        }

        #endregion
    }
}

Usage

using MssqlDbHelper;
using System.Data.SqlClient;

// SQL Connection Ojbect
private MssqlDb MyDb;
private SqlConnection MssqlConnection = new SqlConnection(Settings.Default.DB_Connection);

#region Mssql Db Connection Functions

private void DbConnect()
{
    try
    {
        if (MssqlConnection.State == ConnectionState.Closed)
        {
            MssqlConnection.Open();
            MyDb = new MssqlDb(MssqlConnection);
            MyDb.OnError += new MssqlDb.ErrorHandler(Db_OnError);
        }
    }
    catch (Exception ex) { Utils.ShowException(ex); }
    finally
    {
        if (DbConnectionOk())
        {
            this.UIThread(() => { logsViewer.AddEntry(LogLevel.Success, "Connected To MSSQL Database Server"); });
        }
    }
}

void Db_OnError(MssqlDb sender, DbErrorEventArg e)
{
    this.UIThread(() => { logsViewer.AddEntry(null, LogLevel.Error, e.ErrorMsg + " | Last Query : " + e.LastQuery); });
}

private void DbDisconnect()
{
    try
    {
        if (DbConnectionOk())
        {
            MssqlConnection.Close();
        }
    }
    catch { }
}

private Boolean DbConnectionOk()
{
    return null != MyDb &&
        null != MssqlConnection &&
        MssqlConnection.State == ConnectionState.Open &&
        (MyDb.ExecuteScalar<int>("SELECT 1") == 1);
}

private void InitDbConnection()
{
    int ConnectAttempt = 1;
    while (!DbConnectionOk() && ConnectAttempt < Settings.Default.Max_Db_Connect_Attempts)
    {
        if (ConnectAttempt > 1)
        {
            this.UIThread(() => { logsViewer.AddEntry(LogLevel.Info, "Trying to connect to MSSQL DB Server, Attempt #" + ConnectAttempt); });
        }
        DbDisconnect();
        DbConnect();
        if (!DbConnectionOk())
        {
            Thread.Sleep(3000);
        }
        ConnectAttempt++;
    }
    if (!DbConnectionOk())
    {
        this.UIThread(() => { logsViewer.AddEntry(LogLevel.Error, "Could not connect to MSSQL DB Server, after " + Settings.Default.Max_Db_Connect_Attempts + " attempts"); });
    }
}

#endregion

// Usage
try
{
    // Init database connection
    InitDbConnection();

    // Select single string field
    string stringValue = MyDb.ExecuteScalar<string>(
        "SELECT field FROM some_table WHERE condition = 'paramValue'");

    // Select single int field
    int intValue = MyDb.ExecuteScalar<int>(
        "SELECT COUNT(*) AS countTotal FROM some_table WHERE condition = '{0}'",
            paramValue);

    // Select one or more rows
    List<Dictionary<string, object>> Results = MyDb.ExecuteReader(
        "SELECT Id AS QueueId, Name FROM SomeTable WHERE Synced = 0");
    foreach (Dictionary<string, object> Row in Results)
    {
        // (int)Row["QueueId"]
        // Row["Name"].ToString()
    }

    // Delete query
    MyDb.ExecuteNonQuery("DELETE FROM some_table WHERE condition = '{0}'",
        paramValue);

    // You get the idea...

    // Close database connection
    DbDisconnect();
}
catch (Exception ex)
{
    // Handle ex
}

Answer 1

public class DbErrorEventArg : EventArgs
{
    private String _ErrorMsg;
    public String ErrorMsg
    {
        set { this._ErrorMsg = value; }
        get { return this._ErrorMsg; }
    }
    private String _LastQuery;
    public String LastQuery
    {
        set { this._LastQuery = value; }
        get { return this._LastQuery; }
    }
}  

• If you don't need to have any validation in the setter of a property you should consider using auto-implemented properties.

• The idiomatic way would be to use the string alias instead of the String class.

• Why should a consumer of an event which passes this event arguments be able to change any of the properties ? Using a private setter in combination with a constructor which takes the values will make this better.

Implementing the mentioned points will lead to

public class DbErrorEventArg : EventArgs
{

    public string ErrorMsg { get; private set; }
    public string LastQuery { get; private set; }

    public DbErrorEventArg (string errorMesg, string lastQuery)
    {
        ErrorMsg = errorMsg;
        LastQuery = lastQuery;
    }
}

---

MssqlDb

private SqlConnection MySqlConnection;  

this is just misleading. Having a class named MssqlDb shouldn't have a field named MySqlConnection. Naming the field just connection would be much better. While talking about fields, fields (variables) should be named using camelCase casing. See NET naming guidelines

public MssqlDb(SqlConnection P_SqlConnection, Int32 P_Timeout = 120)
{
    MySqlConnection = P_SqlConnection;
    if (null == MySqlConnection)
        throw new Exception("Invalid MSSQL Database Conection Handle");
    if (MySqlConnection.State != System.Data.ConnectionState.Open)
        throw new Exception("MSSQL Database Connection Is Not Open");
    MyTimeout = P_Timeout;
}  

Due to the lack of vertical space (new lines) which could group related code, this is hard to read.

Regarding the constructor arguments, please see the naming guideline.

Not using braces {} although they might be optional can lead to serious bugs and makes your code error prone. The decision if you use braces or don't is yours to decide, but if you choose one style you should stick to it. Right now you are using both styles.

internal void LogDbException(String ErrorMsg)
{
    if (OnError != null)
    {
        OnError(this, new DbErrorEventArg()
        {
            ErrorMsg = ErrorMsg,
            LastQuery = LastQuery
        });
    }
}  

A very misleading method name. If I read this name I will suppose that there is some logging taken place. To fix any threading issues the OnError delegate should be assigned to a local variable like so

internal void LogDbException(String ErrorMsg)
{
    var errorDelegate = OnError;
    if (errorDelegate != null)
    {
        errorDelegate (this, new DbErrorEventArg()
        {
            ErrorMsg = ErrorMsg,
            LastQuery = LastQuery
        });
    }
}  

private T Retry<T>(Func<T> MyFunc)
{
    try
    {
        int RetryCount = 30;
        TimeSpan RetryDelay = TimeSpan.FromSeconds(1);
        while (true)
        {
            try
            {
                if (MySqlConnection.State == System.Data.ConnectionState.Closed)
                {
                    MySqlConnection.Open();
                }
                return MyFunc();
            }
            catch (SqlException e)
            {
                --RetryCount;
                if (RetryCount <= 0) { throw; }
                if (e.Number == 1205)
                {
                    LogDbException("[ Attempts Left : " + RetryCount + " ] Failed to execute Query, SQL Deadlock occured. Retrying again in " + RetryDelay.Seconds + " seconds ...");
                }
                else if (e.Number == -2)
                {
                    LogDbException("[ Attempts Left : " + RetryCount + " ] Failed to execute Query, SQL Timeout occured. Retrying again in " + RetryDelay.Seconds + " seconds ...");
                }
                else { throw; }
                Thread.Sleep(RetryDelay);
            }
        }
    }
    catch (Exception e)
    {
        LogDbException("Failed to execute SQL Query : " + e.Message);
        return default(T);
    }
}

I don't like this double try..catch style. You should try to refactor this.

The magic numbers 1205 and -2 should be extracted to meaningful constants. Do you still know what the numbers mean ? I don't think so.

It would be better to compose the error messages by using string.Format() instead of string concatenation. For the general exception, you should use the StackTrace value too.

public T ExecuteScalar(String MyQuery)

Instead of using Parse() in combination with try..catch you should use TryParse().

Having ToString() in a try..catch for typeof(string) isn't needed, because this won't throw (for 99.99999%).

This would lead to (also renaming the local fields)

public T ExecuteScalar<T>(String query)
{
    LastQuery = query;
    return Retry(() =>
    {
        using (SqlCommand sqlCommand = new SqlCommand(query, MySqlConnection))
        {
            sqlCommand.CommandTimeout = MyTimeout;
            object result = sqlCommand.ExecuteScalar();

            if (null == result || result is DBNull)
            {
                return default(T);
            }

            Type type = typeof(T);
            if (type == typeof(int))
            {
                int value;
                int.TryParse(result.ToString(), out value);
                return value;
            }

            if (type == typeof(decimal))
            {
                decimal value;
                decimal.TryParse(result.ToString(), out value);
                return value;
            }

            if (type == typeof(string))
            {
                return result.ToString();
            }
            return (T)result;
        }
    });
}

---

public List<Dictionary<String, Object>> ExecuteReader(String MyQuery)
{
    LastQuery = MyQuery;
    return Retry(() =>
    {
        using (SqlCommand MySqlCommand = new SqlCommand(MyQuery, MySqlConnection))
        {
            MySqlCommand.CommandTimeout = MyTimeout;
            List<Dictionary<string, object>> Results = new List<Dictionary<string, object>>();
            using (SqlDataReader MySqlDataReader = MySqlCommand.ExecuteReader())
            {
                if (null != MySqlDataReader)
                {
                    if (MySqlDataReader.HasRows)
                    {
                        while (MySqlDataReader.Read())
                        {
                            Dictionary<string, object> ResultData = new Dictionary<string, object>();
                            for (int i = 0; i < MySqlDataReader.FieldCount; i++)
                            {
                                string ColumnName = MySqlDataReader.GetName(i);
                                object ColumnData = MySqlDataReader[ColumnName];
                                ResultData.Add(ColumnName, ColumnData);
                            }
                            Results.Add(ResultData);
                        }
                    }
                }
                MySqlDataReader.Close();
            }
            return Results;
        }
    });
}

• you have some naming issues regarding the names and the casing
• by enclosing the SqlDataReader in an using block the reader will be automatically closed.
• instead of having such arrow code you should return early if possible.
• the Read() method of the datareader returns false if there aren't anymore rows to read, so it is superflous to check for HasRows

Implementing these changes lead to

public List<Dictionary<String, Object>> ExecuteReader(String query)
{
    LastQuery = query;
    return Retry(() =>
    {
        using (SqlCommand command = new SqlCommand(query, MySqlConnection))
        {
            command.CommandTimeout = MyTimeout;
            List<Dictionary<string, object>> results = new List<Dictionary<string, object>>();
            using (SqlDataReader reader = command.ExecuteReader())
            {
                if (null == reader) { return results; }

                while (reader.Read())
                {
                    Dictionary<string, object> resultData = new Dictionary<string, object>();
                    for (int i = 0; i < reader.FieldCount; i++)
                    {
                        string columnName = reader.GetName(i);
                        object value = reader[columnName];
                        resultData.Add(columnName, value);
                    }
                    results.Add(resultData);
                }

            }
            return results;
        }
    });
}  

---

The remaining code just needs some facelifting regarding method arguments and variable names.

Using regions can be considered as code smell. See: Are #regions an antipattern or code smell?