10
\$\begingroup\$

Lately, I have been reading Uncle Bob's Clean Code book, and decided to try and follow some of his principles to make a class that was a somewhat confusing static helper class, with an abundance of private static methods, into an instance class which more clearly expresses its intent.

The problem is to execute a stored procedure based upon a variety of attributes that may be applied to it, like with with parameters, with a command timeout, or within a transaction, etc.

Class Code:

I have tried to emulate a builder pattern with a fluid API, so the class allows you to prepare the various attributes of before calling Execute to with those attributes.

public class StoredProcedureExecuter<TResultSetType> : IDisposable
        where TResultSetType : class, new()
{
    #region Fields

    private readonly DbConnection _connection;
    private readonly string _procedureName;
    private readonly Type _resultSetType;
    private bool _connectionAlreadyOpen;
    private IEnumerable<SqlParameter> _procedureParameters;
    private int? _commandTimeoutOverride;
    private CommandBehavior _commandBehavior;
    private SqlTransaction _transaction;
    private DbCommand _command;

    #endregion

    #region Constructors

    /// <summary>
    /// Initializes a new instance of the <see cref="StoredProcedureExecuter{TResultSetType}"/> class.
    /// </summary>
    /// <param name="connection">The databse connection to execute the procedure against.</param>
    /// <param name="procedureName">Name of the procedure to execute.</param>
    /// <exception cref="System.ArgumentNullException">
    /// connection
    /// or
    /// procedureName
    /// </exception>
    public StoredProcedureExecuter(
        DbConnection connection,
        string procedureName)
    {
        if (connection == null) throw new ArgumentNullException("connection");
        if (string.IsNullOrWhiteSpace(procedureName)) throw new ArgumentNullException("procedureName");

        _connection = connection;
        _procedureName = procedureName;
        _resultSetType = typeof(TResultSetType);
    }

    #endregion

    #region Dispose and Finalise

    /// <summary>
    /// Gets a value indicating whether this <see cref="StoredProcedureExecuter{TResultSetType}"/> 
    /// is disposed.
    /// </summary>
    /// <value>
    ///   <c>true</c> if disposed; otherwise, <c>false</c>.
    /// </value>
    public bool Disposed { get; private set; }

    /// <summary>
    /// Finalizes an instance of the <see cref="StoredProcedureExecuter{TResultSetType}"/> class.
    /// </summary>
    ~StoredProcedureExecuter()
    {
        Dispose(false);
    }

    /// <summary>
    /// Performs application-defined tasks associated with freeing, releasing, 
    /// or resetting unmanaged resources.
    /// </summary>
    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }

    /// <summary>
    /// Releases unmanaged and - optionally - managed resources.
    /// </summary>
    /// <param name="disposing">
    /// <c>true</c> to release both managed and unmanaged resources; 
    /// <c>false</c> to release only unmanaged resources.
    /// </param>
    private void Dispose(bool disposing)
    {
        if (!Disposed)
        {
            if (disposing)
            {
                DisposeCommand();
            }

            // There are no unmanaged resources to release, but
            // if we add them, they need to be released here.
        }
        Disposed = true;
    }

    #endregion

    #region Public Members

    /// <summary>
    /// Executes the stored procedure.
    /// </summary>
    /// <returns></returns>
    /// <exception cref="System.ObjectDisposedException">
    /// Cannot call Execute when this object is disposed</exception>
    public StoredProcedureExecuter<TResultSetType> Execute()
    {
        if (Disposed) throw new ObjectDisposedException("Cannot call Execute when this object is disposed");

        CacheOriginalConnectionState();

        try
        {
            OpenClosedConnection();
            CreateCommand();
            ExecuteCommand();
        }
        catch (Exception ex)
        {
            AddMoreInformativeInformationToExecuteError(ref ex);
            throw;
        }
        finally
        {
            DisposeCommand();
            RestoreOriginalConnectionState();
        }
        return this;
    }

    /// <summary>
    /// Gets the results of the stored procedure call.
    /// </summary>
    /// <value>
    /// The results of the stored procedure call.
    /// </value>
    public TResultSetType Results { get; private set; }

    public StoredProcedureExecuter<TResultSetType> WithCommandBehavior(CommandBehavior commandBehavior)
    {
        _commandBehavior = commandBehavior;
        return this;
    }

    public StoredProcedureExecuter<TResultSetType> WithParameters(IEnumerable<SqlParameter> procedureParameters)
    {
        if (procedureParameters == null) throw new ArgumentNullException("procedureParameters");

        _procedureParameters = procedureParameters;

        return this;
    }

    public StoredProcedureExecuter<TResultSetType> WithCommandTimeoutOverride(int commandTimeoutOverride)
    {
        _commandTimeoutOverride = commandTimeoutOverride;

        return this;
    }

    public StoredProcedureExecuter<TResultSetType> WithTransaction(SqlTransaction transaction)
    {
        _transaction = transaction;

        return this;
    }

    #endregion

    #region Public Factory Methods

    /// <summary>
    /// Creates a new instance of the <see cref="StoredProcedureExecuter{TResultSetType}"/> class.
    /// </summary>
    /// <param name="connection">The databse connection to execute the procedure against.</param>
    /// <param name="procedureName">Name of the procedure to execute.</param>
    /// <exception cref="System.ArgumentNullException">
    /// connection
    /// or
    /// procedureName
    /// </exception>
    public static StoredProcedureExecuter<TResultSetType> CreateStoredProcedureExecuter(
        DbConnection connection,
        string procedureName)
    {
        if (connection == null) throw new ArgumentNullException("connection");
        if (string.IsNullOrWhiteSpace(procedureName)) throw new ArgumentNullException("procedureName");

        return new StoredProcedureExecuter<TResultSetType>(connection, procedureName);
    }

    #endregion

    #region Private Members

    private void AddMoreInformativeInformationToExecuteError(ref Exception ex)
    {
        var detailedMessage = string.Format(
            ExceptionMessages.ErrorReadingStoredProcedure,
            _procedureName,
            ex.Message);
        Type exceptionType = ex.GetType();
        var fieldInfo = exceptionType.GetField("_message", BindingFlags.Instance | BindingFlags.NonPublic);

        if (fieldInfo != null) fieldInfo.SetValue(ex, detailedMessage);
    }

    private void CacheOriginalConnectionState()
    {
        _connectionAlreadyOpen = (_connection.State == ConnectionState.Open);
    }

    private void CreateCommand()
    {
        DisposeCommand();

        if (!HasCommandTimeoutOverride && !HasParameters && !HasTransaction)
        {
            CreateCommandWithoutParametersOrCommandTimeoutOrTransaction();
        }
        else if (HasCommandTimeoutOverride && !HasParameters && !HasTransaction)
        {
            CreateCommandWithoutParametersOrTransactionButWithCommandTimeout();
        }
        else if (!HasCommandTimeoutOverride && !HasParameters && HasTransaction)
        {
            CreateCommandWithoutParametersOrCommandTimeoutButWithTransaction();
        }
        else if (HasCommandTimeoutOverride && !HasParameters && HasTransaction)
        {
            CreateCommandWithoutParametersButWithCommandTimeoutAndTransaction();
        }
        else if (!HasCommandTimeoutOverride && HasParameters && !HasTransaction)
        {
            CreateCommandWithParametersButWithoutCommandTimeoutOrTransaction();
        }
        else if (HasCommandTimeoutOverride && HasParameters & !HasTransaction)
        {
            CreateCommandWithParametersAndCommandTimeoutButWithoutTransaction();
        }
        else if (!HasCommandTimeoutOverride && HasParameters & HasTransaction)
        {
            CreateCommandWithParametersAndTransactionButWithoutCommandTimeout();
        }
        else if (HasCommandTimeoutOverride && HasParameters && HasTransaction)
        {
            CreateCommandWithParametersCommandTimeoutAndTransaction();
        }
        else
        {
            throw new InvalidOperationException("An invalid combination of command attributes have been set!");
        }
    }

    private void CreateCommandWithoutParametersOrCommandTimeoutOrTransaction()
    {
        _command = StoredProcedureDbCommandCreator
            .CreateStoredProcedureDbCommandCreator(_connection, _procedureName)
            .BuildCommand()
            .Command;
    }

    private void CreateCommandWithoutParametersOrTransactionButWithCommandTimeout()
    {
        _command = StoredProcedureDbCommandCreator
            .CreateStoredProcedureDbCommandCreator(_connection, _procedureName)
            .WithCommandTimeout(_commandTimeoutOverride)
            .BuildCommand()
            .Command;
    }

    private void CreateCommandWithoutParametersOrCommandTimeoutButWithTransaction()
    {
        _command = StoredProcedureDbCommandCreator
            .CreateStoredProcedureDbCommandCreator(_connection, _procedureName)
            .WithTransaction(_transaction)
            .BuildCommand()
            .Command;
    }

    private void CreateCommandWithoutParametersButWithCommandTimeoutAndTransaction()
    {
        _command = StoredProcedureDbCommandCreator
            .CreateStoredProcedureDbCommandCreator(_connection, _procedureName)
            .WithCommandTimeout(_commandTimeoutOverride)
            .WithTransaction(_transaction)
            .BuildCommand()
            .Command;
    }

    private void CreateCommandWithParametersButWithoutCommandTimeoutOrTransaction()
    {
        _command = StoredProcedureDbCommandCreator
            .CreateStoredProcedureDbCommandCreator(_connection, _procedureName)
            .WithParameters(_procedureParameters)
            .BuildCommand()
            .Command;
    }

    private void CreateCommandWithParametersAndCommandTimeoutButWithoutTransaction()
    {
        _command = StoredProcedureDbCommandCreator
            .CreateStoredProcedureDbCommandCreator(_connection, _procedureName)
            .WithParameters(_procedureParameters)
            .WithCommandTimeout(_commandTimeoutOverride)
            .BuildCommand()
            .Command;
    }

    private void CreateCommandWithParametersAndTransactionButWithoutCommandTimeout()
    {
        _command = StoredProcedureDbCommandCreator
            .CreateStoredProcedureDbCommandCreator(_connection, _procedureName)
            .WithParameters(_procedureParameters)
            .WithTransaction(_transaction)
            .BuildCommand()
            .Command;
    }

    private void CreateCommandWithParametersCommandTimeoutAndTransaction()
    {
        _command = StoredProcedureDbCommandCreator
            .CreateStoredProcedureDbCommandCreator(_connection, _procedureName)
            .WithParameters(_procedureParameters)
            .WithCommandTimeout(_commandTimeoutOverride)
            .WithTransaction(_transaction)
            .BuildCommand()
            .Command;
    }

    private void DisposeCommand()
    {
        if (_command != null)
        {
            _command.Dispose();
            _command = null;
        }
    }

    private void ExecuteCommand()
    {
        if (HasNoReturnType)
        {
            ExecuteCommandWithNoReturnType();
            return;
        }

        ExecuteCommandWithResultSet();
    }

    private void ExecuteCommandWithNoReturnType()
    {
        _command.ExecuteNonQuery();
    }

    private void ExecuteCommandWithResultSet()
    {
        if (HasSingleRecordSetOnly)
        {
            ExecuteCommandForSingleRecordSet();
        }
        else
        {
            ExecuteCommandForMultipleRecordSets();
        }
    }

    private void ExecuteCommandForMultipleRecordSets()
    {
        Results = new TResultSetType();
        var recordSetIndex = 0;
        var resultSetTypeProperties = _resultSetType.GetMappedProperties();

        using (DbDataReader reader = _command.ExecuteReader(_commandBehavior))
        {
            bool readerContainsAnotherResult;
            do
            {
                var recordSetDtoList = GetRecordSetDtoList(resultSetTypeProperties, recordSetIndex);
                ReadRecordSetFromReader(reader, recordSetDtoList);

                recordSetIndex += 1;
                readerContainsAnotherResult = reader.NextResult();

            } while (readerContainsAnotherResult);
            reader.Close();
        }
    }

    private IList GetRecordSetDtoList(PropertyInfo[] resultSetTypePropertyInfos, int recordSetIndex)
    {
        var recordSetPropertyName = resultSetTypePropertyInfos[recordSetIndex].Name;
        var recordSetDtoList = GetRecordSetDtoList(recordSetPropertyName);
        EnsureRecorsetListIsInstantiated(recordSetDtoList, recordSetPropertyName);

        return recordSetDtoList;
    }

    private void ExecuteCommandForSingleRecordSet()
    {
        var recordSetDtoList = (IList)new TResultSetType();

        using (DbDataReader reader = _command.ExecuteReader(_commandBehavior))
        {
            ReadRecordSetFromReader(reader, recordSetDtoList);
            reader.Close();
        }

        Results = (TResultSetType)recordSetDtoList;
    }

    private void ReadRecordSetFromReader(DbDataReader reader, IList recordSetDtoList)
    {
        Type listItemType = recordSetDtoList.GetType().GetGenericArguments()[0];
        PropertyInfo[] listItemProperties = listItemType.GetMappedProperties();

        while (reader.Read())
        {
            AddRecordToResults(listItemType, recordSetDtoList, reader, listItemProperties);
        }
    }

    private IList GetRecordSetDtoList(string recordSetPropertyName)
    {
        PropertyInfo recordSetPropertyInfo = _resultSetType.GetProperty(recordSetPropertyName);
        IList recordSetDtoList = (IList)recordSetPropertyInfo.GetValue(Results);
        return recordSetDtoList;
    }

    private void EnsureRecorsetListIsInstantiated(
        IList dtoList,
        string listPropertyName)
    {
        if (dtoList != null) return;

        string errorMessage = string.Format(
            ExceptionMessages.RecordSetListNotInstatiated,
            _resultSetType.Name,
            listPropertyName);
        throw new NullReferenceException(errorMessage);
    }

    private void AddRecordToResults(
        Type outputType,
        IList results,
        DbDataReader reader,
        PropertyInfo[] dtoListItemTypePropertyInfos)
    {
        var constructorInfo = (outputType).GetConstructor(Type.EmptyTypes);
        bool noConstructorDefined = (constructorInfo == null);
        if (noConstructorDefined) return;

        var item = Activator.CreateInstance(outputType);
        reader.ReadRecord(item, dtoListItemTypePropertyInfos);
        results.Add(item);
    }

    private void OpenClosedConnection()
    {
        if (!_connectionAlreadyOpen) _connection.Open();
    }

    private void RestoreOriginalConnectionState()
    {
        if (!_connectionAlreadyOpen) _connection.Close();
    }

    private bool HasSingleRecordSetOnly
    {
        get { return _resultSetType.ImplementsICollectionInterface(); }
    }

    private bool HasCommandTimeoutOverride
    {
        get { return _commandTimeoutOverride.HasValue; }
    }

    private static bool HasNoReturnType
    {
        get { return (typeof(TResultSetType) == typeof(NullStoredProcedureResult)); }
    }

    private bool HasParameters
    {
        get { return _procedureParameters != null; }
    }

    private bool HasTransaction
    {
        get { return _transaction != null; }
    }

    #endregion
}

