This way I'm writing this code is testable, but is there something wrong with it I'm missing?

Question

I have an interface called IContext. For the purposes of this it doesn't really matter what's it does except the following:

T GetService<T>();

What this method does is look at the current DI container of the application and attempts to resolve the dependency. Fairly standard I think.

In my ASP.NET MVC application, my constructor looks like this.

protected MyControllerBase(IContext ctx)
{
    TheContext = ctx;
    SomeService = ctx.GetService<ISomeService>();
    AnotherService = ctx.GetService<IAnotherService>();
}

So rather than adding multiple parameters in the constructor for each service (because this will get really annoying and time-consuming for the developers extending the application) I am using this method to get services.

Now, it feels wrong. However, the way I'm currently justifying it in my head is this - I can mock it.

I can. It wouldn't be difficult to mock up IContext to test the Controller. I'd have to anyway:

public class MyMockContext : IContext
{
    public T GetService<T>()
    {
        if (typeof(T) == typeof(ISomeService))
        {
            // return another mock, or concrete etc etc
        }

        // etc etc
    }
}

But as I said, it feels wrong. Any thoughts / abuse welcome.

Answer 1

This design is known as Service Locator* and I don't like it. There are lots of arguments against it:

Service Locator couples you to your container. Using regular dependency injection (where the constructor spells out the dependencies explicitly) you can straightforwardly replace your container with a different one, or go back to new-expressions. With your IContext that's not really possible.

Service Locator hides dependencies. As a client, it's very difficult to tell what you need to construct an instance of a class. You need some sort of IContext, but you also need to set the context up to return the correct objects in order to make the MyControllerBase work. This is not at all obvious from the signature of the constructor. With regular DI the compiler tells you exactly what you need. If your class has a lot of dependencies, you should feel that pain because it will spur you to refactor. Service Locator hides the problems with bad designs.

Service Locator causes run-time errors. If you call GetService with a bad type parameter you'll get an exception. In other words, your GetService function is not a total function. (Total functions are an idea from the FP world, but it basically means that functions should always return a value.) Better to let the compiler help and tell you when you've got the dependencies wrong.

Service Locator violates the Liskov Substitution Principle. Because its behaviour varies based on the type argument, Service Locator can be viewed as if it effectively has an infinite number of methods on the interface! This argument is spelled out in detail here.

Service Locator is hard to test. You've given an example of a fake IContext for tests, which is fine, but surely it's better not to have to write that code in the first place. Just inject your fake dependencies directly, without going via your service locator.

In short, just don't do it. It seems like a seductive solution to the problem of classes with lots of dependencies, but in the long run you're just going to make your life miserable.

_{* I'm defining Service Locator as an object with a generic Resolve<T> method which is capable of resolving arbitrary dependencies and is used throughout the codebase (not just at the Composition Root). This is not the same as Service Facade (an object which bundles up some small known set of dependencies) or Abstract Factory (an object which creates instances of a single type - the type of an Abstract Factory may be generic but the method is not).}

Answer 2

Having one instead of many parameters in the constructor is not the problematic part of this design. As long as your IContext class is nothing but a service facade, specificially for providing the dependencies used in MyControllerBase, and not a general service locator used throughout your whole code, that part of your code is IMHO ok.

Your first example might be changed to

protected MyControllerBase(IContext ctx)
{
    TheContext = ctx;
    SomeService = ctx.GetSomeService();
    AnotherService = ctx.GetAnotherService();
}

that would not be a substantial design change of MyControllerBase. If this design is good or bad depends only on the fact if your want to

• make sure TheContext, SomeService and AnotherService are always all initialized with mock objects, or all of them with real objects
• or, to allow initialization them with different combinations of the 3 objects (which means, for this case your would need to pass the parameters individually)

So using only one parameter instead of 3 in the constructor can be fully reasonable.

The thing which is problematic is IContext exposing the GetService method in public. IMHO you should avoid this, instead keep the "factory methods" explicit. So will it be ok to implement the GetSomeService and GetAnotherService methods from my example using a service locator? IMHO that depends. As long the IContext class keeps just beeing a simple abstract factory for the specific purpose of providing an explicit list of service objects, that is IMHO acceptable. Abstract factories are typically just "glue" code, which don't have to be unit-tested itself. Nethertheless you should ask yourself if in the context of methods like GetSomeService, if you really need a service locator, or if an explicit constructor call would not be simpler.

So beware, when you stick to a design where the IContext implementation is just a wrapper around a public, generic GetService method, allowing to resolve any arbitrary depencencies by arbitrary classes, then everything applies what @BenjaminHodgson wrote in his answer.

Answer 3

The best arguments against the Service Locator anti-pattern are plainly stated by Mark Seemann so I won't go too much into why this is a bad idea - it is a learning journey that you have to take the time to understand for yourself (I also recommend Mark's book).

