Simple SqlHelper which wraps ADO.NET methods

Question

I am creating a simple SqlHelper which will simplify the ADO.NET method usage. Can someone please review this class for any issue or missing something?

public static class SqlHelper
{
    public static async Task<int> ExecuteNonQueryAsync(string connectionString, CommandType cmdType, string cmdText,
        params SqlParameter[] commandParameters)
    {
        using (var connection = new SqlConnection(connectionString))
        {
            using (var command = new SqlCommand(cmdText, connection))
            {
                try
                {
                    command.CommandType = cmdType;
                    command.Parameters.AddRange(commandParameters);
                    connection.Open();
                    return await command.ExecuteNonQueryAsync();
                }
                finally
                {
                    connection.Close();
                }
            }
        }

    }


    public static async Task<SqlDataReader> ExecuteReaderAsync(string connectionString, CommandType cmdType,
        string cmdText, params SqlParameter[] commandParameters)
    {
        using (var connection = new SqlConnection(connectionString))
        {
            using (var command = new SqlCommand(cmdText, connection))
            {
                try
                {
                    command.CommandType = cmdType;
                    command.Parameters.AddRange(commandParameters);
                    connection.Open();
                    return await command.ExecuteReaderAsync();
                }
                finally
                {
                    connection.Close();
                }
            }
        }
    }

    public static async Task<object> ExecuteScalarAsync(string connectionString, CommandType cmdType, string cmdText,
        params SqlParameter[] commandParameters)
    {
        using (var connection = new SqlConnection(connectionString))
        {
            using (var command = new SqlCommand(cmdText, connection))
            {
                try
                {
                    command.CommandType = cmdType;
                    command.Parameters.AddRange(commandParameters);
                    connection.Open();
                    return await command.ExecuteScalarAsync();
                }
                finally
                {
                    connection.Close();
                }
            }
        }
    }

}

Answer 1

Nesting.

You can reduce nesting by stacking the using blocks, like this:

    using (var connection = new SqlConnection(connectionString))
    using (var command = new SqlCommand(cmdText, connection))
    {
        try
        {
            command.CommandType = cmdType;
            command.Parameters.AddRange(commandParameters);
            connection.Open();
            return await command.ExecuteNonQueryAsync();
        }
        finally
        {
            connection.Close();
        }
    }

---

I like your usage of var, and the fact that you've wrapped the IDisposable stuff in using blocks. However this is overkill:

    finally
    {
        connection.Close();
    }

Because the connection is wrapped in a using block, the connection will be cleanly closed and disposed, whatever the reason is for the code to reach the end of the using scope. In other words, you can also ditch the try block, and further reduce nesting:

    using (var connection = new SqlConnection(connectionString))
    using (var command = new SqlCommand(cmdText, connection))
    {
        command.CommandType = cmdType;
        command.Parameters.AddRange(commandParameters);
        connection.Open();
        return await command.ExecuteNonQueryAsync();
    }

That's because a using block is a language shortcut for... a try...finally block.

---

The XxxxxHelper Syndrome

The problem with a static class called SqlHelper, is that over time, it becomes a dumping ground for anything remotely related to SQL; it grows hair and tentacles, and becomes a huge mess before you even notice: "Helper" is simply too vague of a name to result in anything cleanly focused.

I would suggest renaming it to something like SqlCommandWrapper - then it's far less likely to become bloated with things that have nothing to do with executing a SqlCommand, but might be remotely related to SqlWhatever.

Answer 2

Having written this type of class before, I would suggest an additional overload with the following signature:

public static async Task<IEnumerable<T>> ExecuteReaderAsync(
    string connectionString, 
    CommandType cmdType,
    string cmdText,
    Func<IDataReader, T> transform), 
    params SqlParameter[] commandParameters)

With this overload, you will iterate over the results returned by the data reader, and for each row, invoke the transform method and store it in a List<T>. When it's finished, return the result to the caller as an IEnumerable<T>.

My particular implementation always returned an empty enumerable, never null. You may choose to do otherwise. (I preferred predictability, and guaranteed that the method never returned null.)

This prevents callers from having to write this type of boilerplate themselves. They simply call:

var foo = await SqlHelper.ExecuteReaderAsync(
    myConnectionString,
    CommandType.StoredProcedure,
    "GetTheFoos",
    r => new Foo { Id = r["foo"], Name = r["Name"] },
    ); // No parameters

Other than that, the other reviews are splendid.

Answer 3

I don't find having to repeat the connection string with each command simpler. Why not make SqlHelper a non-static class, pass the connection string into a constructor and then use it from your methods?

---

Similarly, I believe most commands are going to use CommandType.Text, so that should be the default. You can't use optional parameters together with params, so you'll have to use a separate overload for that.

---

I'm not sure ExecuteReaderAsync is going to work. Doesn't the returned reader need the connection to be open?