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.

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.

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.

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.

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