current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 6 down vote favorite
3

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 dapper method (since it's contain 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.

I'm just wondering are there any side effects for this implementation since it doesn't explicitly state to dispose the IDbConnection object?

Update :

As mentioned below, it's not a good idea to share the same instance (singelton) for the connection.

Another variation that we can do is to use delegate like Action or Func, like so:

public void WithConnection(Action<IDbConnection> actions)
{
    using (var db = new MySqlConnection(ConnString))
    {
        db.Open();
        actions.Invoke(db);
    }
}

public void WithTransaction(Action<IDbConnection> actions)
{
    using (var db = new MySqlConnection(ConnString))
    {
        db.Open();
        using (var context = db.BeginTransaction())
        {
            actions.Invoke(db);
            context.Commit();
        }
    }
}

So you can use it like this:

_repository.WithConnection(c =>
{
    c.Query<MyTable>("SELECT * FROM MyTable").ToList();
});

Sure it remove the using statement, but it's still doesn't remove the need of the indentation of the code.

So, in order to correct that, we need a Dependency Injection implementation that will remove IConnectionBase object automatically at the end of the scope.

share|improve this question
    
Welcome to CodeReview reptildarat. This is an interesting question, I hope you get a fine answer. –  Legato Apr 22 at 2:06
    
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. –  Jesse C. Slicer Apr 22 at 2:24
    
@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? –  reptildarat Apr 22 at 3:08
    
@reptildarat it's certainly not best practice for handling database connections as I am to understand it. –  Jesse C. Slicer Apr 22 at 4:18
    
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 :) –  RobH Apr 22 at 9:12

1 Answer 1

up vote 3 down vote accepted

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);
    }
}
share|improve this answer
    
How about the idea of Lazy<T> implementation? is it will solve the multiple threads problem since it will create thread-safe object? –  reptildarat Apr 22 at 11:01
1  
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. –  RobH Apr 22 at 11:07
    
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? –  reptildarat Apr 22 at 11:12
1  
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. –  RobH Apr 22 at 11:23

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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