3
\$\begingroup\$

I am writing this question to get some advice on improving this Database Management library. I'll explain a little about it:

Database Manager - DatabaseManager is the holder, it generates new connections when the NewConnection is called, it returns a new DatabaseConnection with the saved connection string.

Database Connection - DatabaseConnection is a connection containing a new connection that's created on each call from DatabaseManager.

Usage:

using (var databaseConnection = Serber.GetDatabase().NewDatabaseConnection)
{
    databaseConnection.SetQuery("SELECT * FROM `table` WHERE `enabled` = '1' ORDER BY `name` DESC;");
    databaseConnection.Open();

    using (MySqlDataReader Reader = databaseConnection.ExecuteReader())
    {
        while (Reader.Read())
        {
            try
            {
                // do some work
            }
            catch (DatabaseException ex)
            {
                log.Error("Unable to load item for ID [" + Reader.GetInt32("id") + "]", ex);
            }
        }
    }
}

DatabaseManager:

internal sealed class DatabaseManager
{
    private readonly string _connectionString;

    public DatabaseManager()
    {
        var connectionString = new MySqlConnectionStringBuilder
        {
            ConnectionLifeTime = (60 * 5),
            ConnectionTimeout = 30,
            Database = Hariak.HariakServer.Config.GetConfigValueByKey("database.mysql.database"),
            DefaultCommandTimeout = 120,
            Logging = false,
            MaximumPoolSize = uint.Parse(Hariak.HariakServer.Config.GetConfigValueByKey("database.mysql.pool_maxsize")),
            MinimumPoolSize = uint.Parse(Hariak.HariakServer.Config.GetConfigValueByKey("database.mysql.pool_minsize")),
            Password = Hariak.HariakServer.Config.GetConfigValueByKey("database.mysql.password"),
            Pooling = Hariak.HariakServer.Config.GetConfigValueByKey("database.mysql.pooling") == "1",
            Port = uint.Parse(Hariak.HariakServer.Config.GetConfigValueByKey("database.mysql.port")),
            Server = Hariak.HariakServer.Config.GetConfigValueByKey("database.mysql.hostname"),
            UseCompression = false,
            UserID = Hariak.HariakServer.Config.GetConfigValueByKey("database.mysql.username"),
        };

        _connectionString = connectionString.ToString();
    }

    public bool ConnectionWorks()
    {
        try
        {
            using (var databaseConnection = NewDatabaseConnection)
            {
                databaseConnection.OpenConnection();
            }

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

    public DatabaseConnection NewDatabaseConnection => new DatabaseConnection(_connectionString);
}

DatabaseConnection:

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

    private MySqlConnection _connection;
    private List<MySqlParameter> _parameters;
    private MySqlCommand _command;

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

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

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

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

    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)
        {
            _command.Dispose();
            _command = null;
        }
    }
}
\$\endgroup\$
4
\$\begingroup\$

Your code looks great to me at first glance. I have some tips for you that could increase the performance of your database manager:

  • Go async

    • I wouldn't say that the official ADO.NET MySQL connector is bad. It does pretty well what it has to, but let's admit it, async programming is not that rare today. You should take a look at one particular connector repo on Github, which is a fresh, clean and fully async ADO.NET MySQL connector that also supports .NET Core.
  • Move the parameter and the command holder outside your DatabaseConnection class. You might not need a holder for parameters during every query. If you move those back to your connector (DatabaseManager) and just grab one when needed, you can spare some allocated space.

    Like this:

    DatabaseManager:

    public MySqlConnection CreateConnectionObject() => Activator.CreateInstance(typeof(MySqlConnection)) as MySqlConnection;
    public MySqlCommand CreateCommandObject() => Activator.CreateInstance(typeof(MySqlCommand)) as MySqlCommand;
    public MySqlParameter CreateParameterObject() => Activator.CreateInstance(typeof(MySqlParameter)) as MySqlParameter;
    
  • Refactor

    • Only make methods and variables public, if they really must be exposed to the environment. Any internal method that shouldn't be called from the outside has to be private or protected.
    • It's better, if you have only one DatabaseManager instance (which is the connector) and all the other are DatabaseConnection instances that link back to the DatabaseManager (which are the individual database interfaces and should only one per database exist). First you should create your DatabaseManager connector, then you create one DatabaseConnection for each database and if you would like to commit a query on that particular database, you can instruct the appropriate DatabaseConnection to do it.
    • Break your code into sections depending on the MySQL commands. Instead of one global query handler, you would have different methods for different actions (Select, Insert, Delete etc.). It results in a much cleaner code. Also it's great, if you have one MySqlCommand builder method, that decides whether parameters are needed or not.

    See example below for what I mean:

    private MySqlCommand CreateSqlCommand(MySqlConnection Connection, string Sql, params object[] Args)
    {
        MySqlCommand SqlCommand = connector.CreateCommandObject(); // connector = DatabaseManager instance
    
        SqlCommand.Connection = Connection;
        SqlCommand.CommandText = Sql;
        SqlCommand.CommandTimeout = 300;
    
        if (Args.Length > 0)
        {
            MySqlParameter[] Params = new MySqlParameter[Args.Length];
            for (var i = 0; i < Args.Length; i++)
            {
                MySqlParameter Param = connector.CreateParameterObject(); // connector = DatabaseManager instance
                Param.ParameterName = "";
                Param.Value = Args[i];
                Params[i] = Param;
            }
    
            SqlCommand.Parameters.AddRange(Params);
        }
        return SqlCommand;
    }
    
    public async Task<MySqlDataReader> SelectAsync(string Sql, params object[] Args)
    {
        try
        {
            using (MySqlCommand Command = CreateSqlCommand(CreateConnection(), Sql, Args))
                return await Command.ExecuteReaderAsync();
        }
        catch (Exception ex)
        {
            Console.WriteLine(ex.ToString());
            return null;
        }
    }
    
  • Coding style

    • If an if, using, while or for statement is followed by a single action, then scoping ({ and }) is not needed.
    • Commenting is useful, when not overused. Personally, I dislike commenting everything, it makes my code messy and difficult to work with. If you name your variables after their purpose (as you did), comments are not that necessary, since the code speaks for itself.
| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.