current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 7 down vote favorite
4

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?

share|improve this question
    
Welcome to CodeReview reptildarat. This is an interesting question, I hope you get a fine answer. – Legato Apr 22 '15 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 '15 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 '15 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 '15 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 '15 at 9:12
up vote 4 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 '15 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 '15 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 '15 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 '15 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.