Unit of Work Pattern for creating users

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:

• Fetch the Connection String

• Open the database connection

• Start the transaction

• Inserts a user record

• Loop through User Role entity list and Insert all the records

• Commits the transaction

• 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 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.In

#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:

• 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.

StackExchange Code Review Q#113922, answer score: 5