Generic class for Unit of Work pattern

Question

In my project I use the Unit of Work pattern and it works really well. The implementation is the following:

public class UnitOfWork : IUnitOfWork
{
    private readonly MokaKukaTrackerDbContext _dbContext;

    public UnitOfWork(MokaKukaTrackerDbContext dbContext)
    {
        this._dbContext = dbContext;
    }

    public void Commit()
    {
        _dbContext.SaveChanges();
    }

    public void Dispose()
    {
        _dbContext.Dispose();
        GC.SuppressFinalize(this); // Preventing to call Object.finalize(), since the resource was already cleand up by Dispose()
    }
}

I use this unit of work pattern if I want to presist different models via multiple repositories. The following ContainerPlacerForNewOrderService represents an usage for the unit of work pattern, in my project:

public class ContainerPlacerForNewOrderService : IContainerPlacerForNewOrderService
{
    private readonly IUnitOfWork _unitOfWork;
    private readonly IOrderRepository _orderRepository;
    private readonly IContainerRepository _containerRepository;
    private readonly ITruckDriverRepository _truckDriverRepository;

    public ContainerPlacerForNewOrderService(IUnitOfWork unitOfWork, IOrderRepository orderRepository, IContainerRepository containerRepository, ITruckDriverRepository truckDriverRepository)
    {
        _unitOfWork = unitOfWork;
        _orderRepository = orderRepository;
        _containerRepository = containerRepository;
        _truckDriverRepository = truckDriverRepository;
    }

    public void Assign(ContainerAssignerViewModel viewModel)
    {
        using (_unitOfWork)
        {
            ChangeContainerStatusAndIncreaseNumberOfTurns(viewModel);

            ChangeAssignedOrderIdAndTruckDriverOfContainer(viewModel);

            _unitOfWork.Commit();
        }
    }

    private void ChangeContainerStatusAndIncreaseNumberOfTurns(ContainerAssignerViewModel viewModel)
    {
        var order = _orderRepository.Get(viewModel.OrderId);
        order.Status = OrderStatus.Active;
        order.NumberOfTurns++;

        _orderRepository.Update(order);
    }

    private void ChangeAssignedOrderIdAndTruckDriverOfContainer(ContainerAssignerViewModel viewModel)
    {
        var container = _containerRepository.GetByCompoundId(viewModel.ContainerCompoundId);
        container.OrderId = viewModel.OrderId;
        container.LayedDownBy = _truckDriverRepository.GetByFirstName(viewModel.TruckDriver);

        _containerRepository.Update(container);
    }

}

Recently I realized that I write a lot of duplicated code for every similar class like this. Namely I always add the IUnitOfWork dependency, use "using" with the injected unitOfWork dependency, and at the end of it I commit all the changes. Therefore I would make a generic class where I would wrap the mentioned duplicated logic. I've implemented the following:

public class RunnableTransaction
{

    private readonly IUnitOfWork _unitOfWork;

    public RunnableTransaction(IUnitOfWork unitOfWork)
    {
        _unitOfWork = unitOfWork;
    }

    public void WithoutResult(Action transactionToDatabase)
    {
        using (_unitOfWork)
        {
            transactionToDatabase.Invoke();

            _unitOfWork.Commit();
        }
    }

    public T WithResult<T>(RunnableWithReturnValue<T> transactionToDatabase)
    {
        T result;
        using (_unitOfWork)
        {
            result = transactionToDatabase.Invoke();

            _unitOfWork.Commit();
        }

        return result;
    }

    public T ReadOnly<T>(RunnableWithReturnValue<T> transactionToDatabase)
    {
        T result;
        using (_unitOfWork)
        {
            result = transactionToDatabase.Invoke();
        }

        return result;
    }

    public delegate T RunnableWithReturnValue<out T>();
}

Using this class I could make the following class for ContainerPlacerForNewOrderService

public class ContainerPlacerForNewOrderService
{
    private readonly IUnitOfWork _unitOfWork;
    private readonly IContainerPlacerForNewOrderService _containerPlacerForNewOrderService;

    public ContainerPlacerForNewOrderService(IUnitOfWork unitOfWork, IContainerPlacerForNewOrderService containerPlacerForNewOrderService)
    {
        _unitOfWork = unitOfWork;
        _containerPlacerForNewOrderService = containerPlacerForNewOrderService;
    }

    public void Assign(ContainerAssignerViewModel viewModel)
    {
        new RunnableTransaction(_unitOfWork).WithoutResult(() => _containerPlacerForNewOrderService.Assign(viewModel));
    }
}

What do you think about this solution? Is it a right direction in order to remove the duplication? I also would like to know whether this using(_unitOfWork) and _unitOfWork.Commit() is enough for handling the transactions in my project.

Answer 1

A few points:

1. You should only ever Dispose() something if you own it's lifecycle. Otherwise the person calling your method might not realise you've done it. So

   using (new UnitOfWork()) { ... }
   

   and not

   using (_unitOfWork) {}
   

   In your case, if you want to break the dependency there, you can pass a Func<IUnitOfWork> into your constructor, which creates your UOW.

2. Architecturally, your ContainerPlacerForNewOrderService class is actually not really a class, but instead a method that operates on its parameters. You might refactor it completely to remove the class, unless you have some very pressing reason to want to have a class there.

3. Your RunnableTransaction class would actually work better as a set of extension methods on IUnitOfWork - and there is no need for the RunnableWithResult delegate - you can just use Func<T>

4. Overall, the major mistake you're making in your code is to create classes and instances of classes (and therefore state) that just isn't necessary to achieve what you want. The result is lots and lots of code, where a few lines would have done the job.