2

Currently I have 3 service classes, they all have a similar pattern, and I need to make a 4th service class which amalgamates the functionality of the other three.

Current three classes

public class PersonService : IPersonService
{
    public PersonServiceResult CreatePerson(CreatePersonParams createPersonParams)
    {
        using (var unitOfWork = _unitOfWorkFactory.Create())
        {
            // [code]

            unitOfWork.Commit();
            return new PersonServiceResult(...);
        }
    }
}

public class OrderService : IOrderService
{
    public OrderServiceResult CreateOrder(
        int personId, 
        IEnumerable<PurchaseItem> items)
    {
        using (var unitOfWork = _unitOfWorkFactory.Create())
        {
            // [code]

            unitOfWork.Commit();
            return new OrderServiceResult(...);
        }
    }
}

public class PaymentProcessingService : IPaymentProcessingService
{
    public PaymentResult ProcessPayment(int orderId, CreditCard creditCard)
    {
        using (var unitOfWork = _unitOfWorkFactory.Create())
        {
            // [code]

            unitOfWork.Commit();
            return new PaymentResult(...);              
        }
    }
}

Now I have a need to use the behavior in all 3 of these classes into one unit-of-work, I still need the 3 other classes to exist for other dependecies in the project.

My first thought would be to extract a class for each one of the 3 services which takes a unit-of-work as an argument.

Extraction of behavior from current classes

public class PersonCreator
{
    public CreatePersonResult CreatePerson(
        IUnitOfWork unitOfWork, 
        CreatePersonParams createPerson)
    {
        // [code]
    }
}

public class PersonService : IPersonService
{
    private readonly PersonCreator Creator = new PersonCreator();

    public PersonServiceResult CreatePerson(CreatePersonParams createPersonParams)
    {
        using (var unitOfWork = _unitOfWorkFactory.Create())
        {
            var result = Creator.CreatePerson(unitOfWork, createPersonParams);

            unitOfWork.Commit();
            return new PersonServiceResult(result);
        }
    }       
}

Proposed new class using extracted behavior

public class QuickOrderService : IQuickOrderService
{
    private readonly PersonCreator PersonCreator = new PersonCreator(); 
    private readonly OrderCreator OrderCreator = new OrderCreator();    
    private readonly PaymentProcessor PaymentProcessor = new PaymentProcessor();    

    public QuickOrderResult CreateQuickOrder(
        CreatePersonParams createPersonParams,
        IEnumerable<PurchaseItem> items,
        CreditCard creditCard)
    {
        using (var unitOfWork = _unitOfWorkFactory.Create())
        {
            var person = PersonCreator.CreatePerson(unitOfWork, createPersonParams);
            var order = OrderCreator.CreateOrder(unitOfWork, person.Id, items);
            var payment = PaymentProcessor.Process(unitOfWork, order.Id, creditCard);

            unitOfWork.Commit();

            return QuickOrderResult(...);
        }
    }
}

This works fine, but I am concerned about testing this new class.

Questions about proposal

  1. Duplicate the majority of the tests for the QuickOrderService class
  2. Do not test this class as the majority of it is covered by other tests
  3. Inject PersonCreator, OrderCreator, and PaymentProcessor into QuickOrderService instead of instanciating them and just test that QuickOrderService calls the dependencies correctly

Though when doing this whole excersise it makes me think there may be a better solution that reduces duplication while retains code coverage.

  • What exactly are your rules for how a unit of work is used? – Ben Aaronson Apr 8 '15 at 16:00
  • Not sure what you mean about rules of usage, but we use it primarily to group operations to ensure they're done atomically. Primarily it's an abstraction over a a database transaction and fires domain events when the tx completes. – Matthew Apr 8 '15 at 16:05
  • DO test this class to check that it actually uses the classes it is aggregating/composing and to ensure that it keeps doing that. Injecting your creator classes is going to make that a lot easier. I'd even go as far as to see that without a mocking framework that can fake on the fly, you have no option but to use injection. So your option 3. – Marjan Venema Apr 8 '15 at 18:10
2

I would start with eliminating the duplicated code first by building a generic creation service, something along the lines of

   class GenericCreatorService<T>
   {
        UnitOfWorkFactory _unitOfWorkFactory;

        // ...

        public T Create(Func<T,UnitOfWork> func)
        {
           using (var unitOfWork = _unitOfWorkFactory.Create())
           {
                T result = func(unitOfWork);
                unitOfWork.Commit();
                return result;
           }
        }
   }

Now you just need to unit-test that once. Reuse it to implement your original service classes, for example,

public class PersonService : IPersonService
{
    private readonly PersonCreator Creator = new PersonCreator();

    public PersonServiceResult CreatePerson(CreatePersonParams createPersonParams)
    {
        var gs = new GenericCreatorService<PersonServiceResult>(...);
        return gs.Create( uow => PersonCreator.CreatePerson(uow,createPersonParams))
    }       
}

You should focus on testing the individual Creator classes, one test for each, and its debatable if you really need to unit test the individual service classes any more, since they contain only some very simple orchestration code. If you feel you need an extra test for your QuickOrderService because the real implementation is more complicated, you can still do what @JDT suggested and use DI for the other creators.

  • 1
    An after this, replace the Create methods with actual classes and you get yourself Query objects (ayende.com/blog/154241/…) – Euphoric Apr 9 '15 at 10:34
  • @Euphoric Thanks for the blog post, looks to be a useful pattern to follow. – Matthew Apr 9 '15 at 13:55
  • @DocBrown I ended up going with this solution, the QuickOrderService I ended up mocking out the dependencies ensure its correctness. – Matthew Apr 9 '15 at 13:56
1

Inject the three Creator-classes and test the calls to them by unittesting the QuickOrderService. This keeps the tests on the relevant classes.

Also, you might not need to haul the unitOfWork around. The implementation of Create could be changed to simply create a new transaction if none exists and otherwise return the existing transaction. This could keep track of the amount of calls to Create and Commit and only commit the transaction when the 'last' transaction that was requested is committed. Right now you have three services that take a unitOfWork and one that doesn't. Strive for consistency in your project and either pass ALL the services a unitOfWork or have the service manage a unitOfWork for itself.

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.