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 StringOpen the database connectionStart the transactionInserts a user recordLoop through User Role entity list and Insert all the recordsCommits the transactionFinally or Catch Code block gets executedI 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    }}
Unit of Work Pattern for creating users
c#;design patterns;.net;repository
#region#region CreateTenantGet 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 connectionthis.connection.Open();//start the transactionthis.transaction = this.connection.BeginTransaction();These comments are about as useful as:i++; // increment iGood 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 disposeexceptionsSomething hit me in your description of the code:Finally or Catch Code block gets executedThat'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 workA 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.