Calling code:

Where dbConnection is a System.Data.Common.DbConnection object and procedureFullName is the fully qualified Sql Server stored procedure name.

TResultSetType results;
using (var procedureExecuter = new StoredProcedureExecuter<TResultSetType>(dbConnection, procedureFullName))
{
    results = procedureExecuter
        .WithParameters(procedureSqlParameters)
        .WithCommandBehavior(CommandBehavior.Default)
        .WithCommandTimeoutOverride(commandTimeoutOverride)
        .WithTransaction(transaction)
        .Execute()
        .Results;
}

As you can see, I have opted for a fluid API which has methods which describe how to prepare the object, a method which will then execute the procedure, and a property to retrieve any results if they were returned.

Does this class now clearly reveal and express its intent? If not, where can I improve it?

\$\endgroup\$
5
\$\begingroup\$
  • The DbConnection class implements the IDbConnection interface, so unless you need something exposed by the Component base class, you could use the interface instead of the concrete type. This will make your code more flexible and testable.

  • If you're using C#6, you can use the nameof operator to refactor proof your argument exceptions.

     ... throw new ArgumentNullException(nameof(connection));
    
  • I don't see a benefit to storing the Type from the type parameter in an instance variable. You could get it only if you need it. A case of DRY gone too far? Maybe you know something I don't though.

