.NET MSSQL Wrapper

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 != Syste

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(Func 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 fie

StackExchange Code Review Q#110139, answer score: 3