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

• BCdotWEB: "I had to a write a lot of repetitive code for executing the database queries" But... why? "if you’re writing ADO.Net code by hand, you’re stealing from your employer or client. " Instead, use an ORM.
• Heslacher: I have rolled back the last edit. Please see what you may and may not do after receiving answers .

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.

• Heslacher: @EBrown that readonly stuff had been a lack of the english language. I have updated my answer.
• t3chb0t: Using this classes will abstract away the underlying database system. This is true only up to a point. As soon as you need to use parameters you need to/should know their exact types and they are different between ms sql, sqlite, mysql so you're back to the roots and the whole abstraction is usless :-( unless you have another abstraction for all the data types and you can map them from .net to the data provider.
• Heslacher: @t3chb0t I have added a simplified dal to the follow up: codereview.stackexchange.com/a/138235/29371

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.