It's really very pleasant to read this code, even if there's quite a bit of it. I'm also a big fan of fluent API's and it seems that you've implemented one that I wouldn't mind working with.

\$\endgroup\$
  • \$\begingroup\$ Thank you for the constructive review. I will certainly look at using the IDbConnection as I don't think there is anything I need that is not in the interface. It may however have a knock on effect on some of the objects this class uses. I will investigate and see. The deeper unit testing benefit will be a big plus! I'm not using C#6, but will investigate if there are other benefits. I can see your point on not caching the ResultType, as there is no runtime performance loss, due to the typing being done at compile time, The only benefit is mildly cleaner code, so I may leave that in. \$\endgroup\$ – Dib Dec 10 '15 at 6:45
  • \$\begingroup\$ I'll feed back after investigation into using the interface. Thanks again for the constructive review. \$\endgroup\$ – Dib Dec 10 '15 at 6:50
  • 1
    \$\begingroup\$ I have been able to swap out the concrete DbConnection to be the IDbConnection interface, and this has also driven out other code that this object depends upon to swap out usage of the concrete DbCommand to the IDbCommand interface and also DbDataReader to IDataReader, which is a good thing. All of my existing tests pass so nothing is broken from those changes, and I now have better options to create further tests. I'll update my post to include your suggested change. \$\endgroup\$ – Dib Dec 10 '15 at 7:06
  • 1
    \$\begingroup\$ Awesome @Dib, but please don't update your post. We have a policy of asking new questions for iterative reviews. Happy to help, it really was a pleasure to read this code. \$\endgroup\$ – RubberDuck Dec 10 '15 at 10:04
  • 2
    \$\begingroup\$ These interfaces are considered deprecated since .Net 2.0 and aren't currently available on .Net Core. So it may not be a good idea to use them. \$\endgroup\$ – svick Dec 10 '15 at 14:27
