1
\$\begingroup\$

For some database requests, I like to use raw ADO.NET.

In the context of a web request, I have created a class which provides an open IDbConnection object.

I use a Dependency Injection library ("DI") to do this, scoping the object instantiated from this class to a web request. So, disposal of the object is handled by the DI container.

The abstraction for this class is simple and looks like this:

public interface IDbConnectionManager : IDisposable
{
    IDbCommand BuildCommand(DbParameter[] parameters, string query);
    IDbConnection GetOpenConnection();
}

And the concrete implementation, written for Sql Server, looks like this:

public class DbConnectionManager : IDbConnectionManager
{
    public SqlConnection DbConnection;
    public string ConnectionString { get; set; }

    public DbConnectionManager(string connectionString)
    {
        ConnectionString = connectionString;
    }

    public IDbConnection GetOpenConnection()
    {
        return GetOpenSqlConnection();
    }

    private SqlConnection GetOpenSqlConnection()
    {
        if (ReferenceEquals(DbConnection, null))
        {
            DbConnection = new SqlConnection(ConnectionString);
        }

        if (DbConnection.State != ConnectionState.Open)
        {
            DbConnection.Open();
        }

        return DbConnection;
    }

    public IDbCommand BuildCommand(DbParameter[] parameters, string query)
    {
        if (parameters == null) throw new ArgumentNullException(nameof(parameters));
        if (string.IsNullOrWhiteSpace(query)) throw new ArgumentException(nameof(query));

        var command = new SqlCommand { Connection = GetOpenSqlConnection() };
        command.Parameters.AddRange(parameters);
        command.CommandText = query;

        return command;
    }

    public void Dispose()
    {
        DbConnection?.Dispose();
    }
}

You can see the BuildCommand method uses the GetOpenSqlConnection for its connection.

The main reason I have taken this approach is because DbConnections are expensive. But I'm not sure whether that relates to creating a connection or opening one.

I'm aware that connection pooling is available (if enabled), but I figured for simple web requests (think API), with perhaps just 2 queries to the database, that this would probably be a good approach.

And as can be seen, there's not a lot to the code.

I just wanted a bit of feedback on this code/approach. And if you can see any potential problems with it, by all means let me know that too.

As a last comment, I am aware that a developer could manually call Dispose or use it in a using block (thereby calling Dispose). This would be on me as lead developer to ensure that this doesn't happen and that devs understand that the DI container disposes of the object.

\$\endgroup\$
2
\$\begingroup\$

As far as I know, in ADO.NET the connection pooling is enabled by default, and if you don't need to pool connections, you'll have to explicitly disable pooling.

So, for your implementation, everything seems okay to me, but DbConnectionManager seems to be specific for handling the connection! so, BuildCommand method will be odd in this class!

I would prefer to rename it to something covers all exposed operations and keep everything simpler.

for GetOpenSqlConnection() this could be unnessary, since you can do it in the property level.

What I suggest is to make DbConnection & ConnectionString static, so you ensure you only have a single instance of SqlConnection. and create two constructors one takes SqlConnection, and the other one takes connectionString. you would have something like this :

public class DbConnectionManager : IDbConnectionManager
{
    // make it static to have a single instance
    private static SqlConnection DbConnection
    {
        get => DbConnection is null ? new SqlConnection(ConnectionString) : DbConnection;
        set => DbConnection = value;
    }

    // store the connectionString of SqlConnection, for connection backup.
    private static string ConnectionString { get; set; } = DbConnection.ConnectionString;


    public DbConnectionManager(string connectionString) 
        : this(new SqlConnection(connectionString)) { }

    public DbConnectionManager(SqlConnection dbConnection)
    {
        DbConnection = dbConnection;
        ConnectionString = dbConnection.ConnectionString;
    }
}

in both constructors, they're initiating a new DbConnection and the ConnectionString is just backup, in case your actual DbConnection is lost, you can re-initiate it with the connectionString that you've stored.

doing that, it'll eleminate the need of :

private SqlConnection GetOpenSqlConnection()
{
    if (ReferenceEquals(DbConnection, null))
    {
        DbConnection = new SqlConnection(ConnectionString);
    }

    if (DbConnection.State != ConnectionState.Open)
    {
        DbConnection.Open();
    }

    return DbConnection;
}

for :

public IDbCommand BuildCommand(DbParameter[] parameters, string query)
{
    if (parameters == null) throw new ArgumentNullException(nameof(parameters));
    if (string.IsNullOrWhiteSpace(query)) throw new ArgumentException(nameof(query));

    var command = new SqlCommand { Connection = GetOpenSqlConnection() };
    command.Parameters.AddRange(parameters);
    command.CommandText = query;

    return command;
}

Since there is no actual execution is going here, opening a connection here is not needed, because you built the command and return the instance back to execute it somewhere else. this may requires you to create two public methods for opening and closing connection before you call the execute methods such as command.ExecuteNonQuery().

What I would do is maybe make this method private, and create public methods for each SQL execution type such as ExecuteNonQuery and ExecuteScalar and ExecuteReader, with the same arguments of BuildCommand has, and all will be from within the same class. Something like this :

public void ExecuteNonQuery(DbParameter[] parameters, string query)
{
    var command = BuildCommand(parameters, query);

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

    command.ExecuteNonQuery();
}

The best approach for that will be creating a static property of SqlCommand then you initiate it, use it across the class, dispose it whenever you're done.

another question got in my mind is this :

if (parameters == null) throw new ArgumentNullException(nameof(parameters));

why you made parameters required? suppose you need to execute a query with no parameters such as SELECT * FROM table, then, you would have to adjust the current implementation or create new method for that. So, keeping it optional will come in handy.

\$\endgroup\$
  • \$\begingroup\$ Thanks for your suggestions. I'm still reviewing them. One thing I completely agree with is having the BuildCommand method in that class as it violates the Single Responsibility Principle. I'm thinking of creating another abstraction which handles commands and injecting the IDbConnectionManager abstraction into that. I'll work with it a bit tonight and see if I can run with some of your suggestions. Cheers. \$\endgroup\$ – onefootswill Dec 16 '19 at 4:21
  • \$\begingroup\$ The only thing I wasn't sold on was making the connection static. The class will be a single instance for the duration of the request. And I want the retrieval of the connection to be part of the abstraction (interface). I appreciate your time and your thoughts. It's great to bounce ideas off people and get good alternative solutions. Cheers. \$\endgroup\$ – onefootswill Dec 16 '19 at 23:50
  • \$\begingroup\$ @onefootswill making it static depends on your overall usage and implementation. so, go with whatever you see best fit for your implementation. \$\endgroup\$ – iSR5 Dec 17 '19 at 0:17
  • \$\begingroup\$ Indeed. The other thing I was contemplating was moving your ExecuteNonQuery code into an extension method over the connection class. Kinda like what Dapper does. Good times :) \$\endgroup\$ – onefootswill Dec 17 '19 at 1:15
  • \$\begingroup\$ @onefootswill for long-term usage, I wouldn't suggest it, as, these are core functionality, and it needs to be in a concrete implementation. You might need to create a wrapper for your classes, this would make things easier to work with (kinda like small framework). \$\endgroup\$ – iSR5 Dec 17 '19 at 1:30

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.