MySQL database wrapper

Question

Here is a wrapper to a MySQL connection.

First I have a DatabaseController and then a DatabaseConnection.

How I use this is shown below.

using (var dbConnection = Program.DatabaseController.NewConnection)
{
}

Here is the full code for both classes.

internal sealed class DatabaseController
{
    private readonly string _connectionString;

    public DatabaseController(string connectionString)
    {
        _connectionString = connectionString;
    }

    public DatabaseConnection Connection => new DatabaseConnection(_connectionString);

    public bool ConnectionWorks()
    {
        try
        {
            using (Connection)
            {
            }

            return true;
        }
        catch (Exception)
        {
            return false;
        }
    }
}

internal sealed class DatabaseConnection : IDisposable
{
    private static readonly ILogger Logger = LogManager.GetCurrentClassLogger();
    private MySqlCommand _command;

    private MySqlConnection _connection;
    private List<MySqlParameter> _parameters;

    public DatabaseConnection(string connectionString)
    {
        _connection = new MySqlConnection(connectionString);
        _command = _connection.CreateCommand();

        OpenConnection();
    }

    public void Dispose()
    {
        Dispose(true);
    }

    private void OpenConnection()
    {
        if (_connection.State == ConnectionState.Open)
        {
            throw new InvalidOperationException("Connection already open.");
        }

        _connection.Open();
    }

    public void AppendParameter(string key, object value)
    {
        if (_parameters == null)
        {
            _parameters = new List<MySqlParameter>();
        }

        _parameters.Add(new MySqlParameter(key, value));
    }

    public void SetQuery(string query)
    {
        _command.CommandText = query;
    }

    public int ExecuteNonQuery()
    {
        if (_parameters != null && _parameters.Count > 0)
        {
            _command.Parameters.AddRange(_parameters.ToArray());
        }

        try
        {
            return _command.ExecuteNonQuery();
        }
        catch (MySqlException e)
        {
            Logger.Error(e, "Database error was logged.");
            return 0;
        }
        finally
        {
            _command.CommandText = string.Empty;
            _command.Parameters.Clear();

            if (_parameters != null && _parameters.Count > 0)
            {
                _parameters.Clear();
            }
        }
    }

    public int GetLastId()
    {
        try
        {
            return (int) _command.LastInsertedId;
        }
        catch (MySqlException e)
        {
            Logger.Error(e, "Database error was logged.");
            return 0;
        }
        finally
        {
            _command.CommandText = string.Empty;
            _command.Parameters.Clear();

            if (_parameters != null && _parameters.Count > 0)
            {
                _parameters.Clear();
            }
        }
    }

    public int ExecuteSingleInt()
    {
        try
        {
            if (_parameters != null && _parameters.Count > 0)
            {
                _command.Parameters.AddRange(_parameters.ToArray());
            }

            return int.Parse(_command.ExecuteScalar().ToString());
        }
        catch (MySqlException e)
        {
            Logger.Error(e, "Database error was logged.");
            return 0;
        }
        finally
        {
            _command.CommandText = string.Empty;
            _command.Parameters.Clear();

            if (_parameters != null && _parameters.Count > 0)
            {
                _parameters.Clear();
            }
        }
    }

    public string ExecuteSingleString()
    {
        try
        {
            if (_parameters != null && _parameters.Count > 0)
            {
                _command.Parameters.AddRange(_parameters.ToArray());
            }

            return _command.ExecuteScalar().ToString();
        }
        catch (MySqlException e)
        {
            Logger.Error(e, "Database error was logged.");
            return "";
        }
        finally
        {
            _command.CommandText = string.Empty;
            _command.Parameters.Clear();

            if (_parameters != null && _parameters.Count > 0)
            {
                _parameters.Clear();
            }
        }
    }

    public bool TryExecuteSingleInt(out int value)
    {
        try
        {
            if (_parameters != null && _parameters.Count > 0)
            {
                _command.Parameters.AddRange(_parameters.ToArray());
            }

            var scalar = _command.ExecuteScalar();

            if (scalar == null)
            {
                value = 0;
                return false;
            }

            value = int.Parse(scalar.ToString());
            return true;
        }
        catch (MySqlException e)
        {
            Logger.Error(e, "Database error was logged.");
            value = 0;
            return false;
        }
        finally
        {
            _command.CommandText = string.Empty;
            _command.Parameters.Clear();

            if (_parameters != null && _parameters.Count > 0)
            {
                _parameters.Clear();
            }
        }
    }