1
\$\begingroup\$

The CreateCommand function looks smelly to me, a lot of if/else and 8 private methods just for initializing the Command. You could simplify it the code sample below.

private void CreateCommand()
{
    DisposeCommand();

    if (!HasCommandTimeoutOverride && !HasParameters && !HasTransaction)
    {
        CreateCommandWithoutParametersOrCommandTimeoutOrTransaction();
    }
    else if (HasCommandTimeoutOverride && !HasParameters && !HasTransaction)
    {
        CreateCommandWithoutParametersOrTransactionButWithCommandTimeout();
    }
    else if (!HasCommandTimeoutOverride && !HasParameters && HasTransaction)
    {
        CreateCommandWithoutParametersOrCommandTimeoutButWithTransaction();
    }
    else if (HasCommandTimeoutOverride && !HasParameters && HasTransaction)
    {
        CreateCommandWithoutParametersButWithCommandTimeoutAndTransaction();
    }
    else if (!HasCommandTimeoutOverride && HasParameters && !HasTransaction)
    {
        CreateCommandWithParametersButWithoutCommandTimeoutOrTransaction();
    }
    else if (HasCommandTimeoutOverride && HasParameters & !HasTransaction)
    {
        CreateCommandWithParametersAndCommandTimeoutButWithoutTransaction();
    }
    else if (!HasCommandTimeoutOverride && HasParameters & HasTransaction)
    {
        CreateCommandWithParametersAndTransactionButWithoutCommandTimeout();
    }
    else if (HasCommandTimeoutOverride && HasParameters && HasTransaction)
    {
        CreateCommandWithParametersCommandTimeoutAndTransaction();
    }
    else
    {
        throw new InvalidOperationException("An invalid combination of command attributes have been set!");
    }
}

Sample code

private void CreateCommand()
{
    DisposeCommand();

    var creator = StoredProcedureDbCommandCreator.CreateStoredProcedureDbCommandCreator(_connection, _procedureName)

    if(HasCommandTimeoutOverride)
    {
      creator.WithCommandTimeout(_commandTimeoutOverride);
    }

    if(HasParameters)
    {
      creator.WithParameters(_procedureParameters);
    }

    if(HasTransaction)
    {
      creator.WithTransaction(_transaction);
    }

    _command = creator
            .BuildCommand()
            .Command;
}
\$\endgroup\$
  • \$\begingroup\$ You should add more text to backup your answer. This helps people better understand your code and avoid misinterpretations. \$\endgroup\$ – AnotherGuy Dec 15 '15 at 9:48
  • \$\begingroup\$ @AnotherGuy: I've updated my answer \$\endgroup\$ – Kien Chu Dec 15 '15 at 10:22
  • \$\begingroup\$ @kienct89 - I have followed your suggestion and the code is now much easier to read in that area. Great suggestion. Thanks. \$\endgroup\$ – Dib Dec 15 '15 at 21:17

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.