10
\$\begingroup\$

I replace this common statement:

using (var connection = new MySqlConnection(connectionString))
{
     connection.Open();
     // Do work here; connection closed on following line.
}

With this connection manager class that I made:

public class ConnectionBase : IConnectionBase
{
    private static readonly string ConnString =
        ConfigurationManager.ConnectionStrings["ConnStr"].ConnectionString;

    private static readonly Lazy<MySqlConnection> ConnectionString =
        new Lazy<MySqlConnection>(() => new MySqlConnection(ConnString));

    private IDbConnection _db;

    public IDbConnection Db
    {
        get
        {
            if (_db != null) return _db;

            _db = ConnectionString.Value;
            if (_db.State == ConnectionState.Closed) _db.Open();
            return _db;
        }
    }

    public void Dispose()
    {
        if (_db != null)
            _db.Dispose();
    }
}

And wired up to my repository like this:

public abstract class BaseRepository
{
    protected readonly IDbConnection _repository;

    public BaseRepository(IConnectionBase connection)
    {
        _repository = connection.Db;
    }
} 

Then I use it on top of the dapper method (since it contains connection.close() at the end of every method that it used).

public class MyTableRepository : BaseRepository
{
    public MyTableRepository(IConnectionBase connection) : base(connection) {}

    public List<MyTable> GetAllMyTable()
    {
        return _repository.Query("SELECT * FROM MyTable").ToList();
    }

}

It's been working great thus far for quite some time in production.

Are there any side effects for this implementation since it doesn't explicitly state to dispose the IDbConnection object?

\$\endgroup\$
  • \$\begingroup\$ Welcome to CodeReview reptildarat. This is an interesting question, I hope you get a fine answer. \$\endgroup\$ – Legato Apr 22 '15 at 2:06
  • \$\begingroup\$ I'm looking at the dapper-dot-net source and it only closes the database connection if it was closed to begin with. Your code opens it every time, so there's no way dapper will close it and you'll be leaking an unmanaged resource. \$\endgroup\$ – Jesse C. Slicer Apr 22 '15 at 2:24
  • \$\begingroup\$ @JesseC.Slicer Yes you are right, it will open everytime. But, it just leave 1 connection open, since this more like singelton implementation, is that bad? \$\endgroup\$ – reptildarat Apr 22 '15 at 3:08
  • \$\begingroup\$ @reptildarat it's certainly not best practice for handling database connections as I am to understand it. \$\endgroup\$ – Jesse C. Slicer Apr 22 '15 at 4:18
  • \$\begingroup\$ What's your reason for the above code? Are you trying to reduce the clutter? Improve perf? There's a few things not quite right and it will be easier to know why you wrote the code :) \$\endgroup\$ – RobH Apr 22 '15 at 9:12
8
\$\begingroup\$

I can understand why you want to remove the clutter from your code - but sharing the instance of MySqlConnection isn't a good idea. As soon as multiple threads are trying to use the connection at the same time, things are going to go wrong.

The connections are already pooled for you (See here) so you creating and disposing IDbConnections is cheap and easy.

I think you should create a connection factory:

public interface IConnectionFactory
{
    IDbConnection GetOpenConnection();
}

public class ConnectionFactory : IConnectionFactory
{
    private static readonly string connectionString =
        ConfigurationManager.ConnectionStrings["ConnStr"].ConnectionString;

    public IDbConnection GetOpenConnection()
    {
        // Is there an overload to automatically open the connection?
        var connection = new MySqlConnection(connectionString);
        connection.Open();
        return connection;
    }
}

Then your base repository can do:

public abstract class BaseRepository
{
    protected readonly IConnectionFactory connectionFactory;

    public BaseRepository(IConnectionFactory connectionFactory)
    {
        this.connectionFactory = connectionFactory;
    }
} 

Then all of your actual repositories can do:

public class MyTableRepository : BaseRepository
{
    public MyTableRepository(IConnectionFactory connectionFactory) : base(connectionFactory) {}

    public List<MyTable> GetAllMyTable()
    {
        using (var connection = connectionFactory.GetOpenConnection())
        {
            return connection.Query("SELECT * FROM MyTable").ToList();
        }
    }
}

I realise it doesn't save you much over the original code other than centralising the connection string...

A more advanced idea would be add an Execute method to your base repo but I don't think it would add much...

protected T Execute<T>(Func<IDbConnection, T> query)
{
    using (var connection = connectionFactory.GetOpenConnection())
    {
        return query(connection);
    }
}
\$\endgroup\$
  • \$\begingroup\$ How about the idea of Lazy<T> implementation? is it will solve the multiple threads problem since it will create thread-safe object? \$\endgroup\$ – reptildarat Apr 22 '15 at 11:01
  • 1
    \$\begingroup\$ Lazy<T> will create you one instance in a thread safe way but it's still not safe to share that one instance between threads. \$\endgroup\$ – RobH Apr 22 '15 at 11:07
  • \$\begingroup\$ if I wired up IConnectionBase to DI implementation and sort of auto-dispose it by the end of request or some other events. is it more okay that way? \$\endgroup\$ – reptildarat Apr 22 '15 at 11:12
  • 1
    \$\begingroup\$ It will be okay to have IConnectionBase a "per request" object which is disposed at the end of each request as long as you removed the statics . Effectively you're not sharing the connection between threads (well, not strictly true is ASP.Net). I'd rethink the names though. IConnectionBase doesn't sound right. ConnectionWrapper might be better. \$\endgroup\$ – RobH Apr 22 '15 at 11:23
2
\$\begingroup\$

You could do the following approach:

public class DataContext : ISampleRepository
{
     private IDbConnection dbConnection;
     public DataContext(string dbConnection)
     {
         if(dbConnection != ConnectionState.Open())
              this.dbConnection = new SqlConnection(dbConnection).Open();
     }

     // Interface Implementation:
}

public interface DataContextFactory : ISampleFactory
{
     private IConfigurationRoot configuration;
     public DataContextFactory(IConfigurationRoot configuration) => this.configuration = configuration;
     ISampleRepository Create() => new DataContext(configuration.GetConnectionString());
}

public interface ISampleFactory
{
     ISampleRepository Create();
}

public interface ISampleRepository
{
     // Method Signature.
}

The reason this approach could be ideal, the connection information will be linked to each concrete implementation. But it doesn't obfuscate with additional connection repository or factory. Each repository, will have the proper knowledge of the connection as it is instantiated.

\$\endgroup\$
  • 1
    \$\begingroup\$ @t3chb0t Done. Added an explanation. \$\endgroup\$ – Greg Feb 17 '17 at 17:06
  • \$\begingroup\$ Now it deserves a +1 not a -1 ;-] \$\endgroup\$ – t3chb0t Feb 17 '17 at 17:08

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.