DependencyInjection - Constructor over-injection smell vs service locator - where is the proper approach?

Question

I am trying to improve my MVC projects and I read a few articles about DI, IoC containers, Constructor Injection and Service locators. I wanted to go with Ninject to help with the dependencies, but got puzzled with some of the stuff I read.

The most controversial would be the widely discussed one here http://jeffreypalermo.com/blog/constructor-over-injection-anti-pattern (both parts).

And then some responses to it, like Mark Seemann's response, and a follow up

Jeffrey expreses my fears and addresses them in an incorrect manner - however, I am not sure what to do in my case.

For example, lets look at an MVC controller. It serves as a communication layer between an MCV App and a WebAPI. I'm a bit concenrned about the number of dependencies I would have when doing Constructor Injection.

public class OcrController : ConnectorControllerBase, IOcrController
{
    private readonly ILogger logger;
    private readonly IConnectorConfigurator config;
    private readonly HttpClient client;
    private readonly IOcrConnector connector;

    public OcrController(
            ILogger logger, 
            IConnectorConfigurator config, 
            HttpClient client, 
            IOcrConnector connector) 
    {
        this.logger = logger;
        this.config = config;
        this.client = client;
        this.connector = connector;
    }
}

While the IOcrConnector and HttpClient are clear depenendencies that I have no issues with, I am concerned about the other two. Depending on an action , it require an IConfiguration to read something from the config and similarily a Logger might be needed in some cases.

Same would be true for many of my classes - one or two actual dependencies, and the logger and config and maybe something else in the future.

At the moment I am using a simple and poor ServiceLocator, where instead of passing the ILogger and IConfig to constructor, I create them when needed:

protected ILogger Logger => this.logger 
    ?? (this.logger = ComponentFactory.GetLogger(this.GetType()));

protected IConnectorConfigurator Config => this.config 
    ?? (this.config = ComponentFactory.GetConfig());

The 'Factory' is very simple, because it simply returns my ILogger wrapper like below:

public static ILogger GetLogger(Type type)
{
    return new Log4NetLogger(type);
}

This factory lives in a separate assembly, so it seems to me it decouples the concrete ILogger from the actual projects sufficiently...?

I do not intend to be exchanging the ILogger or IConfig (which is now just a wrapper for built-in ConfigurationManager) at runtime or whatever, but when/if the need arises, I would just change the type returned from the GetLogger(Type type); method.

So, please let me know if that kind of mix of proper DI and ServiceLocator makes sense? Other alternative I think of would maybe be passing an IComponentFactory into the constructor (instead of having a globally accessed static?) But that is still a service locator, only placed elsewhere...

Please let me know what you think

Answer 1

In your linked example the blogger is arguing that the code should be refactored because the dependency 'is passed down the chain' rather than being used directly.

Its a 'Hobo parameter' code smell rather than anything to do with DI. He could fix it by injecting into Order instead of OrderProcessor

In your example your controller will presumably directly used ILogger? So its not the same issue.

Moveover MVC controllers will always require many service classes to be injected as they must necessarily be designed in a procedural way.

Consider that a Method call on your controller which takes an Order parameter must construct that object from the pure value data in the request via binding.

Because the Order input parameter is created by this pipeline you must inject the dependency into the controller or the binder.

Either way the pipeline must act as the dependency setup for the call in exactly the way the blogger objects to, because the Order object must be created from the incoming data rather than be passed in by reference

Answer 2

There is also a difference between injecting a fresh instance every time, or injecting the same instance again: the latter one is much faster.

That said, in the case of the logger, you can create one instance and inject it where ever it is required at virtually no time. The same may hold true for your configuration engine.

Answer 3

Passing in ILogger seems a bit overkill to me since using Log4Net or even one of the many logger wrappers allow you to get your logger very easily through the standard log factory:

private static ILog logger = 
    LogManager.GetLogger(System.Reflection.MethodBase.GetCurrentMethod().DeclaringType);

I don't see how using dependency injection really solves anything for logging. Even Microsoft's logging wrapper in .Net Standard allows for this type of logger access without sacrificing the ability to swap out the implementation with proper configuration.

Which really leaves just one: the IConnectorConfigurator. Personally, I would probably live with constructor injection until we prove it's a real problem. It sounds like the type of things that can be a singleton in your app, in which case the same instance would be provided to all the implementations.

You have to ask yourself:

• Am I addressing a measurable problem? (i.e. not able to meet performance requirements)
• Is the added complexity of the problem worth the complexity of the proposed solution?
• How will this affect the response time of fixing bugs in my app?

If you have a dozen or more constructor parameters, you probably should be rethinking the division of responsibilities in your components. In this case, I have a feeling that the proposed solution adds more complexity and surprise to maintainers than it solves. Don't get me wrong, there is a time and a place for deviating from the simplicity of constructor injection--I just think in this case it's probably overkill.