6
\$\begingroup\$

I have coded a database manager in C# that connects to a MySQL database and does a few things, etc.. I just wanted someone to give me a few tips on how I can improve this, and maybe make it a bit faster.

DatabaseManager.cs

using MySql.Data.MySqlClient;    
namespace Kiwi.Application.Base.Core.Database
{
    sealed class DatabaseManager
    {
        private string host;
        private string username;
        private string password;
        private string database;
        private uint port;
        private uint maxConnections;

        private string _connectionString;

        private DatabaseConnection _dbCon;

        public void load()
        {
            MySqlConnectionStringBuilder cs = new MySqlConnectionStringBuilder
            {
                ConnectionLifeTime = (60 * 5),
                ConnectionTimeout = 30,
                Database = this.database,
                DefaultCommandTimeout = 120,
                Logging = false,
                MaximumPoolSize = this.maxConnections,
                MinimumPoolSize = 3,
                Password = this.password,
                Pooling = true,
                Port = this.port,
                Server = this.host,
                UseCompression = false,
                UserID = this.username,
            };

            this._connectionString = cs.ToString();
            this._dbCon = new DatabaseConnection(this._connectionString);
        }

        public DatabaseConnection generateNewConnection()
        {
            // NEW connection each time...
            return new DatabaseConnection(this._connectionString);
        }
    }
}

DatabaseConnection.cs

using log4net;
using MySql.Data.MySqlClient;
using System;
using System.Collections.Generic;
using System.Data;

namespace Kiwi.Application.Base.Core.Database
{
    sealed class DatabaseConnection
    {
        private MySqlConnection connection;
        private MySqlCommand command;
        private DateTime startTime;
        private List<MySqlParameter> mysqlParams;
        private MySqlTransaction transaction;
        private ILog myLogger;

        public DatabaseConnection(string connectionString)
        {
            this.connection = new MySqlConnection();
            this.connection.ConnectionString = connectionString;
            this.command = this.connection.CreateCommand();
            this.mysqlParams = new List<MySqlParameter>();
            this.myLogger = LogManager.GetLogger(typeof(DatabaseConnection));
        }

        public void openConnection()
        {
            if (this.connection.State == ConnectionState.Open)
                throw new InvalidOperationException("MySQL connection has already been opened.");
            else
                this.connection.Open(); //open the connection

            this.startTime = DateTime.Now;
        }

        public bool connectionOpen()
        {
            return this.connection.State == ConnectionState.Open;
        }

        public void registerParameter(string param, object value)
        {
            this.mysqlParams.Add(new MySqlParameter(param, value));
        }

