2
\$\begingroup\$

I have DatabaseContext class.

public interface IDatabaseContext : IDisposable
{
    SqlConnection Connection { get; }
}

public class DatabaseContext : IDatabaseContext
{
    private readonly string _connectionString;
    private SqlConnection _connection;

    public DatabaseContext(string connectionString)
    {
        _connectionString = connectionString;
    }

    public SqlConnection Connection
    {
        get
        {
            if (_connection == null)
                _connection = new SqlConnection(_connectionString);

            if (_connection.State != ConnectionState.Open)
                _connection.Open();

            return _connection;
        }
    }

    public void Dispose()
    {
        if (_connection != null && _connection.State == ConnectionState.Open)
            _connection.Close();
    }
}

And I use it in PageService class using dependency injection, for example:

public class PageService
    {
        private readonly IDatabaseContext _databaseContext;

        public PageService(IDatabaseContext databaseContext)
        {
            _databaseContext = databaseContext;
        }

        public IEnumerable<CustomPage> GetAllPages()
        {
            IEnumerable<CustomPage> pages;
            using (var db = _databaseContext)
            {
                //using Dapper.Net to get list of pages.
                pages = db.Connection.GetList<CustomPage>();
            }
            return pages;
        }
    }

Do you see any improvement / issue of my DatabaseContext class?

\$\endgroup\$
3
\$\begingroup\$

I've never seen GetList() method on SqlConnection so I'm assuming this is your own class?

If that were the case would it not be best to have IDataContext abstract the Connection details away and return an IConnection contract.

public interface IDatabaseContext : IDisposable
{
    IConnection Connection { get; }
}

Doing this you can ensure your calling classes are completely decoupled from the implementation details of a connection. Otherwise any implementation of IDataContext would have been coupled to a SQLConnection and you lose the benefit of the initial abstraction.

\$\endgroup\$
  • \$\begingroup\$ Thanks for your reply, with IConnection it looks better! GetList() is the method of DapperExtensions library. \$\endgroup\$ – sDima May 18 '14 at 13:47
  • 1
    \$\begingroup\$ @sDima. Ah I didn't know that about Dapper. Makes sense now. In light of that makes even more sense to either make it an interface/abstract class so that it takes these dependencies i.e. Dapper out of the equation. \$\endgroup\$ – dreza May 18 '14 at 20:17
1
\$\begingroup\$

In your dispose method you should also dispose your SqlConnection :

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

About correct IDispose implementation read here: http://msdn.microsoft.com/en-us/library/b1yfkh5e%28v=vs.110%29.aspx

\$\endgroup\$

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.