3
\$\begingroup\$

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;
        }
    }
}
\$\endgroup\$
5
  • 1
    \$\begingroup\$ Looks like it encourages leaving a connection open which is not good for scale. \$\endgroup\$
    – paparazzo
    Commented Oct 13, 2016 at 10:03
  • \$\begingroup\$ When I look at this quesiton and your other ones it's like the code was written by two different persons ;-] \$\endgroup\$
    – t3chb0t
    Commented Oct 13, 2016 at 10:20
  • \$\begingroup\$ Theres just a good amount of time between when both codes were created. \$\endgroup\$
    – Liam Hardy
    Commented Oct 13, 2016 at 12:34
  • 1
    \$\begingroup\$ Could you describe how you plan to use the code? \$\endgroup\$
    – paparazzo
    Commented Oct 13, 2016 at 14:28
  • \$\begingroup\$ To execute MySQL querys and fetch data, thats pretty much it. \$\endgroup\$
    – Liam Hardy
    Commented Oct 13, 2016 at 16:06

2 Answers 2

3
\$\begingroup\$

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).

\$\endgroup\$
2
  • \$\begingroup\$ So should I just add private fields inside the DatabaseManager class instead of holding them in DatabaseSettings class? or what do you suggest? \$\endgroup\$
    – Liam Hardy
    Commented Oct 13, 2016 at 16:09
  • \$\begingroup\$ Because the instance variable databaseSettings is not used, I would suggest to remove it. \$\endgroup\$
    – JanDotNet
    Commented Oct 13, 2016 at 16:32
1
\$\begingroup\$

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.

\$\endgroup\$
2
  • \$\begingroup\$ +1, though I'm not sure I agree with the idea that it's preferable to have validation rules rather than encoding valid ranges with types, where validation comes for free and sometimes at compile time. Unfortunately, C# can sometimes make unsigned numeric types difficult to work with. That said, I would still advocate at least making the port ushort, since port numbers are literally defined as 0-65535. \$\endgroup\$
    – Dan Lyons
    Commented Oct 13, 2016 at 18:13
  • \$\begingroup\$ @DanLyons meh, IMO it's so much simpler to just provide a connection string and be done with it. Store the connection string in the app settings, retrieve it from configuration manager. \$\endgroup\$ Commented Oct 13, 2016 at 18:21

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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