OK so to answer the question - let's re-state your actual problem:

So rather than adding multiple parameters in the constructor for each service (because this will get really annoying and time-consuming for the developers extending the application) I am using this method to get services.

There is a question that addresses this issue on StackOverflow. As mentioned in one of the comments there:

The best remark: "One of the wonderful benefits of Constructor Injection is that it makes violations of the Single Responsibility Principle glaringly obvious."

You're looking in the wrong place for the solution to your problem. It is important to know when a class is doing too much. In your case I strongly suspect that there is no need for a "Base Controller". In fact, in OOP there is almost always no need for inheritance at all. Variations in behaviour and shared functionality can be achieved entirely through appropriate use of interfaces, which usually results in better factored and encapsulated code - and no need to pass dependencies to superclass constructors.

In all of the projects I have worked on where there is a Base Controller, it was done purely for the purposes of sharing convenient properties and methods, such as IsUserLoggedIn() and GetCurrentUserId(). STOP. This is horrible misuse of inheritance. Instead, create a component that exposes these methods and take a dependency on it where you need it. This way, your components will remain testable and their dependencies will be obvious.

Aside from anything else, when using the MVC pattern I would always recommend skinny controllers. You can read more about this here but the essence of the pattern is simple, that controllers in MVC should do one thing only: handle arguments passed by the MVC framework, delegating other concerns to some other component. This again is the Single Responsibility Principle at work.

It really would help to know your use case to make a more accurate judgment, but honestly I can't think of any scenario where a base class is preferable to well-factored dependencies.

Answer 4

I am adding an answer to this based on everybody else's contributions. So many thanks everbody. First here's my answer: "No, there's nothing wrong with it".

Doc Brown's "Service Facade" answer I accepted this answer because what I was looking for (if the answer was "no") was some examples or some expansion upon what I was doing. He provided this in suggesting that A) it's got a name, and B) there are probably better ways to do this.

Benjamin Hodgson's "Service Locator" answer As much as I appreciate the knowledge I've gained here, what I have is not a "Service Locator". It is a "Service Facade". Everything in this answer is correct but not for my circumstances.

USR's answers

I'll tackle this in more detail:

You're giving up a lot of static information this way. You're deferring decisions to runtime like many dynamic languages do. That way you loose static verification (safety), documentation and tooling support (auto-complete, refactorings, find usages, data flow).

I don't lose any tooling and I'm not losing any "static" typing. The service facade will return what I've configured in my DI container, or default(T). And what it returns is "typed". The reflection is encapsulated.

I don't see why adding additional services as constructor arguments is a big burden.

It's certainly not "rare". As I'm am using a base controller, each time I need to change it's constructor, I may have to change 10, 100, 1000 other controllers.

If you use a dependency injection framework you will not even have to manually pass in the parameter values. Then again you loose some static advantages but not as many.

I am using dependency injection. That's the point.

And finally, Jules' comment on Benjamin's answer I am not losing any flexibility. It's my service facade. I can add as many parameters to GetService<T> as I want to to distinguish between different implementations, just as one would do when configuring a DI container. So for example, I could change GetService<T>() to GetService<T>(string extraInfo = null) to get around this "potential problem".

---

Anyway, thanks again to everybody. It's been really useful. Cheers.