        public MySqlDataReader executeReader()
        {
            if (this.mysqlParams != null && this.mysqlParams.Count > 0)
            {
                this.command.Parameters.AddRange(this.mysqlParams.ToArray());
            }

            try
            {
                return this.command.ExecuteReader();
            }
            catch (MySqlException e)
            {
                myLogger.Error(e);
                throw e;
            }
            finally
            {
                this.command.CommandText = string.Empty;
                this.command.Parameters.Clear();

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

        public string getString()
        {
            if (this.mysqlParams != null && this.mysqlParams.Count > 0)
            {
                this.command.Parameters.AddRange(this.mysqlParams.ToArray());
            }

            try
            {
                return Convert.ToString(this.command.ExecuteScalar().ToString());
            }
            catch (MySqlException e)
            {
                myLogger.Error(e);
                throw e;
            }
            finally
            {
                this.command.CommandText = string.Empty;
                this.command.Parameters.Clear();

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

        public void assignQuery(string Command)
        {
            this.command.CommandText = Command;
        }

        public DataSet executeDataSet()
        {
            if (this.mysqlParams != null && this.mysqlParams.Count > 0)
            {
                this.command.Parameters.AddRange(this.mysqlParams.ToArray());
            }

            DataSet Set = new DataSet();

            try
            {
                using (MySqlDataAdapter Adapter = new MySqlDataAdapter(this.command))
                {
                    Adapter.Fill(Set);
                }

                return Set;
            }
            catch (MySqlException e)
            {
                myLogger.Error(e);
                throw e;
            }
            finally
            {
                this.command.CommandText = string.Empty;
                this.command.Parameters.Clear();

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

        public DataTable executeTable()
        {
            DataSet DataSet = this.executeDataSet();
            return DataSet.Tables.Count > 0 ? DataSet.Tables[0] : null;
        }

        public DataRow executeRow()
        {
            DataTable DataTable = this.executeTable();
            return DataTable.Rows.Count > 0 ? DataTable.Rows[0] : null;
        }

        public int executeNonQuery()
        {
            try
            {
                if (this.mysqlParams != null && this.mysqlParams.Count > 0)
                    this.command.Parameters.AddRange(this.mysqlParams.ToArray());

                return this.command.ExecuteNonQuery();
            }
            catch (MySqlException exception)
            {
                myLogger.Error(exception);
                throw exception;
            }
            finally
            {
                this.command.CommandText = string.Empty;
                this.command.Parameters.Clear();

                if (this.mysqlParams != null && this.mysqlParams.Count > 0)
                    this.mysqlParams.Clear();
            }
        }

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

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

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

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

            int Finish = (DateTime.Now - this.startTime).Milliseconds;

            if (Kiwi.getServer().getServerInfo().isDebugging)
                myLogger.Debug("Query completed in " + Finish + "ms");

            else if (Finish >= 5000)
                myLogger.Debug("Query took 5 seconds or longer");
        }
    }
}
\$\endgroup\$
1
  • \$\begingroup\$ Welcome to Code Review! I hope you get some great answers! \$\endgroup\$
    – Phrancis
    Commented Nov 5, 2015 at 0:09

4 Answers 4

7
\$\begingroup\$

Naming

I have coded a database manager in C# that connects to a MySQL database and does a few things, etc..

A red flag goes off for me every time I get to hear "this class does this, and that etc." - this is at odds with Single Responsibility Principle, stating that class should only have one. Do one thing, do it well.

The infamous ...Manager suffix is a code smell, too. How vague is that? All too often it actually means "I had no idea how to call it".

In this case it's more of a naming issue, really - your DatabaseManager doesn't really manage anything, it just builds database connections. Not connection strings, but connections themselves.

Correspondingly, I would rename it to ConnectionBuilder or perhaps - since it's not parameterized - ConnectionProvider. It describes what it does way more accurately.

I would also try to be consistent - if you have:

private MySqlConnection connection;
private MySqlCommand command;

(and not mySqlConnection, mySqlCommand), then why:

private List<MySqlParameter> mysqlParams;

instead of just params?

Another thing:

public bool connectionOpen()

Apart from using upper case for method names (as pointed out by @paritosh), I think it's a reasonable convention to use Is prefix for methods returning a boolean representing some state that may or may not be. Case in point: https://msdn.microsoft.com/en-us/library/system.io.ports.serialport.isopen%28v=vs.110%29.aspx

Note this is a property, meaning you could indeed replace connectionOpen with a read-only property. Not that there's anything bad with leaving it like it is - properties are just more idiomatic in C#.

Using a property kind of puts more emphasis on the fact that we're only checking on some state, and there's no side effects to that. (You can still implement a property in such a way that it causes side effects, it's just blatantly against the semantics of it).

Last but not least, variables should be named with lower case in C#:

DataSet Set = new DataSet();

Same with parameters.

public void assignQuery(string Command)

Code style

Magic numbers

In "DatabaseManager", I would convert "magic numbers" (connection life time being 5 minutes, command timeout set to 120 etc.) into constants.

Fluff / noise

if (this.connection.State == ConnectionState.Open)
    throw new InvalidOperationException("MySQL connection has already been opened.");
else

else is redundant here - once you throw an exception, further code won't execute anyway.

this.connection.Open(); //open the connection

Do you think this comment adds any value?

Copy-pasta

this.command.CommandText = string.Empty;
this.command.Parameters.Clear();
if (this.mysqlParams != null && this.mysqlParams.Count > 0) { this.mysqlParams.Clear(); }

This bit repeats several times. I would encapsulate it as a private method (ResetCommand?) to avoid code repetition.

And as a side note, I don't really see any purpose in this.mysqlParams.Count > 0, it's just clutter. Clear the collection anyway (even if it's empty), I wouldn't expect much of a performance boost from this check :)

Benchmarking

This is weird to me. You're setting startTime in openConnection:

this.startTime = DateTime.Now;

Then the end time in Dispose:

int Finish = (DateTime.Now - this.startTime).Milliseconds;

And you log it as:

"Query completed in " + Finish + "ms"

But why would you assume that whatever time passed between opening the connection and disposing of the entire object, was consumed by query execution? Especially since you do this at the very end, after disposing of the connection, params object, transaction and command objects :) and the object is apparently supposed to be reusable (assignQuery).

Disposing

public void Dispose()
{
    if (this.connection.State == ConnectionState.Open)
    {
        this.connection.Close();

I'm not sure about this condition. There are other states where connection state isn't Open, and yet it should still be closed - see https://msdn.microsoft.com/en-us/library/system.data.connectionstate%28v=vs.110%29.aspx

I would err on the safe side and try to close it anyway. You could test it, but I don't think anything bad's going to happen if it's closed already.

Or you could make it if (this.connection.State != ConnectionState.Closed) instead. Since there are other possibilities (Broken, Connecting, Executing, Fetching), this is not the same thing.

\$\endgroup\$
1
  • 3
    \$\begingroup\$ "The infamous ...Manager suffix is a code smell, too." I Shall Call It Something Manager should be required reading for every developer. I see this way too often. "There are two hard things in computer science: cache invalidation, naming things, and off-by-one errors." - Martin Fowler \$\endgroup\$ Commented Nov 5, 2015 at 14:19
6
\$\begingroup\$
  1. Proper naming conventions:

    Please use PascalCase for methods. As such, your methods may look like this:

    public int ExecuteNonQuery()  {    }
    
  2. Too many public methods.

    Users of this class will have to remember a lot of things for executing any data operation, which is not good. Use parameters to avoid this kind of situation.

    public int ExecuteNonQuery(CommandType commandType, string sql,IEnumerable<MySqlParameter> parameters=null)
    {
    }
    

    If you use them in AssignQuery, then registerParameter will become irrelevant.

  3. Name of methods should not be confusing.

    GetString is a very confusing name; it is just doing the ExecuteScalarmethod. In addition to it, either make this method return an object or provide support for generic types as follows:

    public T ExecuteScalar<T>(yourParameter )
    {
    
    }
    
  4. Implement the IDisposable interface rather than a method. Two advantages to it:

    • The user will know that dispose is required.
    • Can be used in conjunction with the using keyword.
\$\endgroup\$
4
\$\begingroup\$

The only thing I can add is that methods returning a boolean value are generally written as properties in C#.

public bool connectionOpen()
{
    return this.connection.State == ConnectionState.Open;
}

Becomes:

public bool IsConnected
{
    get { return this.connection.State == ConnectionState.Open; }
}

Using the "IsFoo" naming convention makes your code more expressive when in use:

var db = new DatabaseManager(...);

if (db.IsConnected)
{
    // ...
}

This reads fluently like natural language: "If db is connected".

\$\endgroup\$
4
\$\begingroup\$

Although most is said about your code I will focus mainly on DatabaseConnection.Dispose().

You have a potential bug/unexpected behaviour in this method. This is because you don't save the state that Dispose() had been called.

So whats the problem with that, quite easy, a second call to Dispose() will throw an NullReferenceException which shouldn't happen but avoided.

This behaviour can be achieved by having a private bool disposed which will be checked before any managed ressource will be closed/disposed.

private bool disposed = false;
public void Dispose()
{
    if (disposed) { return; }
    disposed = true;

    // the remaining code
}

I agree with @paritosh's answer regarding IDisposable too.

\$\endgroup\$

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.