Test-Friendly Architecture

Question

I have a question about the best way to design classes in order to be test-friendly. Suppose I have an OrderService class, which is used to place new orders, check the status of orders, and so on. The class will need to access customer information, inventory information, shipping information, etc. So the OrderService class will need to use CustomerService, InventoryService, and ShippingService. Each service also has its own backing repository.

What is the best way to design the OrderService class to be easily testable? The two commonly used patterns that I've seen are dependency injection and service locator. For dependency injection, I'd do something like this:

class OrderService
{
   private ICustomerService CustomerService { get; set; }
   private IInventoryService InventoryService { get; set; }
   private IShippingService ShippingService { get; set; }
   private IOrderRepository Repository { get; set; }

   // Normal constructor
   public OrderService()
   {
      this.CustomerService = new CustomerService();
      this.InventoryService = new InventoryService();
      this.ShippingService = new ShippingService();
      this.Repository = new OrderRepository();         
   }

   // Constructor used for testing
   public OrderService(
      ICustomerService customerService,
      IInventoryService inventoryService,
      IShippingService shippingService,
      IOrderRepository repository)
   {
      this.CustomerService = customerService;
      this.InventoryService = inventoryService;
      this.ShippingService = shippingService;
      this.Repository = repository;
   }
}

// Within my unit test
[TestMethod]
public void TestSomething()
{
   OrderService orderService = new OrderService(
      new FakeCustomerService(),
      new FakeInventoryService(),
      new FakeShippingService(),
      new FakeOrderRepository());
}

The disadvantage to this is that every time I create an OrderService object that I'm using in a test, it takes a lot of code to call the constructor within my tests. My Service classes also end up with a bunch of properties for each Service and Repository class that they use. And as I expand my program and add more dependencies between various Service and Repository classes, I have to go back and add more and more parameters to constructors of classes that I've already made.

For a service locator pattern, I could do something like this:

class OrderService
{
   private CustomerService CustomerService { get; set; }
   private InventoryService InventoryService { get; set; }
   private ShippingService ShippingService { get; set; }
   private OrderRepository Repository { get; set; }

   // Normal constructor
   public OrderService()
   {
      ServiceLocator serviceLocator = new ServiceLocator();
      this.CustomerService = serviceLocator.CreateCustomerService()
      this.InventoryService = serviceLocator.CreateInventoryService();
      this.ShippingService = serviceLocator.CreateShippingService();
      this.Repository = serviceLocator.CreateOrderRepository();         
   }

   // Constructor used for testing
   public OrderService(IServiceLocator serviceLocator)
   {
      this.CustomerService = serviceLocator.CreateCustomerService()
      this.InventoryService = serviceLocator.CreateInventoryService();
      this.ShippingService = serviceLocator.CreateShippingService();
      this.Repository = serviceLocator.CreateOrderRepository();   
   }
}

// Within a unit test
[TestMethod]
public void TestSomething()
{
   OrderService orderService = new OrderService(new TestServiceLocator());
}

I like how the service locator pattern results in less code when calling the constructors, but it also gives less flexibility.

What's the recommended way to set up my Service classes that have dependencies on several other Services and Repositories so that they can be easily tested? Are either or both of the ways that I showed above good, or is there a better way?

Answer 1

Just a really quick answer to put you on the right track. In my experience, if you aim for easily testable code you tend to end up with clean maintainable code as a nice side-effect. :-)

Some key points to remember:

1. The SOLID principles will really help you create good, clean, testable code.

   (S + O + I) Break this Service up into smaller services that only do one thing, and will therefore only have one reason to change. At a minimum placing an order and checking the status of an order are completely different things. If you think quite deeply about it, you don't really need to follow the most obvious steps (eg. check credit->check stock->check shipping), some of these can be done out of order - but that's a whole other story that would probably require a different business model. Anyway you can use the Facade pattern to create a simplified view on top of those smaller services if you really need it.

2. Use an IoC container (eg unity)

3. Use a Mocking framework (eg Moq)

4. The service locator pattern is actually considered an anti-pattern/code smell - so please don't use it.

5. Your tests should use the same paths as you real code, so get rid of the 'Normal constructor'. The 'Constructor used for testing' in your first example is what your constructor should look like.

6. Do NOT instantiate the required services inside your class - they should be passed in instead, as an Interface. The IoC container will help you deal with this part. By doing this you are following the D in Solid (Dependency inversion principle)

