Singleton Database class

Question

Currently my team is building an application involving a database. We have to write a lot of data (approx 2,000,000 records) to the database and therefore we decided to open a connection once and close it afterwards. The connection will become a local connection, so we don't have to worry about unwanted intruders that much.

The code works as it is supposed to, however we are trying to make sure every little bit is optimized to make it as efficient as possible. Is there anything that could be done better?

public class Database
{
 /// <summary>
 /// The connection which is established when connecting to the database.
 /// </summary>
 private MySqlConnection _conn;
 /// <summary>
 /// A command which can be used to execute queries.
 /// </summary>
 private MySqlCommand _cmd;
 /// <summary>
 /// The private singleton instance of the database.
 /// </summary>
 private static Database _instance;
 /// <summary>
 /// The constructor.
 /// </summary>
 private Database() {} 
 /// <summary>
 /// The public singleton instance of the database
 /// </summary>
 public static Database Instance
 {
 get
 {
 if (_instance == null)
 {
 //Create a new instance if it is not already done.
 _instance = new Database();
 }
 return _instance;
 }
 }
 /// <summary>
 /// Return whether the connection is open or not.
 /// </summary>
 public bool isConnected
 {
 get { return _conn.State == ConnectionState.Open; }
 }
 /// <summary>
 /// This method will let the user connect to the database with a given connection string.
 /// </summary>
 /// <param name="conStr">The connection string</param>
 public void Connect(string conStr)
 {
 //Make the connection
 _conn = new MySqlConnection(conStr);
 //Open the connection
 _conn.Open();
 }
 /// <summary>
 /// This method will let the user connect to the database given specific values.
 /// </summary>
 /// <param name="ip">The IP-address of the database (127.0.0.1 for local)</param>
 /// <param name="db">The name of the database</param>
 /// <param name="uid">The user ID</param>
 /// <param name="pw">The password</param>
 public void Connect(string ip, string db, string uid, string pw)
 {
 //Make the connection
 _conn = new MySqlConnection(@"Data Source=" + ip + ";Database=" + db + ";Uid=" + uid + ";Pwd=" + pw + ";Allow User Variables=True");
 //Open the connection
 _conn.Open();
 }
 /// <summary>
 /// This method will execute the given query and will return the result given from the database
 /// </summary>
 /// <param name="query">The query</param>
 /// <returns>The result given from the database</returns>
 public DataTable Read(string query)
 {
 DataTable _resultTable = new DataTable();
 //Only procede if there is a connection. Return null otherwise.
 if (_conn == null)
 {
 return null;
 }
 //Create the command with the gien query
 _cmd = new MySqlCommand(query, _conn);
 //We need MySqlDataAdapter to store all rows in the datatable
 using (MySqlDataAdapter adapter = new MySqlDataAdapter(_cmd))
 {
 adapter.Fill(_resultTable);
 }
 //Return the result.
 return _resultTable;
 }
}

Clarification

• The _cmd is only created once and hence private. We use this MySqlCommand multiple times in multiple methods, and therefore we reuse _cmd every time.
• The _conn is only created once since we only want to make one connection.
• The use of MySqlDataAdapter is on purpose, because DataTable.Load(_cmd.ExecuteReader()) method will overwrite rows when multiple corresponding primary keys are given by a query.

There are more methods that involve the creation of INSERT and DELETE queries, but they are not of any use here.

Note: We encourage the use of comments in the code since we are recruiting new team members and therefore we want to make the code more readable. I added the comments in my post, but if you want me to delete them, tell me so.

• EngineerBetter_DJ: I would strongly recommend against this. I made the same mistake in my first job. As soon as you need to add another connection type (say, to Oracle) you'll end up with either a dirty API or race conditions, and having a singleton also makes it hard to test.
• Heslacher: What you can and cannot do after receiving answers
• Dion V.: @Heslacher my bad, I didn't know that. I will unedit it and post it as an answer. Thanks for pointing it out.
• Heslacher: I have already rolled back your edit. Also if you post it as answer be sure to post it as a review.

Answer 1

• You shouldn't be sharing Connection instances, because ADO is already doing the job for you, it uses object pooling behind the scenes. Sharing the Connection object will make it very hard to clean up your resources after you are done with it.
• Use interfaces instead of concrete implementation.Use IDbConnection instead of MySQLConnection

• You should use IDbCommands with IDbParameters rather than passing string to query the database

  public DataTable Read(string query) // DANGER!!!
  

But the most important thing is, forget about using a singleton for that. Open a new connection and close it afterwards, and ADO will take care of connection pooling,you might need to add pooling=true to your connection string.

• Dion V.: What do you exactly mean by "sharing Connection"? As far as I know, since the class is a singleton it does not share any connection. It just uses one, and every query is sent through the same class with the one connection. You do have a point on the parameters though, any idea how you would implement it?
• Dion V.: I just saw your edit. Can you explain why you would not want to use a singleton? Isn't it very heavy for the system to open and close the connection for every query, especially when I have to write so many of them?
• Sleiman Jneidi: @DionV.exactly, I said sharing connection and not connections
• Sleiman Jneidi: @DionV. Because it is done for you using connection pooling
• Sleiman Jneidi: @BCdotNET But he is using MySQL and not MSSQL

Answer 2

Just commenting on ... the comments in the source (sorry): I think they add a lot of noise, and don't provide any improved clarity (in this case). They're mostly restating what the following line of code obviously does, or repeating what a variable name already describes.

Such comments "by habbit" are actually harmful, as they have no benefit and could prevent authors from making the code clear in the first place.

