Optimise MSSQL Connection .NET Helper Library

Question

I recently posted my old .NET MSSQL Wrapper to be code reviewed: .NET MSSQL Wrapper

I had some good feedback and based on that, I was able to re-factor it quite a bit and ended up with the following version: http://pastebin.com/Yhd6RTeS

The above wrapper library is used like this:

Program.cs test console app

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

namespace Test
{
    class Program
    {
        const int MaxDBConnectAttempts = 10;
        private static MSSQLConnection mssqlConnection;
        private static SqlConnection sqlConnection = new SqlConnection("Data Source=DB_SERVER;Initial Catalog=DB_NAME;User ID=DB_USER;Password=DB_PASS;MultipleActiveResultSets=true;");

        static void Main(string[] args)
        {
            try
            {
                // Connect to db
                InitDBConnection();

                // Test int scalar query
                int? totalSyncSetup = mssqlConnection.ExecuteScalar<int?>("select COUNT(*) as countTotal from IC_SyncSetup");
                if (totalSyncSetup.HasValue) Console.WriteLine("Total Sync Setup: {0}\n---", totalSyncSetup);

                // Test string scalar query with where clause
                string magentoOrderLastSync = mssqlConnection.ExecuteScalar<string>("select LastSync from IC_SyncSetup where SyncType = @SyncType",
                    new Dictionary<string, object>()
                    {
                        { "SyncType", "MagentoOrder" }
                    });
                if (!string.IsNullOrEmpty(magentoOrderLastSync)) Console.WriteLine("Magento Order Last Sync: {0}\n---", magentoOrderLastSync);

                // Test insert
                int insertAffectedRows = mssqlConnection.ExecuteNonQuery("insert into IC_SyncSetup (SyncType, LastSync) VALUES (@SyncType, @LastSync)", new Dictionary<string, object>()
                    {
                        { "SyncType", "TestTestType" },
                        { "LastSync", "2015-12-01 00:00:00" }
                    });
                if (insertAffectedRows == 1) Console.WriteLine("New Record Inserted\n---");

                // Test update
                int updateAffectedRows = mssqlConnection.ExecuteNonQuery("update IC_SyncSetup set LastSync = @LastSync WHERE SyncType = @SyncType", new Dictionary<string, object>()
                    {
                        { "LastSync", "2016-12-01 00:00:00" },
                        { "SyncType", "TestTestType" }
                    });
                if (updateAffectedRows > 0) Console.WriteLine("Existing Record(s) Updated\n---");

                // Test reader
                List<Dictionary<string, object>> results = mssqlConnection.ExecuteReader("select * from IC_SyncSetup");
                if (results.Count > 0)
                {
                    Console.WriteLine("Read {0} Rows:", results.Count);
                    foreach (Dictionary<string, object> row in results)
                    {
                        Console.WriteLine("SyncType = {0} | LastSync = {1}",
                            row["SyncType"].ToString(),
                            row["LastSync"].ToString());
                    }
                    Console.WriteLine("---");
                }

                // Test delete
                int deleteAffectedRows = mssqlConnection.ExecuteNonQuery("delete from IC_SyncSetup WHERE SyncType = @SyncType", new Dictionary<string, object>()
                    {
                        { "SyncType", "TestTestType" }
                    });
                if (deleteAffectedRows > 0) Console.WriteLine("Existing Record(s) Deleted\n---");

                // Test bad query 1
                int? testBadQuery1 = mssqlConnection.ExecuteScalar<int?>("select COUNT(*) as countTotal from");
                if (testBadQuery1.HasValue) Console.WriteLine("Total Sync Setup: {0}\n---", testBadQuery1);

                // Test bad query 2
                int? testBadQuery2 = mssqlConnection.ExecuteScalar<int?>("select COUNT(*) as countTotal from IC_SyncSetupXXX");
                if (testBadQuery2.HasValue) Console.WriteLine("Total Sync Setup: {0}\n---", testBadQuery2);

                // Disconnect from db
                DBDisconnect();
            }
            catch (Exception ex)
            {
                Console.WriteLine("Error: {0}\n---", ex.Message);
            }

            Console.WriteLine("Press any key to exit ...");
            Console.ReadKey();
        }

        private static void InitDBConnection()
        {
            int ConnectAttempt = 1;

            while (!DBConnectionOK() && ConnectAttempt < MaxDBConnectAttempts)
            {
                if (ConnectAttempt > 1)
                {
                    Console.WriteLine("Trying to connect to MSSQL DB Server, Attempt #{0}\n---", ConnectAttempt);
                }
                DBDisconnect();
                DBConnect();
                if (!DBConnectionOK())
                {
                    Thread.Sleep(3000);
                }
                ConnectAttempt++;
            }

            if (!DBConnectionOK())
            {
                Console.WriteLine("Could not connect to MSSQL DB Server, after {0} attempts\n---", MaxDBConnectAttempts);
            }
        }

