The Service Locator base class anti-pattern

Something I have seen a lot of over the last few years is a lot of service location, particularly in base classes, particularly in Controllers. But not limited to there, it can happen anywhere we see base classes.

Short rant about inheritance

The longer I do this, the more I find that base classes are not meant to be overused. I find that if we are pumping base classes full of code, we should probably be breaking them out into smaller classes, and including them in the derived versions that actually use that functionality. If it is a cross-cutting concern, probably using attributes would be a cleaner solution. Composition over Inheritance makes testing much simpler IMHO.

Short rant about service location

Containers have no place in Unit Tests. Let me clarify. The application’s IOC container has no place in the unit tests. (AutoFixture can be a helpful tool, but is also a smell). The fact that you use a service locator couples your code to that service locator, and now it is required in your tests to get things to run. Using a service locator hides from you, the consumer of that object, the dependencies that your object requires. If these are hidden in a base class or extension method, you have no way to discover that the dependency is there until your tests (or worst case, your application) comes crashing down around you.

</Rant>

The antipattern

So the antipattern goes like this: We have N classes, with N types of functionality. They derive from a base class, which contains many services that are used across all N derived classes. These services are Service Located.

public class BaseController : Controller
{
    public ICustomerService CustomerService
    {
        get
        {
            return Service.Locate<ICustomerService>();
        }
    }
    
    public IOrderService OrderService
    {
        get
        {
            return Service.Locate<IOrderService>();
        }
    }
}

public class OrderController : BaseController 
{
    public Order Get(int orderId)
    {
        return OrderService.GetOrderById(orderId);
    }
}

public class CustomerController : BaseController 
{
    public Customer Get(int customerId)
    {
        return CustomerService.GetOrderById(orderId);
    }
}

This is a representative sample of services and controllers. You can imagine that the real number of services in the base class could be anywhere between 5 and 20 services, or more, and that there could be 5 to 20, or more, controllers as well.

Also, I have intentionally used Service.Locate because there are many ways to do this approach. You could use some generic Global Static Method or some container specific Global Static variable. It doesn’t matter which, both are just as bad as each other.

So Why is this so bad? Well, when It comes to testing, your Controller now has hidden dependencies. We can no longer tell from the constructor what it requires to be provided to get work done. But they aren’t required right, it is able to resolve them itself? No, not really.

There is a dependency on the Service Locator class Service that has to be configured just right for the actual dependencies to show up. And what’s worse than hidden dependencies? Singleton dependencies that break your ability to run your tests in parallel. If you try and run two tests against two controller instances (or any other type that shares the singleton dependency) then your test may periodically fail, from a race condition with the other test. It is never fun to have false negatives in your test suite, you lose all trust and won’t see a true negative as a real problem.

(Incorrect) Solution 1

Ok, so we see the problems, how should we solve them? Let’s try fixing the singleton problem with dependency injection through the constructor:

public class BaseController : Controller
{
    private ICustomerService _customerService;
    public ICustomerService CustomerService
    {
        get
        {
            return _customerService;
        }
    }
    
    private IOrderService _orderService;
    public IOrderService OrderService
    {
        get
        {
            return _orderService;
        }
    }
    
    protected BaseController(
        ICustomerService customerService,
        IOrderService orderService)
    {
        _customerService = customerService;
        _orderService = orderService;
    }
}

public class OrderController : BaseController 
{
    public OrderController(
        ICustomerService customerService,
        IOrderService orderService)
        : base(customerService, orderService)
    {
    }
    
    public Order Get(int orderId)
    {
        return OrderService.GetOrderById(orderId);
    }
}

public class CustomerController : BaseController 
{
    public CustomerController(
        ICustomerService customerService,
        IOrderService orderService)
        : base(customerService, orderService)
    {
    }
    
    public Customer Get(int customerId)
    {
        return CustomerService.GetOrderById(orderId);
    }
}

By my rough estimate, we doubled the code, but we did expose our dependencies through the constructor right? Again, not really.

If we take a closer look, we have a dependency on the BaseController for the IOrderService, but we never actually use it on the derived CustomerController. With more and more services loaded up on the base class, the worse this gets. The more likely we are that more of the dependencies the base class requires are not used by any particular controller.

If you are using a DI Container, you won’t feel the pain of creating one of these either, but the pain is still there. This is one of my Pet Peeves with Containers, they hide a lot of the pain from bad usages that only get worse over time, instead of annoying you enough to fix the problem fast.

And what if we need to add a new dependency? Well, if we add it to the base class, we have to add it to all (5, or maybe 20) Controllers’ constructors so that it will compile. You might not see this as an issue, you just add it to the one that uses the dependency right? Not so. The base class has become a gravity well, all services go there, whether needed by all, or not. It’s only logical that the next person along will put their new service there as well. If there is code in the base class, it may be the only place to add it to surface the functionality to that code.

This brings me to the next reason base class dependencies are bad. Not every derived class will use it. So why do they all have it available at all, in the first instance, and have a dependency in their constructor for it in the second instance, if it is not actually a dependency?

This leads into a huge con for this approach over the previous one: Over-eager loading. At least with the service locator, the instance and all of its dependencies are only loaded when you ask for the service. With this approach, all dependencies are loaded before they are injected in, which could add a large amount of overhead to creating your controller. And like we said, not every instance is actually even used in this controller at all! No, don’t do this approach if you can help it. (by that I usually mean as a stop-gap solution to a larger refactoring effort. See Reality.)

The way things should have been

