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

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();
}

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.

StackExchange Code Review Q#137879, answer score: 6