        private static void DBConnect()
        {
            try
            {
                if (sqlConnection.State == ConnectionState.Closed)
                {
                    sqlConnection.Open();
                    mssqlConnection = new MSSQLConnection(sqlConnection);
                    mssqlConnection.OnError += MSSQLConnection_OnError;
                }
            }
            catch (Exception ex) { Console.WriteLine("DB Error: {0}", ex.Message); }
            finally
            {
                if (DBConnectionOK())
                {
                    Console.WriteLine("Success: Connected To MSSQL Database Server\n---");
                }
            }
        }

        private static void MSSQLConnection_OnError(MSSQLConnection sender, DBErrorEventArg e)
        {
            Console.WriteLine("DB Error: {0}\nLast Query: {1}\n---", e.ErrorMsg, e.LastQuery);
        }

        private static void DBDisconnect()
        {
            try
            {
                if (DBConnectionOK())
                {
                    sqlConnection.Close();
                }
            }
            catch { }
        }

        private static bool DBConnectionOK()
        {
            return null != mssqlConnection &&
                null != sqlConnection &&
                sqlConnection.State == ConnectionState.Open &&
                (mssqlConnection.ExecuteScalar<int>("SELECT 1") == 1);
        }
    }
}

And here's the output:

As you can see, the wrapper library works great. What I am not happy with is the way the connection code is separate from the wrapper library, because at the moment it has to exist in the main app (e.g. Program.cs).

Can the connection code (e.g. Connect/Disconnect/Check/etc...) be optimized/clean-up so it's part of the same wrapper library? What's the best way to go about this?

Answer 1

"MSSQL" doesn't mean anything and violates the capitalization conventions:

Do capitalize only the first character of acronyms with three or more characters, except the first word of a camel-cased identifier.

But MSSQL isn't one acronym, it actually consists of two: MS and SQL. And there's a word missing: Server. So it should be MsSqlServer instead of MSSQL. Also, I don't know of any rule that explains why MSSQL should be converted to mssql when camelCased.

---

SqlConnection implements IDisposable and should be used with a using statement, which would probably eliminate most of that ugly code in InitDBConnection and DBDisconnect and DBConnect.

I hope that MSSQLConnection also implements IDisposable, and if so it too should be used in combination with a using statement. But I doubt there's any real point to that class (I cannot access that code since pastebin is blocked where I am now), other than providing ill-advised wrappers around certain methods.

---

Why is DBConnectionOK a method when it behaves like a property? I suspect this is because you've got 150+ lines of code in the Program class, when in fact plenty of it should be in a dedicated class.

---

Why are mssqlConnection and sqlConnection even seperate? Why isn't there a property on MSSQLConnection that exposes its SqlConnection?

I don't understand why MSSQLConnection is even called a "Connection", since it does so much more than a SqlConnection? mssqlConnection.ExecuteScalar<int?>(/* query */) just look very odd to me.

---

I cannot imagine a scenario in which the logic in your InitDBConnection() is applicable: if the DB is down, go sleep for a couple of seconds and then try again? If the db is down, report back that it is down and that's it.

---

Using a Dictionary<string, object> to pass parameters is just throwing away so much functionality, especially when you seem to be passing dates: { "LastSync", "2015-12-01 00:00:00" }. Why not use SqlParameter?

---

If there's an exception, your Main doesn't clean up the mssqlConnection or the sqlConnection. Quite frankly, your Main is a mess anyway: each of those "tests" should be a method on its own at the very least.

But then these tests aren't realistic anyway; they only show how ill-advised your wrapper is. For instance: you're not properly maintaining your connections, which is a huge issue.

Nothing this code does improves on code like the one in this example:

using (SqlConnection sqlConn = new SqlConnection(strConn))
{
    using (SqlCommand sqlCmd = new SqlCommand())
    {
        sqlCmd.CommandText = "INSERT INTO SubjectDetails VALUES ('jQuery')";
        sqlCmd.Connection = sqlConn;
        sqlConn.Open();
        //here execute scalar will get first row first column value
        int retValue = sqlCmd.ExecuteNonQuery();
        sqlConn.Close();
    }
}

But at least the average programmer can look at that code and understand what it does. When he uses your wrapper, he doesn't gain anything IMHO. All you've done is add a layer which more or less obfuscates what is happening.

I would sincerely recommend you to throw away this code and go with established solutions like NHibernate or Entity Framework. If a developer checked in this code at a place I was working and I had to review it, I'd tell him/her exactly that.