Is this the right way to dispose the SQLConnection

Question

I was wondering if my below implementation is the most efficient way to dispose the SQLconnection in this case.

I know normally if i'm using the SqlConnection directly I can just wrap the connection inside a using block to dispose it off automatically, but in this case i wanted to keep the connection open and available to All methods in the SQLRespository class.

public class SqlRepository : IRepository
{
    private readonly string connectionString;
    private SqlConnection connection;

    public SqlRepository(string connectionString)
    {
        this.connectionString = connectionString;
        connection = new SqlConnection(connectionString);
        connection.Open();
    }

    public void Method_A()
    {
      // uses the SqlConnection to fetch data
    }     

    public void Method_B()
    {
      // uses the SqlConnection to fetch data
    }     

    public void Dispose()
    {            
        connection.Dispose();
    }
}

Usage:

using (IRepository repository = new SqlRepository(connectionString))
{
   var item = repository.items;     
}

Update IRepository does implement IDisposable

Answer 1

Don't keep the connection open spanning calls. You're defeating connection pooling.

If you're working with a connection that's pooled (like sqlserver), it will pool and reuse. Just open and close within method a & b.

You could argue that if the caller does what you did with using with one method call it's fine. But if you do using {} with sqlconnection inside each worker method (1) the code will be simpler and (2) you're ensured the pooling wont be defeated (meaning your holding items out of the pooling when other requests could use it).

EDIT:

Adding pseudo based on comments.

The pattern is problematic because a caller can do.

//pseudo code
using (SqlRepository r)
{
    r.MethodA();

    // other code here that takes some time.  your holding a connection
    // out of the pool and being selfish.  other threads could have
    // used your connection before you get a chance to use it again.

    r.MethodB();
}  // freed for others here.

That will kill the scalability of the server - trust me. I've seen very large systems get choked by this - usually because they're spanning AT side transactions.

A better pattern:

class Repository
{
    void MethodA()
    {
        using (Sqlconnection)
        {
             // db call
        }
    }

    void MethodB()
    {
        using (Sqlconnection)
        {
            // you can even have multiple calls here (roundtrips)
            // and start transactions.  although that can be problematic
            // for other reasons.  
        }
    }

Now, the pool is most effective. I realize the question was on the disposable pattern - and yes you can do it. But ...

I would not let the connection span the lifetime of the repository.