7. Avoid using/referencing static classes/methods directly inside your own classes as much as possible. Here I'm talking about using things like DateTime.Now() directly, instead of wrapping them in an interface/class first. For example here you could have a IClock interface with a GetLocalTime() method that your classes can use instead of using the system functions directly. This allows you to inject a SystemClock class at run-time and a MockClock during testing. By doing this you can gain full control of exactly what time is returned to your system/class under test. This principle obviously applies to all other static references that could return unpredictable results. I know it adds yet another thing you need to pass into your classes, but it at least makes that pre-existing dependency explicit and prevents the goal posts from continuously moving during testing (without having to resort to black magic, like MS Fakes).

8. This is a minor point, but your private properties here should really be fields

Answer 2

There is a difference between code that is "testable" and code that is loosely coupled.

The primary purpose of using DI is loose coupling. Testability is a side-benefit that is gained from loosely coupled code. But code that is testable isn't necessarily loosely coupled.

While injecting a service locator is obviously more loosely coupled than having a static reference to one, it is still not a best practice. The biggest drawback is lack of transparency of dependencies. You might save a few lines of code now by implementing a service locator and then think you are winning, but whatever is gained by doing so is lost when you actually have to compose your application. There is a distinct advantage to looking at the constructor in intellisense to determine what dependencies a class has then to locating the source code for that class to try to work out what dependencies it has.

So, as you might have guessed, I am recommending you use constructor injection. However, you also have an anti-pattern known as bastard injection in your example. The primary drawback of bastard injection is that you are tightly coupling your classes on each other by newing them up internally. This may seem innocent, but what would happen if you needed to move your services into separate libraries? There is a good chance that would cause circular dependencies in your application.

The best way to deal with this (especially when you are dealing with services and not configuration settings) is to either use pure DI or a DI container and just have a single constructor. You should also use a guard clause to ensure there is no way to create your order service without any dependencies.

class OrderService
{
   private readonly ICustomerService customerService;
   private readonly IInventoryService inventoryService;
   private readonly IShippingService shippingService;
   private readonly IOrderRepository repository;


   // Constructor used for injection (the one and only)
   public OrderService(
      ICustomerService customerService,
      IInventoryService inventoryService,
      IShippingService shippingService,
      IOrderRepository repository)
   {
        if (customerService == null)
            throw new ArgumentNullException("customerService");
        if (inventoryService == null)
            throw new ArgumentNullException("inventoryService");
        if (shippingService == null)
            throw new ArgumentNullException("shippingService");
        if (repository == null)
            throw new ArgumentNullException("repository");              

        this.customerService = customerService;
        this.inventoryService = inventoryService;
        this.shippingService = shippingService;
        this.repository = repository;
   }
}

// Within your unit test
[TestMethod]
public void TestSomething()
{
   OrderService orderService = new OrderService(
      new FakeCustomerService(),
      new FakeInventoryService(),
      new FakeShippingService(),
      new FakeOrderRepository());
}

// Within your application (pure DI)
public class OrderServiceContainer
{
    public OrderServiceContainer()
    {
        // NOTE: These classes may have dependencies which you need to set here.
        this.customerService = new CustomerService();
        this.inventoryService = new InventoryService();
        this.shippingService = new ShippingService();
        this.orderRepository = new OrderRepository();
    }

    private readonly IOrderService orderService;
    private readonly ICustomerService customerService;
    private readonly IInventoryServcie inventoryService;
    private readonly IShippingService shippingService;
    private readonly IOrderRepository orderRepository;

    public ResolveOrderService()
    {
        return new OrderService(
            this.customerService,
            this.inventoryService,
            this.shippingService,
            this.orderRepository);
    }
}

// In your application's composition root, resolve the object graph
var orderService = new OrderServiceContainer().ResolveOrderService();

I also agree with Gordon's answer. If you have 4 service dependencies it is a code smell that your class is taking on too many responsibilities. You should consider refactoring to aggregate services to make your classes singular in responsibility. Of course, 4 dependencies is sometimes necessary, but it is always worth taking a step back to see if there is a domain concept that should be another explicit service.

NOTE: I am not necessarily saying that Pure DI is the best approach, but it can work for some small applications. When an application becomes complex, using a DI container can pay dividends by using convention-based configuration.