Unit of Work Pattern for creating users

Question

CreateUser function assumes the data being delivered via parameters are clean and gets called or initiated from the Business Manager (another class). This function is responsible to do the following things:

1. Fetch the Connection String
2. Open the database connection
3. Start the transaction
4. Inserts a user record
5. Loop through User Role entity list and Insert all the records
6. Commits the transaction
7. Finally or Catch Code block gets executed

I would like to identify is this a correct way of implementing Unit of Work pattern? Is there any catch in my implementation? Comments are written through out the code. By the way, am not using Entity Framework (using Dapper) and don't have an intention to use it.

using System;
using System.Collections.Generic;
using System.Data;
using Application.Entities.Interfaces;
using Dapper;

namespace Application.DataStore.UnitOfWork
{
    public class UserUnitOfWork
    {
        private IDbConnection connection;
        private IDbTransaction transaction;

        #region CreateTenant
        public void CreateUser(IUserEntity userEntity, IEnumerable<IUsersUserRoleEntity> usersUserRoleListEntity)
        {
            try
            {
                //fetching the connection string from the config file.
                this.connection = new DbConnectionHelper.DbConnectionHelper().GetDbConnection();

                //open the connection
                this.connection.Open();

                //start the transaction
                this.transaction = this.connection.BeginTransaction();

                //insert into user table
                int? userId = this.connection.Insert(userEntity, transaction);

                //multiple inserts in a loop for user roles
                foreach (var userRole in usersUserRoleListEntity)
                {
                    //assign UserId before the insert
                    userRole.UserId = userId.Value;

                    //insert into user role table
                    this.connection.Insert(userRole, transaction);
                }

                //commit
                this.transaction.Commit();
            }
            catch (Exception ex)
            {
                this.transaction.Rollback();
                throw ex;
            }
            finally
            {
                this.transaction.Dispose();  //Transaction dispose
                this.transaction = null;

                this.connection.Close();
                this.connection.Dispose();
                this.connection = null;
            }
        }
        #endregion
    }
}

Answer 1

#region

#region CreateTenant

Get rid of those. The region encompasses only a single method anyway, and your IDE should already allow you to collapse method scopes. Even if the region had more content, I'd recommend against it. See:

Are #regions an antipattern or code smell?

---

comments

//open the connection
this.connection.Open();

//start the transaction
this.transaction = this.connection.BeginTransaction();

These comments are about as useful as:

i++; // increment i

Good comments are comments that let the code speak for itself and only show up when there's a need to say why the code is doing what it's doing.

Good comments say why, not what. I would simply remove all comments in your code, because comments that state the obvious are nothing but a distraction and a waste of time.

this.transaction.Dispose();  //Transaction dispose

---

exceptions

Something hit me in your description of the code:

7. Finally or Catch Code block gets executed

That's not how finally works. finally blocks get executed regardless of whether or not an exception was thrown - it runs even when the catch block runs.

Speaking of the catch block:

    catch (Exception ex)
    {
        this.transaction.Rollback();
        throw ex;
    }

The problem is that throw ex isn't how you cleanly rethrow an exception. What you've done here destroys the stack trace and as far as client code is concerned, the stack trace begins in the catch block of this CreateUser method, which we all know is a lie.

This would have preserved the precious stack trace with all of its information:

    catch (Exception)
    {
        this.transaction.Rollback();
        throw;
    }

---

unit of work

A unit of work encapsulates a transaction - you know that.

    private IDbConnection connection;
    private IDbTransaction transaction;

That was a good start!

The problem is that, well..

finally
{
    this.transaction.Dispose();  //Transaction dispose
    this.transaction = null;

    this.connection.Close();
    this.connection.Dispose();
    this.connection = null;
}

You can only ever call the method once in a transaction. You own that transaction and that connection, so you should be disposing them.. and you do... but then if they're only going to be used in the CreateUser method, they shouldn't be instance-level fields, but local variables wrapped in a using block - and then you wouldn't even need that finally block.

Your disposing is overkill. Setting the object reference to null is useless, as is explicitly Close()-ing a connection that you're Dispose()-ing.

Alternatively, take the connection and transaction as a constructor argument, keep the instance-level fields, and implement IDisposable to dispose them.

Answer 2

Let's start with class coupling. This line

this.connection = new DbConnectionHelper.DbConnectionHelper().GetDbConnection();

is a dependency of your class and should instead be managed and injected by the caller (preferably in the constructor) like so

public class UserUnitOfWork
{
    private readonly IDbConnection connection;

    public UserUnitOfWork(IDbConnection connection)
    {
        if (connection == null)
        {
            throw new ArgumentNullException("connection");
        }

        this.connection = connection;
    }

Now the coupling between UserUnitOfWork and DbConnectionHelper is broken and you're coding to the IDbConnection interface. I also added readonly to the class member to indicate it should not be modified by any other code after construction.

Also note that I didn't include the transaction member in that snippet. It doesn't need to be there. Its lifetime is within the CreateUser method only.

Next,

catch (Exception ex)
{
    this.transaction.Rollback();
    throw ex;
}

is almost always incorrect. throw ex throws a new exception at that statement, causing a new stack trace to be generated. You likely don't want to do that, but rather

catch (Exception)
{
    this.transaction.Rollback();
    throw;
}

Just throw preserves the exact location of the exception and the stack trace.

Next,

this.connection.Close();
this.connection.Dispose();

is redundant. However, the whole block is handled nicely with a construct called using. However, if you follow the "injectable" advice above, it's really up to the caller to Dispose() it and you'd likely keep the Close().

Answer 3

This is not a correct use of the UoW. A Unit of work "does two important things: first it maintains in-memory updates and second it sends these in-memory updates as one transaction to the database." [1]

Your unit of work looks more like a Repository pattern, which takes care of hiding the implementation of the data access.

In fact, the UoW should take care of the transaction part, and the Repository should take care of the data access, so at the moment both are mixed up in your class.

The rest of your code looks pretty fine.