    public MySqlDataReader ExecuteReader()
    {
        if (_parameters != null && _parameters.Count > 0)
        {
            _command.Parameters.AddRange(_parameters.ToArray());
        }

        try
        {
            return _command.ExecuteReader();
        }
        catch (MySqlException e)
        {
            Logger.Error(e, "Database error was logged.");
            return null;
        }
        finally
        {
            _command.CommandText = string.Empty;
            _command.Parameters.Clear();

            if (_parameters != null && _parameters.Count > 0)
            {
                _parameters.Clear();
            }
        }
    }

    public DataSet ExecuteDataSet()
    {
        if (_parameters != null && _parameters.Count > 0)
        {
            _command.Parameters.AddRange(_parameters.ToArray());
        }

        var dataSet = new DataSet();

        try
        {
            using (var adapter = new MySqlDataAdapter(_command))
            {
                adapter.Fill(dataSet);
            }

            return dataSet;
        }
        catch (MySqlException e)
        {
            Logger.Error(e, "Database error was logged.");
            return null;
        }
        finally
        {
            _command.CommandText = string.Empty;
            _command.Parameters.Clear();

            if (_parameters != null && _parameters.Count > 0)
            {
                _parameters.Clear();
            }
        }
    }

    public DataTable ExecuteTable()
    {
        var dataSet = ExecuteDataSet();
        return dataSet.Tables.Count > 0 ? dataSet.Tables[0] : null;
    }

    public DataRow ExecuteRow()
    {
        var dataTable = ExecuteTable();
        return dataTable.Rows.Count > 0 ? dataTable.Rows[0] : null;
    }

    private void Dispose(bool disposing)
    {
        if (!disposing)
        {
            return;
        }

        if (_connection.State == ConnectionState.Open)
        {
            _connection.Close();
            _connection = null;
        }

        if (_parameters != null)
        {
            _parameters.Clear();
            _parameters = null;
        }

        if (_command == null)
        {
            return;
        }

        _command.Dispose();
        _command = null;
    }
}

Answer 1

Lot of repeated code.

As stated in the other answer clear parameters is not always what they want.

They have to deal with the commands anyway. Why not let them deal directly with MySqlCommand? If you want to abstract then why not use an ORM like Entity Framework?

This is SQLcommand but look at the type of stuff you might do that is not supported by your wrapper. This does not even include any async methods or transaction.

    SqlConnection conn = new SqlConnection("conString");
    List<string> victims = new List<string>();
    try
    {
        conn.Open();
        using (SqlCommand cmd = conn.CreateCommand())
        {
            cmd.CommandText = "select count(*) from tableX where col1 = @col1";
            SqlParameter paramCol1 = new SqlParameter("@col1", SqlDbType.VarChar, 80);
            cmd.Parameters.Add(paramCol1);
            paramCol1.Value = "jason";
            int iJason = (int)cmd.ExecuteScalar();
            paramCol1.Value = "keller";
            int iKeller = (int)cmd.ExecuteScalar();
            cmd.CommandText = "delete tableX where col1 = @col1";
            if(iJason > iKeller)
            {                   
                int rowsRet = cmd.ExecuteNonQuery();
                paramCol1.Value = "jason";
            }
            else
            {
                paramCol1.Value = "jason";
                int rowsRet = cmd.ExecuteNonQuery();
                paramCol1.Value = "keller";
            }
            cmd.CommandText = "select victim from tableX where col1 = @col1";
            using (SqlDataReader rdr = cmd.ExecuteReader())
            {
                victims.Add(rdr.GetString(0));
            }
            //no need to clear parameters as they will go away with using
        }        
    }
    catch (SqlException e)
    {
        //log execption
    }
    finally
    {
        conn.Close();
    }
    foreach(string victim in victims)
    {
        //process victim
    }
    victims.Clear();

Answer 2

I'll just start by saying that I'm not terribly fond of these sorts of 'state collecting' classes (at least not anymore), which accumulate a whole load of parameters, do something with them and then reset. If I'm executing a query, then I have to do this sort of thing:

controller.SetQuery("Some SQL")
controller.AddParameters(param1);
controller.AddParameters(param2);
var reader = controller.ExecuteReader()
DoStuffWithRead(reader);
return GetLastId();
// what state am I leaving the controller in?

It just feels fragile. There is a good reason why DbCommand/MySqlCommand exist: they encapsulate all the state necessary to perform some operation. If you don't want that state after you've used it, then you just throw it away, but it doesn't mystically get cleared.

There is a good argument to requiring that you restate all of your parameters with each query, but your current implementation - for example - leaves you free to 'forget' to set the MySqlParameters. Personally I'd much rather see the query, MySqlParameters, and everything else passed to a single method call: then if you don't want to provide any parameters you can see at the Exectute call-site that you are not providing any parameters. It also means you know exactly where the parameters you are passing are coming from. It sounds like a nice idea to be able to pass this wrapper around and collect state from different places, but it will be a nightmare to debug when something goes wrong and you have no idea where the different parameters are coming from: much better (I think) to collect them all somehow (this could be as simple as passing a list around/accumulating returned values) and then explicitly pass these to the command when you want to execute it.

Making up data

Again, as I said in your other question, you are making up data! Unless you have very strict design criteria which mandate that ExecuteSingleInt() should return 0 if the operation fails, then you should be throwing an exception. The operation failed for a reason, and the programmer/maintainer wants to know about it! If you ever find yourself thinking "what should I do if <edge case> or if <something goes wrong> which isn't formerlly decided", the default responce should be document this in the most helpful way possible by throwing an exception if it ever happens, and start a design discussion about what to do when this edge case/error is encountered.

I'll just forewarn that if you let your DB exceptions escape/throw a wrapping exception (which I'd prefer) then you will lose all the valuable state (i.e. what you were trying to do) because the finally caluses will clear it before you can look at it.

Repeated Code

There is a lot of repeated code that operates on _command and _parameters that is repeated. Pull these out into their own methods:

private void FillCommandParameters()
{
    if (_parameters != null && _parameters.Count > 0)
    {
        _command.Parameters.AddRange(_parameters.ToArray());
    }
}

private void ClearCommandAndParameters()
{
    _command.CommandText = string.Empty;
    _command.Parameters.Clear();

    if (_parameters != null && _parameters.Count > 0)
    {
        _parameters.Clear();
    }
}

I think you can define ExecuteSingleInt in terms of TryExecuteSingleInt. On that point, I wouldn't be afraid of returning an int? in the 'naive' method if it more closely matches the DB representation.

Use of ExecuteScalar()

In ExecuteSingleInt(), you retrieve an integer by parsing the scalar after a ToString(). This is not good, because it means this code won't crash if you try to use it for reading ints which are stored as a string. A straight cast is better, because it won't "just happen to work" if you are reading string (or doubles, or anything else) that 'look' like integers.

I'd say the same applied to ExectuteSingleString: it isn't the job of the DB wrapper to coerce data into the right shape for output: I'd much rather see an "ExecuteScalarInvarient" which returns some invarient type which the code can call ToString() on if it wants to, rather than risking 'ToStringing' the data by accident.

DatabaseController.NewConnection

As Nkosi points out: this method doesn't exist! How can this possibly be the correct usage?!

DatabaseConnection._parameters

There are a lot of null checks around this parameter. I would be strongly inclinced to make it readonly and assign an empty list from the start, and remove all the null checks: they don't add anything (since it has the same 'meaning' as an empty list of parameters), just clutter the code.

DatabaseConnection.GetLastId()

It isn't clear to me why this method clears _command and _parameters: is this necessary?

ExecuteRow()

It's been a while since I did any proper DB work, but this method looks like it would be better off reading from a DataReader (rather than assembling a whole table).

If the idea is that this is only used by queries which return exactly one row, then it ought to be indicating if another other than 1 row is returned (e.g. by throwing, or possibly returning null (as you do when there is no row), or something) rather than glossing over this fact. Whatever it is meant to do should be documented, because with inline doc (///) so that consumers can work out how to use it without consulting the code.

DatabaseConnection.Dispose(bool disposing)

I see no reason not to invert if (_command == null) and only operate on it if it is not null just you have with the other members: this would be consistent, and remove any maintainability concerns associated with early returns.