4
\$\begingroup\$

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.

\$\endgroup\$
3
\$\begingroup\$

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.

\$\endgroup\$
  • 2
    \$\begingroup\$ lol. I suppose, thinking about it, the using statement is disposing something it doesn't own. I stand by 1 though. Better not than to. \$\endgroup\$ – Adam Brown Jan 11 '18 at 21:27
  • 1
    \$\begingroup\$ @Mat'sMug it's very practical when the new owner takes care of disposing the resources it receives because then you can easily do things like this return new CsvReader(File.OpenText(path), separator); where you create resources inline without having to think about how do I dispose the file-stream now? I makes a lot of thing a lot easier and I think it's a good design. \$\endgroup\$ – t3chb0t Jan 12 '18 at 5:19
  • 1
    \$\begingroup\$ @Mat'sMug Without it, you could not hide reading a file behind a simple interface like CsvReader FromFile( string path) and you would always have to create the file-reader yourself. such a waste of time. \$\endgroup\$ – t3chb0t Jan 12 '18 at 5:24
  • 1
    \$\begingroup\$ @t3chb0t I agree the reader stuff was implemented like this because it makes a convenient API, but that's framework design, not software design. If something is constructor-injected, it's owned by someone else, and shouldn't be disposed with that class; I'm with Adam here - disposing constructor-injected dependencies is outright dangerous, 99.9% of the time. \$\endgroup\$ – Mathieu Guindon Jan 12 '18 at 5:26
  • 1
    \$\begingroup\$ 1 was aimed at the ContainerPlacerForNewOrderService class, which as you said, is unnecessary. \$\endgroup\$ – Adam Brown Jan 12 '18 at 18:32

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.