MySQL Database Library

Question

so I created a database manager and wanted to know if I could improove it in any way? Thanks.

DatabaseManager.cs:

using MySql.Data.MySqlClient;
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace Sahara.Core.Database
{
    class DatabaseManager
    {
        private readonly DatabaseSettings databaseSettings;
        private readonly string connectionString;

        public DatabaseManager(DatabaseSettings databaseSettings)
        {
            this.databaseSettings = databaseSettings;

            MySqlConnectionStringBuilder mysqlConnectionString = new MySqlConnectionStringBuilder
            {
                ConnectionLifeTime = (60 * 5),
                ConnectionTimeout = 30,
                Database = databaseSettings.DatabaseName,
                DefaultCommandTimeout = 120,
                Logging = false,
                MaximumPoolSize = databaseSettings.MaximumConnections,
                MinimumPoolSize = 3,
                Password = databaseSettings.DatabasePassword,
                Pooling = true,
                Port = 3306,
                Server = databaseSettings.DatabaseHost,
                UseCompression = false,
                UserID = databaseSettings.DatabaseUsername
            };

            this.connectionString = mysqlConnectionString.ToString();
        }

        public bool ValidConnection()
        {
            try
            {
                using (var databaseConnection = GetDatabaseConnection())
                {
                    databaseConnection.OpenConnection();
                    databaseConnection.SetQuery("SELECT 1+1;");
                    databaseConnection.RunQuery();
                }
            }
            catch (MySqlException)
            {
                return false;
            }

            return true;
        }

        public DatabaseConnection GetDatabaseConnection()
        {
            return new DatabaseConnection(connectionString);
        }
    }
}

DatabaseConnection.cs:

using MySql.Data.MySqlClient;
using Sahara.Core.Logging;
using System;
using System.Collections.Generic;
using System.Data;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace Sahara.Core.Database
{
    class DatabaseConnection : IDisposable
    {
        private MySqlConnection mysqlConnection;
        private MySqlCommand mysqlCommand;
        private MySqlTransaction mysqlTransaction;
        private List<MySqlParameter> mysqlParameters;
        private DateTime startTime;

        public DatabaseConnection(string ConnectionStr)
        {
            this.mysqlConnection = new MySqlConnection(ConnectionStr);
            this.mysqlCommand = this.mysqlConnection.CreateCommand();
        }

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

            this.mysqlConnection.Open();
            this.startTime = DateTime.Now;
        }

        public bool IsConnectionOpen()
        {
            return this.mysqlConnection.State == ConnectionState.Open;
        }

        public void AddParameter(string param, object value)
        {
            if (this.mysqlParameters == null) { this.mysqlParameters = new List<MySqlParameter>(); }
                this.mysqlParameters.Add(new MySqlParameter(param, value));
        }

        public void SetQuery(string Query)
        {
            this.mysqlCommand.Parameters.Clear();
            this.mysqlCommand.CommandText = Query;
        }

        public void RunQuery(string query)
        {
            SetQuery(query);
            RunQuery();
        }

        public void RunQuery()
        {
            try
            {
                mysqlCommand.ExecuteNonQuery();
            }
            catch (Exception exception)
            {
                Sahara.GetServer().GetLogManager().Log("MySQL Error: " + exception.Message, LogType.Error);
                throw exception;
            }
        }

        public DataTable getTable()
        {
            var dataTable = new DataTable();

            try
            {
                using (MySqlDataAdapter adapter = new MySqlDataAdapter(mysqlCommand))
                {
                    adapter.Fill(dataTable);
                }
            }
            catch (Exception exception)
            {
                Sahara.GetServer().GetLogManager().Log("MySQL Error: " + exception.Message, LogType.Error);
            }

            return dataTable;
        }

        public int getInteger()
        {
            int result = 0;
            try
            {
                object obj2 = mysqlCommand.ExecuteScalar();
                if (obj2 != null)
                {
                    int.TryParse(obj2.ToString(), out result);
                }
            }
            catch (Exception exception)
            {
                Sahara.GetServer().GetLogManager().Log("MySQL Error: " + exception.Message, LogType.Error);
            }

            return result;
        }

        public string getString()
        {
            string str = string.Empty;

            try
            {
                object obj2 = mysqlCommand.ExecuteScalar();
                if (obj2 != null)
                {
                    str = obj2.ToString();
                }
            }
            catch (Exception exception)
            {
                Sahara.GetServer().GetLogManager().Log("MySQL Error: " + exception.Message, LogType.Error);
            }

            return str;
        }

        public DataRow getRow()
        {
            DataRow row = null;
            try
            {
                DataSet dataSet = new DataSet();
                using (MySqlDataAdapter adapter = new MySqlDataAdapter(mysqlCommand))
                {
                    adapter.Fill(dataSet);
                }
                if ((dataSet.Tables.Count > 0) && (dataSet.Tables[0].Rows.Count == 1))
                {
                    row = dataSet.Tables[0].Rows[0];
                }
            }
            catch (Exception exception)
            {
                Sahara.GetServer().GetLogManager().Log("MySQL Error: " + exception.Message, LogType.Error);
            }

            return row;
        }

        public void Dispose()
        {
            if (this.mysqlConnection.State == ConnectionState.Open)
            {
                this.mysqlConnection.Close();
                this.mysqlConnection = null;
            }

            if (this.mysqlParameters != null)
            {
                this.mysqlParameters.Clear();
                this.mysqlParameters = null;
            }

            if (this.mysqlTransaction != null)
            {
                this.mysqlTransaction.Dispose();
                this.mysqlTransaction = null;
            }

            if (this.mysqlCommand != null)
            {
                this.mysqlCommand.Dispose();
                this.mysqlCommand = null;
            }

            int Finish = (DateTime.Now - startTime).Milliseconds;
            Sahara.GetServer().GetLogManager().Log("Query completed in " + Finish + "ms", LogType.Information);
        }
    }
}