• cja: Exactly. Massively overcommented. The code is readable by itself. Changing the code is extra work because the comments have to be read and changed too.
• Dion V.: Are you referring to all the comments or just those that are not in the summary? Because the summary is shown when accessing a method/property/field which could actually help when programming. I have to admit those summaries fill a lot of space, but it is still useful.
• Gnurfos: I am referring to both kind. The summaries too are "noise" (even leaving the different question of html tags aside). They are not "summarizing" but just repeating.
• Gnurfos: Example: /// The private singleton instance of the database. private static Database _instance; is just saying the exact same thing twice (except the comment is missing the "static" part)
• Gnurfos: Try removing the comments, looking at the code, and asking yourself "is this any less clear" ? It might be, for someone not knowing the language, but this is code, probably not meant to be read by such people (and they'd have trouble understanding some comments like "return null" anyway).

Answer 3

First, you can use implicit typing when the type is easily determined from the RHS, so:

DataTable _resultTable = new DataTable();

becomes:

var _resultTable = new DataTable();

etc. This saves you changing the type of a variable in more than one place (declaration and instantiation).

Secondly, I'd recommend some abstracting, particularly the MySQL stuff.

/// <summary>
/// The connection which is established when connecting to the database.
/// </summary>
public IDbConnection _conn;
/// <summary>
/// A command which can be used to execute queries.
/// </summary>
public IDbCommand _cmd;

And then have an IOC framework, or your code fill in these dependencies.

This way if you decide to move to a different DB provider, it will be a much simpler refactor, and this class shouldn't need touching. This also lets your mock the database, allowing you to properly Unit Test this code.

Of course, the downside is that the connection and command properties can be altered on the fly, which can be a problem. To prevent that, keep those properties as fields and this database class as abstract base class, and create subclasses for each DB provider you wish to support. Have those subclasses instantiate the fields yourself. Then most of the code remains testable, and it remains unchangeable.

• Dion V.: I personally don't like using var as it makes my code less readible IMHO. I did my research on performance and found out that it does not improve when using var. Also, citing from Jon Skeet; "It's a personal preference though" - here. I will definitely look into the abstracting, thanks!
• Nick Udell: I personally find it easier to only have to change a type in one place when I refactor, which I would consider enough of a bonus to merit using it, however it is definitely a personal preference and if you don't intend on refactoring much you likely wouldn't feel the benefit.
• RubberDuck: @DionV. reviewers are free to comment on any and all aspects of the code in question. You can ask for reviewers to focus on or ignore something, but they're under no obligation to do so.
• RubberDuck: @DionV. You said, "I did my research on performance and found out that it does not improve when using var". I am pointing out that answers don't have to address just performance issues. It's also not just a matter of preference. It's in the official style guide.
• Dion V.: @RubberDuck with that sentence I just wanted to make clear that I did put some effort into the decision of using var or not. And even though it is not the official style does not make my code wrong, does it? I would like to add to that that I actually do respect it being pointed out by the poster. I am just not wanting to use it for my own reasons.

Answer 4

I would suggest that if your application is intended to work in multi threaded environment then update your instance creation logic and add locks to that.

private static object syncRoot = new Object();
/// <summary>
/// The public singleton instance of the database
/// </summary>
public static Database Instance
{
 get
 {
 if (instance == null) 
 {
 lock (syncRoot) 
 {
 if (instance == null) 
 {
 //Create a new instance if it is not already done.
 _instance = new Database();
 }
 }
 }
 return instance;
 }
}

Answer 5

After reading the answers, I figured the best practice is to open the connection for every query. I have read that ADO.NET will then manage my pooling, and I will benefit from this feature because C# will manage my resources better. Correct me if I'm wrong.

My code will then be something like this:

public class Database
{
 /// <summary>
 /// The private singleton instance of the database.
 /// </summary>
 private static Database _instance;
 /// <summary>
 /// This will be the connectionstring used for connecting to the database
 /// </summary>
 private string _conStr;
 /// <summary>
 /// The constructor.
 /// </summary>
 private Database(string conStr) 
 {
 _conStr = conStr;
 } 
 /// <summary>
 /// The public singleton instance of the database
 /// </summary>
 public static Database Instance
 {
 get
 {
 if (_instance == null)
 {
 //Create a new instance if it is not already done.
 _instance = new Database();
 }
 return _instance;
 }
 }
 /// <summary>
 /// This method will execute the given query and will return the result given from the database
 /// </summary>
 /// <param name="query">The query</param>
 /// <param name="params">The params for the query</param>
 /// <returns>The result given from the database</returns>
 public DataTable Read(string query, Dictionary<string, string> params)
 {
 DataTable resultTable = new DataTable();
 using(MySqlConnection con = new MySqlConnection(_conStr))
 {
 con.Open(); 
 //Create the command with the given query
 using(MySqlCommand cmd = new MySqlCommand(query, con))
 {
 foreach(KeyValuePair<string, string> pair in params)
 {
 cmd.Parameters.AddWithValue(pair.Key, pair.Value);
 }
 //We need MySqlDataAdapter to store all rows in the datatable
 using (MySqlDataAdapter adapter = new MySqlDataAdapter(cmd))
 {
 adapter.Fill(resultTable);
 }
 }
 //Return the result.
 return resultTable;
 }
}

• Alex: It's better, but it would be even better if you stopped using the dreaded Singleton pattern: blogs.msdn.com/b/scottdensmore/archive/2004/05/25/140827.aspx
• Casey: Another good piece on why singletons are bad to consider.
• Dion V.: Thanks both. I have read both articles and am convinced the Singleton is unneccessary. I will discuss it with the team as soon as possible!
• Nick Udell: This could still use some abstraction through interfaces where possible.