DatabaseManager

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

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

StackExchange Code Review Q#109818, answer score: 7