13

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.

  • 8
    This is called Service Locator and I don't like it. There has been plenty of writing on the subject - see martinfowler.com/articles/injection.html and blog.ploeh.dk/2010/02/03/ServiceLocatorisanAnti-Pattern for a starter. – Benjamin Hodgson Jan 11 '15 at 11:44
  • From the Martin Fowler article: "I've often heard the complaint that these kinds of service locators are a bad thing because they aren't testable because you can't substitute implementations for them. Certainly you can design them badly to get into this kind of trouble, but you don't have to. In this case the service locator instance is just a simple data holder. I can easily create the locator with test implementations of my services." Could you explain why you don't like it? Maybe in an answer? – LiverpoolsNumber9 Jan 11 '15 at 12:00
  • 8
    He is right, this is bad design. It's easy: public SomeClass(Context c). This code is quite clear, isn't it? It states, that SomeClass depends on a Context. Err, but wait, it does not! It only relies on dependency X it gets from Context. That means, every time you make a change to Context it could break SomeObject, even though you only changed Contexts Y. But yeah, you know that you only changed Y not X, so SomeClass is fine. But writing good code is not about what you knows but what the new employee knows when he looks at your code the first time. – valenterry Jan 11 '15 at 14:00
  • @DocBrown To me that is exactly what I said - I don't see the difference here. Can you please explain further? – valenterry Jan 11 '15 at 14:26
  • 1
    @DocBrown I see your point now. Yes, if his Context is just a bundle of all the dependencies, then this is not bad design. It may be however bad naming, but that is also just an assumption. OP should clarify if there are more methods (inner objects) of the context. Also, discussing code is fine, but this is programmers.stackexchange so to me we should also try to see "behind" the things to make the OP improve. – valenterry Jan 11 '15 at 15:26
5

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.

  • I agree with this. The problem with the example is the generic GetService method. Refactoring to explicitly named and typed methods is better. Better still would be to spell out the dependencies of the IContext implementation explicitly in the constructor. – Benjamin Hodgson Jan 11 '15 at 13:40
  • 1
    @BenjaminHodgson: it may be sometimes better, but it is not always better. You note the code smell when the ctor parameter list gets longer and longer. See my former answer here: programmers.stackexchange.com/questions/190120/… – Doc Brown Jan 12 '15 at 16:51
  • @DocBrown the "code smell" of constructor over-injection is indicative of a violation of the SRP, which is the real problem. Simply wrapping up several services into a Facade class, only to expose them as properties, does nothing to address the source of the issue. Thus a Facade should not be a simple wrapper around other components, but should ideally offer a simplified API by which to consume them (or should simplify them in some other way)... – AlexFoxGill Jan 15 '15 at 9:37
  • ... Remember that a "code smell" like constructor over-injection is not a problem in itself. It is just a hint that there is a deeper problem in the code that can usually be solved by refactoring sensibly once the problem has been identified – AlexFoxGill Jan 15 '15 at 9:39
  • Where were you when Microsoft baked this into IValidatableObject? – RubberDuck Apr 12 '16 at 1:46
15

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).

  • 1
    You are complaning about the Service Locator pattern (which I agree to). But actually, in the OP's example, MyControllerBase is neither coupled to a specific DI container, nor is this "really" an example for the Service Locator anti-pattern. – Doc Brown Jan 11 '15 at 12:43
  • @DocBrown I agree. Not because it makes my life easier, but because most of the examples given above are not relevant to my code. – LiverpoolsNumber9 Jan 11 '15 at 13:01
  • 2
    To me, the hallmark of the Service Locator anti-pattern is the generic GetService<T> method. Resolving arbitrary dependencies is the real smell, which is present and correct in the OP's example. – Benjamin Hodgson Jan 11 '15 at 14:01
  • 1
    Another problem with using a Service Locator is that it reduces flexibility: you can only have one implementation of each service interface. If you build two classes that rely on an IFrobnicator, but later decide that one should use your original DefaultFrobnicator implementation, but the other really ought to be using a CacheingFrobnicator decorator around it, you have to change the existing code, whereas if you're injecting dependencies directly all you have to do is change your setup code (or config file if you're using a DI framework). Thus this is an OCP violation. – Jules Jan 11 '15 at 14:15
  • 1
    @DocBrown The GetService<T>() method permits arbitrary classes to be requested: "What this method does is look at the current DI container of the application and attempts to resolve the dependency. Fairly standard I think.". I was responding to your comment at the top of this answer. This is 100% a Service Locator – AlexFoxGill Jan 15 '15 at 10:53
5

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.

  • +1 - this is a new take on the question which none of the other answers have really addressed – Benjamin Hodgson Jan 14 '15 at 14:36
  • 1
    "One of the wonderful benefits of Constructor Injection is that it makes violations of the Single Responsibility Principle glaringly obvious" Love that. Really good answer. Don't agree with all of it. My use case is, funnily enough, not having to duplicate code in a system that will have over 100 controllers (at least). However re SRP - each injected service has a single responsibility, as does the controller which uses them all - how would one refractor that??! – LiverpoolsNumber9 Jan 15 '15 at 10:05
  • 1
    @LiverpoolsNumber9 The key is to move functionality out from BaseController into dependencies, until you have nothing left in BaseController except protected one-liner delegations to the new components. Then you can lose BaseController and replace those protected methods with direct calls to dependencies - this will simplify your model and make all your dependencies explicit (which is a very good thing in a project with 100s of controllers!) – AlexFoxGill Jan 15 '15 at 10:59
  • @LiverpoolsNumber9 - if you could copy a portion of your BaseController to a pastebin I could make some concrete suggestions – AlexFoxGill Jan 15 '15 at 11:00
-2

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.

  • 4
    I disagree that you have a Service Facade here. The GetService<T>() method is able to (attempt to) resolve arbitrary dependencies. This makes it a Service Locator, not a Service Facade, as I explained in the footnote of my answer. If you were to replace it with a small set of GetServiceX()/GetServiceY() methods, as @DocBrown suggests, then it would be a Facade. – Benjamin Hodgson Jan 14 '15 at 14:09
  • 2
    You should read, and pay close attention to, the last section of this article about the Abstract Service Locator, which is essentially what you are doing. I have seen this anti-pattern corrupt an entire project - pay particularly close attention to the quote "it will make your life as a maintenance developer worse because you will need to use considerable amounts of brain power to grasp the implications of every change you make" – AlexFoxGill Jan 14 '15 at 14:10
  • 3
    On static typing - you're missing the point. If I try to write code which doesn't provide a class using DI with a constructor paramter that it needs, it won't compile. That's static, compile-time safety against problems. If you write your code, providing your constructor with an IContext that hasn't been correctly configured to provide the argument it actually needs, then it'll compile and fail at run time – Ben Aaronson Jan 14 '15 at 16:26
  • 3
    @LiverpoolsNumber9 Well then why have a strongly-typed language at all? Compiler errors are a first line of defense against bugs. Unit tests are the second. Yours wouldn't be picked up by unit tests either because it's an interaction between a class and its dependency, so you'd be into the third line of defense: integration tests. I don't know how often you run those, but you're now talking about feedback on the order of minutes or more, rather than the milliseconds it takes your IDE to underline a compiler error. – Ben Aaronson Jan 15 '15 at 10:41
  • 2
    @LiverpoolsNumber9 wrong: your tests now have to populate a IContext and inject that, and crucially: if you add a new dependency, the code will still compile - even though the tests will fail at runtime – AlexFoxGill Jan 15 '15 at 11:15

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.