Create database connection and run the insert, delete, update queries class

Question

Recently while developing our demo project I had to a write a lot of repetitive code for executing the database queries (insert, update, delete). So, I have put all the repetitive code into a class library, imported and used the library for db related actions i.e. open, close, execute insert, update, and delete queries. I'm wondering if this approach is an efficient way to do so.

 /*Connection manager class */
  public abstract class ConnectionManager
    {
        private string ConnectionString { get; set; }

        public ConnectionManager(string passedOnConstring)
        {
            if (!(String.IsNullOrWhiteSpace(passedOnConstring))
            {
                ConnectionString = passedOnConstring;
            }
            else
            {
                throw new ArgumentException("Invalid string connection, it cannot be null or empty.");
            }
        }

        public abstract SqlConnection OpenConnection();

        public abstract void CloseConnection();
    }

    public class MsSqlConnectionManager : ConnectionManager
    {
        private readonly string _conString;

        private SqlConnection _dbconnect;

        public MsSqlConnectionManager(string passedOnConstring) : base(passedOnConstring)
        {
            _conString = passedOnConstring;
        }

        public override SqlConnection OpenConnection()
        {
            try
            {
                _dbconnect = new SqlConnection(_conString);

                if (_dbconnect.State != System.Data.ConnectionState.Open)
                {
                    _dbconnect.Open();
                }

                return _dbconnect;
            }
            catch (SqlException)
            {
                throw;
            }
        }

        public override void CloseConnection()
        {
            try
            {
                if (_dbconnect.State == System.Data.ConnectionState.Open)
                {
                    _dbconnect.Close();
                }
            }
            catch (SqlException)
            {
                throw;
            }
        }
    }

/*DB Command Class */
  public class DBCommand
{
    private readonly ConnectionManager _conManager;

    private readonly string _dbCommand;

    private int _rowsAffected;

   private readonly Dictionary<string, SqlParameter> _params;

   public DBCommand(string dbCommand, ConnectionManager dbConnection, Dictionary<string, SqlParameter> queryParams)
    {
        if ((dbConnection != null) && (!String.IsNullOrWhiteSpace(dbCommand)) && (queryParams != null))
        {
            _conManager = dbConnection;
            _dbCommand = dbCommand;
            _params = queryParams;
        }
        else
        {
            throw new ArgumentException("Invalid connection string or database command, it cannot be null or empty.");
        }
    }

    public int ExecCmdwithParams()
    {
        try
        {
            using (var _sqlCmd = new SqlCommand(_dbCommand, _conManager.OpenConnection()))
            {
                _sqlCmd.CommandType = CommandType.Text;

                foreach (var keyValuePair in _params)
                {
                    _sqlCmd.Parameters.Add(keyValuePair.Value);
                }

                _rowsAffected = _sqlCmd.ExecuteNonQuery();
            }
            Console.WriteLine(_rowsAffected);
            //Return Affected Rows
        }
        catch (SqlException)
        {
            throw;
        }
        finally
        {
            _conManager.CloseConnection();
        }
        return _rowsAffected;
    }
}

Then I have used the code to execute queries as follows:

/* Main program execution*/
 private static void Main(string[] args)
    {
        string conString = @"";/* connection string */

        string dbCommand = @"INSERT INTO [dbo].[TestTable]([Name]) VALUES (@Name)";

        var _params = new Dictionary<string, SqlParameter>();

        _params["Name"] = new SqlParameter("Name", "Paramatervalue");

        var dbCon = new DBCommand(dbCommand, new MsSqlConnectionManager(conString), _params);

        dbCon.ExecCmdwithParams();

       // Console.ReadLine();
    }

Answer 1

What does the abstract ConnectionManager class buy you ? Nothing, really. The passed-in passedOnConstring is only used for validation, the ConnectionString property is private and isn't used at all. The only thing which is useful is that you can pass a class which extends that class around and call the abstract methods. A much better way would be to define an interface doing the same and let the classes, which implement that interface, handle the validation of the connection string themselves.

What does this class manage ? It is only opening and closing a connection but doesn't manage anything.

The other problem I see is that you have an abstract method which returns a SqlConnection which is only used by MS Sql. What if you decide to use MySql or PostgreSql ?

There is the DbConnection which is extended by SqlConnection and at least by MySqlConnection and I suggest using this instead.

That being said, let's dig through your code.

public ConnectionManager(string passedOnConstring)
{
    if (!(String.IsNullOrWhiteSpace(passedOnConstring))
    {
        ConnectionString = passedOnConstring;
    }
    else
    {
        throw new ArgumentException("Invalid string connection, it cannot be null or empty.");
    }
}  

It's much easier to read if one isn't using "negative" results and this makes the use of an else redundant as well, like so

    public ConnectionManager(string passedOnConstring)
    {
        if ((String.IsNullOrWhiteSpace(passedOnConstring))
        {
            throw new ArgumentException("Invalid string connection, it cannot be null or empty.");    
        }

        ConnectionString = passedOnConstring;
    }

and the exception message would read better "Invalid connection string...."

---

public override SqlConnection OpenConnection()
{
    try
    {
        _dbconnect = new SqlConnection(_conString);

        if (_dbconnect.State != System.Data.ConnectionState.Open)
        {
            _dbconnect.Open();
        }

        return _dbconnect;
    }
    catch (SqlException)
    {
        throw;
    }
}  

This try..catch is superfluous because if a SqlException is thrown you are rethrowing it and all other possible exceptions are not caught. You can simply remove that try..catch. But what bothers me more is that you never ever dispose any of the created connections.

---

DBCommand

I like that you are using readonly for the variables which aren't allowed to change. That's a good habit but this also results in this case that your class can be used only for one trip to the database because the dictionary you are using for the execution can't be changed can't be assigned to the variable twice. Sure that wouldn't mean that you can't change the content of that Dictionary but you could do this, having your current code, only from the outside of the class which I would consider to be a side effect because you are changing the state of the ConnectionManager class from outside of the class.

For readonly please see @EBrown's good answer.

If you would pass that dictionary to the ExecCmdwithParams() method instead you could reuse the object.

---

I would like to suggest that you dig into the DbConnection, DbCommand, DbProviderFactory, DbProviderFactories and DbDataAdapter classes which are all living in the System.Data.Common namespace. Using this classes will abstract away the underlying database system.

Answer 2

One note I think you should have:

I'm not sure if you realize or not, but readonly doesn't stop the underlying value of the field from changing. It stops assignments from affecting it.

What I mean is, the following is perfectly valid:

void Main()
{
    var tc = new TestClass();
    Console.WriteLine($"Items count: {tc.Items.Count}");
    tc.Items.Add("Test 1", 5);
    Console.WriteLine($"Items count: {tc.Items.Count}");
    tc.Items.Clear();
    Console.WriteLine($"Items count: {tc.Items.Count}");
}

class TestClass
{
    private readonly Dictionary<string, int> _items;

    public Dictionary<string, int> Items => _items;

    public TestClass()
    {
        _items = new Dictionary<string, int>();
    }
}

Output:

Items count: 0
Items count: 1
Items count: 0

The following is invalid:

tc.Items = new Dictionary<string, int>();

As well as the following:

public void SomeMethod()
{
    _items = new Dictionary<string, int>();
}

Essentially, the collection can still be modified after instantiation, it just cannot be set directly.

Instead of exposing _params as a Dictionary<string, SqlParameter>, expose it as an IEnumerable<KeyValuePair<string, SqlParameter>>. This prevents any accidental .Add or .Clear method calls. You can still directly assign a Dictionary<string, SqlParameter> to an IEnumerable<KeyValuePair<string, SqlParameter>>, so no other code has to change. (Isn't .NET magical?)

---

Of course, with casting, your collection can still be modified. That's not a likely scenario though.

What do I mean by this? Let's take a look:

void Main()
{
    var tc = new TestClass();
    Console.WriteLine($"Items count: {((Dictionary<string, int>)tc.Items).Count}");
    ((Dictionary<string, int>)tc.Items).Add("Test 1", 5);
    Console.WriteLine($"Items count: {((Dictionary<string, int>)tc.Items).Count}");
    ((Dictionary<string, int>)tc.Items).Clear();
    Console.WriteLine($"Items count: {((Dictionary<string, int>)tc.Items).Count}");
}

class TestClass
{
    private readonly IEnumerable<KeyValuePair<string, int>> _items;

    public IEnumerable<KeyValuePair<string, int>> Items => _items;

    public TestClass()
    {
        _items = new Dictionary<string, int>();
    }
}

And our output:

Items count: 0
Items count: 1
Items count: 0

Even though the exposed collection is a readonly IEnumerable, exposed as a get-only property, I still managed to modify it through the underlying Dictionary that supports it, via casting. This essentially says, "I know I assigned a Dictionary to this, let me use it as such" and the compiler lets us get away with it.

---

To prevent all instances of this, use the ReadOnlyDictionary<TKey, TValue> located in System.Collections.ObjectModel in the System.Collections library.