Let’s try to implement this solution again, with all of this in mind. We want our classes to only take dependencies on what they actually use:

public class OrderController : Controller 
{
    private IOrderService _orderService;

    public OrderController(IOrderService orderService)
    {
        _orderService = orderService;
    }
    
    public Order Get(int orderId)
    {
        return _orderService.GetOrderById(orderId);
    }
}

public class CustomerController : Controller 
{
    private ICustomerService _customerService;

    public CustomerController(ICustomerService customerService)
    {
         _customerService = customerService;
    }
    
    public Customer Get(int customerId)
    {
        return _customerService.GetOrderById(orderId);
    }
}

We can’t get away from inheritance completely because of MVC, but we now have one less class to worry about. And in this trivial example, we have the same amount of code. Of course, that bit doesn’t necessarily scale, but that’s not a problem, really. Have we fixed all our problems? I think so.

Our controller now takes only the dependencies it needs. We don’t need to pass so many things to any particular controller, and It is clear what it actually depends on. In reality, we might have had to pass the same services to multiple controllers, but that’s what composition is all about.

We no longer have hidden dependencies. Everything a controller needs is passed in. Much easier for testing. You can even run your tests concurrently without the shared static singleton service locator.

Now since we got rid of the base class, what about any cross-cutting concerns or common code? Well, it should now become clear that some of that code might need to be a dependency passed into the subset of all controllers that actually use it. And it that code still needs context-specific knowledge that you can’t or don’t want to pass in? I suggest looking into Aspect Oriented approaches such as the MVC FilterAttribute.

But we still have far too many dependencies per controller with this approach right? Well, that depends. I’ll hopefully cover this off from the next two sections, but at a high level, you may have mixed concerns in your controller if it still requires many different services. Or perhaps you need to encapsulate some of your business logic out into a new service instead of having it in amongst your MVC logic. The other option, which is hard to achieve in the existing MVC framework but worth mentioning, is the idea of service injection into methods, as discussed under advanced topics.

Reality

In reality, this type of code is probably already in your legacy applications, and It isn’t simple to unravel them. It could take several attempts to transform your code to resemble this type of approach.

If you do want to tackle this type of refactoring, you would first want to pick one controller, and transform that one first. This will require code duplication to move your dependencies in, but you should find that not all of them need to be copied over.

As you refactor out code into attributes and new service classes, think about adding usages from your base class and other controllers at the same time, if the change is small enough to do so.

Another real-world solution to the Service Location problem, especially for testing is to pass these dependencies in a second constructor and use service location from the default constructor instead.

public class BaseController : Controller
{
    ...
    
    protected BaseController()
    : this(Service.Locate<ICustomerService>(),Service.Locate<IOrderService>())
    {
    }
    
    protected BaseController(
        ICustomerService customerService,
        IOrderService orderService)
    {
        _customerService = customerService;
        _orderService = orderService;
    }
}

public class OrderController : BaseController 
{
    public OrderController(
        ICustomerService customerService,
        IOrderService orderService)
        : base(customerService, orderService)
    {
    }
    
    public OrderController() : base()
    {
    }
    
    ...
}

This approach allows you to fix your concurrent testing issues, even if you can’t fix all of your problems straight away.

If you can’t get rid of the base class at all, look for candidates that are only used in some places, and pull these down, so at least new dependencies added will see this approach, and you have a new gravity well approach for others to follow.

Advanced Topics

I said I would mention is service injection into methods. This is something that containers won’t do for you, but there are approaches available in other frameworks and libraries that could allow you to follow a similar pattern. Worst-case an Adapter could be used to achieve this as another layer in your MVC, or use Lazy objects to at least simulate the run-time benefits this creates.

The implementation of the method looks as follows:


public class CustomerController : Controller 
{   
    public Customer Get(int customerId, ICustomerService customerService)
    {
        return CustomerService.GetOrderById(orderId);
    }
}

As well as our parameters, we pass the services along here as well. This way, our controller method only requires the services it really uses, not all the services for all methods in this controller. As I said, you should only have services that most of your actions need anyway, of you have a solid single responsibility in this controller.

The way to simulate some of these benefits, as mentioned before, would look something like this:

public class CustomerController : Controller 
{
    private Lazy<ICustomerService> _customerService;

    public CustomerController(Lazy<ICustomerService> customerService)
    {
         _customerService = customerService;
    }
    
    public Customer Get(int customerId)
    {
        return _customerService.Value.GetOrderById(orderId);
    }
}

Given we have a container, we now we only actually create a service and all of its dependencies in the method where we actually use it, and not anywhere else. Slight gain, but if you are trying to tidy up your controllers, this little trick could come in useful.

Parting words

If you’ve made it this far, thanks for sticking with me, I know it has been a long one. What might render this discussion moot, or highly important is this small piece of advice:

Separate your rendering/routing/MVC style concerns from your implementation details.

Try to keep your controllers void of logic and stick with transforming user data to a payload, passing the payload to a service, and rendering the results from the service. Authorisation and validation might fit better here as well, but not your actual business logic. This will serve you well for simplifying testing, and less fighting with the framework to make it handle injection, dependencies, inheritance and HTTP concerns within your business rules.

There are no silver bullets, but if your system is complex, this approach does make testing easier, overall.

I would like to code up a bigger solution showing all three approaches to stick up on GitHub. We will see if time permits later in the month.

Update

The mentioned GitHub solution is now available: https://github.com/csMacnzBlog/BaseClassAntiPattern