Is it bad practice to have a repository that manually opens and closes a db connection?

Question

Environment: ASP.NET MVC3 C#

Say I have some repository (semi-psuedo):

public interface IRepository
{
 create();read();update();delete();opendb();closedb();
}

public class CarRepository : IRepository
{
 private DbContext namedDbContext;

 public void opendb()
 {
  namedDbContext = new DbContext();
 }
 public void closedb()
 {
  namedDbContext.dispose();
 }
}

And then in a controller the repository is injected and used as follows to manually control the db connection lifetime:

public class SomeController : Controller
{
    private IRepository CarRepository;

    public void SomeController(IRepository _carRepository)
    {
        CarRepository = _carRepository;
    }

    public ActionResult SomeAction(int CarId)
    {
        CarRepository.opendb();
        var car = CarRepository.read(CarId);
        CarRepository.closedb();
    }
}

Is this considered bad practice because it is taking the control of the connection from the repository and placing it in the controller? I am worried about memory leaks from using dependency injection and want to ensure duplicate connections are not opened, nor long running and unused.

Answer 1

Yes. Sure. Most ADO.NET drivers uses connection pooling, so the actual connection process isn't that heavy. And you have TransactionScope which can take care of transaction over multiple connections, but it wont be as fast as one transaction over one connection.

I am worried about memory leaks from using dependency injection and want to ensure duplicate connections are not opened, nor long running and unused.

A IoC will guaranteed clean up the connection (a large user base have made sure of that). There is no guarantee that a programmer will do the cleanup in all places.

Answer 2

The REpository pattern provides an abstraction of the persistence layer. It shouldn't expose any of the persistence details such as db connection. What if the storage is an xml file, or a cloud storage?

So yes, it is bad practice. If you want more control, you might make the repository use the unit of work pattern, so that a higher level should decide when a transaction is commited, but that's it. No knowledge of the database should be exposed by the repository.

AS for memory leaks, make repository implmement IDIsposable (where you close any outstanding open conenctions)and just makes sure that the DI container manages a repository instance per request, it will call Dispose on it.

Answer 3

Part of a repository is abstracting away the details of persistence.

I see two problems with your proposal:

1. You are leaking the abstraction more than necessary by naming these methods "opendb" and "closedb" and
2. If you go down this route, you should return IDisposable (the connection object) from the opendb() method, and wrap the action in a using block to ensure that the connection gets closed.

Typically, you can just let the repository create a connection for each method, so you just have to get it right in your repository methods. The challenge comes when you want to perform multiple actions against the repository, without using a separate connection for each piece.

To achieve that, you could expose the notion of a unit-of-work from the repository. Your unit of work will implement the interface for the repository's methods, so you can't call them outside of a unit-of-work. It will also implement IDisposable, so whenever you call into your repository you will use a using block. Internally, the repository will manage the connection, but will neither expose the connection nor "talk about it."

For example:

public ActionResult SomeAction(int CarId)
{
     using (var repo = CarRepository.BeginUnitOfWork())
     {
        var car = repo.read(CarId);
        // do something meaningful with the car, do more with the repo, etc.
     }
}