DatabaseSettings.cs:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace Sahara.Core.Database
{
    class DatabaseSettings
    {
        public string DatabaseHost
        {
            get;
            set;
        }

        public string DatabaseUsername
        {
            get;
            set;
        }

        public string DatabasePassword
        {
            get;
            set;
        }

        public string DatabaseName
        {
            get;
            set;
        }

        public uint DatabasePort
        {
            get;
            set;
        }

        public uint MaximumConnections
        {
            get;
            set;
        }
    }
}

Answer 1

DatabaseManager

• IMHO it is not a good idea to store the databaseSettings as instance variable because the object may be modified from outside. If so, the created connection string and the properties of databaseSettings are different.

DatabaseConnection

• Use the method IsConnectionOpen within OpenConnection (instead of duplicating the comparisation)

• It is possible to simplyfy Get* methods by return the result directly:

  public string getString()
  {
      try
      {
          object obj2 = mysqlCommand.ExecuteScalar();
          return obj2 != null ? obj2.ToString() : string.Empty;
      }
      catch (Exception exception)
      {
          Sahara.GetServer().GetLogManager().Log("MySQL Error: " + exception.Message, LogType.Error);
      }
  }
  

• I think it makes sense to dispose the connection, even if the state is not open. Check if the variable is not null instead to allow multiple calls of Dispose().

• The get* methods should renamed to Get*
• Finish should be renamed to finish

API

The API is a little bit confusing, because adding a query clears the list of parameters that was added before. That is something non-intuitive behavior that you have to know ;)

If you want to create something around the regular DB-API, I would prefer to create a query object that contains the SQL and all parameters and than execute that query object in one single operation so that the connection object doesn't has a "query state".

Abstraction

It is possible to abstract all that stuff so that the DatabaseManager and the DatabaseConection may work with any database management systems (not just MySQL).

If you want to support all DBMSs, just use the abstract types (DbConnection instead of MySqlConnection, DbCommand instead of MySqlCommand, DbParameter instead of MySqlParameter, and so on).

Answer 2

The DatabaseSettings class is really just a "holder" for a number of values; IMO it would be cleaner to remove the vertical whitespace and simply do this:

class DatabaseSettings
{
    public string DatabaseHost { get; set; }
    public string DatabaseUsername { get; set; }
    public string DatabasePassword { get; set; }
    public string DatabaseName { get; set; }
    public uint DatabasePort { get; set; }
    public uint MaximumConnections { get; set; }
}

That way you can see everything that matters at a glance. It's not clear why DatabasePort and MaximumConnections are uint values; normally you would use an int for this, and explicitly validate the values instead of relying on the data type.

Also, none of these values are meant to be modified after the instance is passed to the DatabaseManager constructor - consider exposing a constructor and making the properties read-only. If you're using C# 6 this is child's play:

class DatabaseSettings
{
    public DatabaseSettings(string host, string userName, string password, string name, int port, int maxConnections)
    {
        if (string.IsNullOrEmpty(host)) throw new ArgumentException(nameof(host));
        if (string.IsNullOrEmpty(userName)) throw new ArgumentException(nameof(userName));
        if (string.IsNullOrEmpty(password)) throw new ArgumentException(nameof(password));
        if (string.IsNullOrEmpty(name)) throw new ArgumentException(nameof(name));
        if (port <= 0) throw new ArgumentException(nameof(port));
        if (maxConnections <= 0) throw new ArgumentException(nameof(maxConnections));

        DatabaseHost = host;
        DatabaseUsername = userName;
        DatabasePassword = password;
        DatabaseName = name;
        DatabasePort = port;
        MaximumConnections = maxConnections;
    }

    public string DatabaseHost { get; }
    public string DatabaseUsername { get; }
    public string DatabasePassword { get; }
    public string DatabaseName { get; }
    public int DatabasePort { get; }
    public int MaximumConnections { get; }
}

With an immutable DatabaseSetting, JanDotNet's first point becomes moot - nobody can ever modify the values of a given instance of that class now, and it's impossible to create an instance with invalid values.

If you're not using C# 6, you can still implement immutable properties, only you'll need explicit backing fields, assigned in the constructor:

private readonly string databaseHost;
public string DatabaseHost { get { return this.databaseHost; } }

---

Style-wise, your casing isn't consistent: sometimes you have PascalCase parameters:

public DatabaseConnection(string ConnectionStr)

And sometimes camelCase methods:

public DataTable getTable()

The use of the this qualifier isn't consistent either:

try
{
    mysqlCommand.ExecuteNonQuery();
}

I like that your fields are all camelCase, and that you're qualifying them with this - but being inconsistent about qualifying with this. makes the reader stop and wonder if they missed a local variable or parameter; the eye grows accustomed to recognizing this.fooBar as a field and fooBar as a local variable or parameter.

On the flipside, you could also drop the this qualifier everywhere, and instead prefix fields with an underscore: the reader's eye will see _fooBar and immediately recognize a private field, or fooBar and know they're looking at a local variable.

Whether you do this.fooBar or _fooBar is personal preference, but consistency is important.

---

The API is confusing: it's not clear who's responsible for creating a connection, nor why GetDatabaseConnection needs to be public.

ValidConnection should try to Open a connection, and then close it and dispose it, period - sending a useless query over the connection won't change anything and introduces a new possible failure point that doesn't tell the user whether the connection is valid, which seems to be the point of the function... but then again, if the connection isn't valid, I would expect the API to just throw an exception when it tries to open it.

DatabaseConnection is a very generic-looking name for something that's MySQL-specific.

This looks funny:

public void RunQuery()
{
    try
    {
        mysqlCommand.ExecuteNonQuery();

Consider passing the query string as a parameter instead, and the term "Query" implies "results" - void RunQuery is just wrong, as emphasized by ExecuteNonQuery being called there.

---

I think you've started the design thinking about implementation details rather than what the public interface should look like.

I'd suggest you start the design over, this time with your public API in mind. Don't start with a class, start with an interface:

public interface IDbCommandWrapper
{
    // todo: design from the client code's perspective
    // think of what the *minimal* interface needs to look like - no fluff!
}

Then implement that interface with a class.

You haven't shown what your client code looks like - I'd start with some mock-up code asking myself "what do I want to do?" and then "what do I need to achieve that?", and end up with something like this:

// we want to pass a DatabaseSettings instance, and have that build the connection string for us
var settings = new DatabaseSettings(/*values*/)
using (IDbCommandWrapper db = new NotImplementedYet(settings))
{
    // we want to be able to get enumerable results by passing a SQL string and some parameter values:
    var results = db.GetResults(selectSql, 42);

    // we want to be able to get a single row result by passing a SQL string and some parameter values:
    var row = db.GetResultRow(anotherSelectSql, 42);

    // we want to be able to get a single value by passing a SQL string and some parameter values:
    var result = db.GetResult<int>(yetAnotherSelectSql, 42);

    // we want to be able to run a non-query by passing a SQL string and some parameter values:
    db.Execute(updateSql, 42, DateTime.Now);
}

That's about it, really; everything about adding parameters, opening, closing, validating or even retrieving a connection, should be irrelevant to the public API.

From the client side of things, the interface would look like this:

public interface IDbCommandWrapper : IDisposable
{
    DataTable GetResults(string sql, params object[] parameters);
    DataRow GetResultRow(string sql, params object[] parameters);
    TResult GetResult<TResult>(string sql, params object[] parameters);
    void Execute(string sql, params object[] parameters);
}

That's all the client code wants to be dealing with: everything else is an implementation detail, and shouldn't need to be exposed.

I'd argue against exposing DataTable and DataRow though; DataTable implements IDisposable, which means you'd be asking the client code to dispose an object it didn't create, which goes against rule #1 of disposables: if you created it, you own it, therefore you should dispose it. DataRow is just clumsy.

Instead, consider:

IEnumerable<TResult> GetResults<TResult>(string sql, Func<DataRow, TResult> resultFactory, params object[] parameters);
TResult GetResult<TResult>(string sql, Func<DataRow, TResult> resultFactory, params object[] parameters);

That way the client code could be dealing with strongly-typed results, and provide GetResults and GetResult with the knowledge of how to turn a DataRow into a TResult, whatever the type of TResult is:

public class Person
{
    string FirstName { get; set; }
    string LastName { get; set; }
    DateTime BirthDate { get; set; }
}

public Person FindSomeone(int id)
{
    var settings = new DatabaseSettings(/*values*/)
    using (IDbCommandWrapper db = new DbCommandWrapper(settings))
    {
        var sql = "SELECT FNAME, LNAME, DOB FROM dbo.PEOPLE WHERE ID=@id";
        var personResultFactory = new Func<DataRow, Person>((row =>
            return new Person
            {
                FirstName = row.Field<string>("FNAME"),
                LastName = row.Field<string>("LNAME"),
                BirthDate = row.Field<DateTime>("DOB")
            });

        return db.GetResult(sql, personResultFactory, 42);
    }
}

And here results would be some IEnumerable<Person> instead of a DataTable; when you design an API, you want to spare the client code as much of the implementation details as possible - the fact that there's a DataTable involved in the inside mechanics of things doesn't need to be relevant to the client code: even the fact that the data source is a MySQL instance should be irrelevant to the outside world.

Once you know what you want the consuming code to look like, then you start implementing your interface.