Constructor Over-injection is a code smell, not an anti-pattern.

Sometimes, people struggle with how to deal with the Constructor Over-injection code smell. Often, you can address it by refactoring to Facade Services. This is possible when constructor arguments fall in two or more natural clusters. Sometimes, however, that isn't possible.

Cross-cutting concerns #

Before we get to the heart of this post, I want to get something out of the way. Sometimes constructors have too many arguments because they request various services that represent cross-cutting concerns:

public Foo(
    IBar bar,
    IBaz baz,
    IQux qux,
    IAuthorizationManager authorizationManager,
    ILog log,
    ICache cache,
    ICircuitBreaker breaker)

This Foo class has seven dependencies passed via the constructor. Three of those (bar, baz, and qux) are regular dependencies. The other four are various incarnations of cross-cutting concerns: logging, caching, authorization, stability. As I describe in my book, cross-cutting concerns are better addressed with Decorators or the Chain of Responsibility pattern. Thus, such a Foo constructor ought really to take just three arguments:

public Foo(
    IBar bar,
    IBaz baz,
    IQux qux)

Making cross-cutting concerns 'disappear' like that could decrease the number of constructor arguments to an acceptable level, thereby effectively dealing with the Constructor Over-injection smell. Sometimes, however, that's not the issue.

No natural clusters #

Occasionally, a class has many dependencies, and the dependencies form no natural clusters:

public Ploeh(
    IBar bar,
    IBaz baz,
    IQux qux,
    IQuux quux,
    IQuuz quuz,
    ICorge corge,
    IGrault grault,
    IGarply garply,
    IWaldo waldo,
    IFred fred,
    IPlugh plugh,
    IXyzzy xyzzy,
    IThud thud)
{
    Bar = bar;
    Baz = baz;
    Qux = qux;
    Quux = quux;
    Quuz = quuz;
    Corge = corge;
    Grault = grault;
    Garply = garply;
    Waldo = waldo;
    Fred = fred;
    Plugh = plugh;
    Xyzzy = xyzzy;
    Thud = thud;
}

This seems to be an obvious case of the Constructor Over-injection code smell. If you can't refactor to Facade Services, then what other options do you have?

Introducing a Parameter Object isn't likely to help #

Some people, when they run into this type of situation, attempt to resolve it by introducing a Parameter Object. In its simplest form, the Parameter Object is just a collection of properties that client code can access. In other cases, the Parameter Object is a Facade over a DI Container. Sometimes, the Parameter Object is defined as an interface with read-only properties.

One way to use such a Parameter Object could be like this:

public Ploeh(DependencyCatalog catalog)
{
    Bar = catalog.Bar;
    Baz = catalog.Baz;
    Qux = catalog.Qux;
    Quux = catalog.Quux;
    Quuz = catalog.Quuz;
    Corge = catalog.Corge;
    Grault = catalog.Grault;
    Garply = catalog.Garply;
    Waldo = catalog.Waldo;
    Fred = catalog.Fred;
    Plugh = catalog.Plugh;
    Xyzzy = catalog.Xyzzy;
    Thud = catalog.Thud;
}

Alternatively, some people just store a reference to the Parameter Object and then access the read-only properties on an as-needed basis.

None of those alternatives are likely to help. One problem is that such a DependencyCatalog will be likely to violate the Interface Segregation Principle, unless you take great care to make a 'dependency catalogue' per class. For instance, you'd be tempted to add Wibble, Wobble, and Wubble properties to the above DependencyCatalog class, because some other Fnaah class needs those dependencies in addition to Bar, Fred, and Waldo.

Deodorant #

Fundamentally, Constructor Over-injection is a code smell. This means that it's an indication that something is not right; that there's an area of code that bears investigation. Code smells aren't problems in themselves, but rather symptoms.

Constructor Over-injection tends to be a symptom that a class is doing too much: that it's violating the Single Responsibility Principle. Reducing thirteen constructor arguments to a single Parameter Object doesn't address the underlying problem. It only applies deodorant to the smell.

Address, if you can, the underlying problem, and the symptom is likely to disappear as well.

Guidance, not law #

This is only guidance. There could be cases where it's genuinely valid to have dozens of dependencies. I'm being deliberately vague, because I don't wish to go into an elaborate example. Usually, there's more than one way to solve a particular problem, and occasionally, it's possible that collecting many services in a single class would be appropriate. (This sounds like a case for the Composite design pattern, but let's assume that there could be cases where that's not possible.)

This is similar to using cyclomatic complexity as a guide. Every now and then, a big, flat switch statement is just the best and most maintainable solution to a problem, even when it has a cyclomatic complexity of 37...

Likewise, there could be cases where it's genuinely not a problem with dozens of constructor arguments.

Summary #

Constructor Over-injection is a code smell, not an anti-pattern. It's a good idea to be aware of the smell, and address it when you discover it. You should, however, deal with the problem instead of applying deodorant to the smell. The underlying problem is usually that the class with the many dependencies has too many responsibilities. Address that problem, and the smell is likely to evaporate as well.


Comments

Dominik Jeske #

Hi Mark, I have question about “Parameter Object” approach you mentioned. In my previous project we was using this approach heavily. I know disadvantages you described and honestly I was against this approach but after some time I saw some advantages I want to discuss.

We have a lot of services that was reading data from some external databases, do some basic validation and fixing and than save to other database – so it was most integration code. Every of our services had bootstrapper attached by attribute that was describing dependencies. Each have only one parameter with so called “catalog” that was façade over DI with read only properties (sometimes even in tree structure to make it simpler to use like Catalog.Repositories.Server.SomeRepository). We have some base class for common dependencies like logging, common infrastructure services etc (we also use interceptor for cross cutting concerns but sometimes we have to log something explicitly).

In unit tests we used same bootstrapper that service was using with repositories changed to mocks. Advantage that I’m seeing is that when using lot of unit tests and service like this when we are maintain the code when something is changing in service we don’t had to touch any unit test – it is similar to using parameter object in general even outside the scope of constructor – when using single complex object as input parameter there is also no problem when on 100 unit test break after we change add some method parameter. It is service locator antipattern but only in one place – other code is not using DI.

Sometimes when I’m thinking about anti pattern approach in general I have a feeling similar to you when you was explaining other approach to async injection “you cannot do that” – “Oh, really”? Maybe pattern police will hunt me but is this really so bad?

2019-08-01 9:07 UTC

Dominik, thank you for writing. To be clear: there's no pattern police. Design patterns is (or was) an attempt to establish a vocabulary that can act as short-cuts for long, elaborate chains of arguments. The same goes for anti-patterns.

Most experienced programmers over time discover that certain ways of doing things yield more benefits than disadvantages (patterns) or yield more disadvantages than benefits (anti-patterns). Instead of having to explain the possible consequences every time we run into such situations, we can reach for a generalised description and refer to it by names such as Composite, Decorator, Service Locator, etc.

The same goes for more basic design principles such as the SOLID principles. If you find yourself writing code such as Catalog.Repositories.Server.SomeRepository, you're already in territory where the Law of Demeter (or the Occasionally Useful Suggestion of Demeter) applies. The Law of Demeter is less than a law and more like a code smell. If you find yourself 'dotting into' a deep object graph like that, you should stop and consider how that's going to impact long-term maintainability.

Either an object doesn't need everything that a service catalogue exposes, in which case it'd be at odds with the Interface Segregation Principle, or it does need all of those services, in which case the class in question most likely violates the Single Responsibility Principle.

You can address the first of these concerns by injecting only the services that a class needs. If you're concerned about the impact on unit tests of changing dependencies, consider using an Auto-mocking Container.

You can address the second of the above concerns by refactoring to Facade Services.

2019-08-03 10:17 UTC
Dominik Jeske #

Mark, thanks for detailed answer (as always).

In matter of catalog approach it was always smelly for me and honestly I was against using it but sometimes I had no good arguments to get rid of it. Automocking container is very promising for me – as matter of fact I saw this approach when analyzing one of your github samples and for start it looked like magic when xunit+autofixture+automoq was used and I have to admit it was love at first sight and I will absolutely deep dive into this topic.

When talking about pattern police I was some kind of joking. I understand the source and meaning of the patterns and it is very good that we have them but sometimes they are problematic. Many times some question / discussion on places like stuckoverflow is broken by some “police man” that is saying that “your approach is antipattern”. Even if this maybe is true it sometimes kills some new approaches because people are real believers and they believe in patterns as if they were some religion and they will be fight for it instead of discuss and assume that maybe it is not always true or maybe this case is not really antipattern.

Other thing is that people forgot after some time what is the real purpose / source of the pattern – the just use it automatically. My favorite anecdote/joke illustrate this

  • Mom why you are cutting edges of the cake
  • Becouse we should do this
  • But why
  • Becoue we have to
  • But why
  • Becouse… my mom did this
  • Grandma why you are cutting edges of the cake
  • Becouse… my mom did this
  • Great grandma why you are cutting edges of the cake
  • Becouse I have small stove
After your explanation I’m closer to see why this is antipattern but for understand thing like this for real it is no enough to say that this is antipattern but this ability is not given to all people 😊

2019-08-05 20:50 UTC


Wish to comment?

You can add a comment to this post by sending me a pull request. Alternatively, you can discuss this post on Twitter or somewhere else with a permalink. Ping me with the link, and I may respond.

Published

Monday, 27 August 2018 07:11:00 UTC

Tags



"Our team wholeheartedly endorses Mark. His expert service provides tremendous value."
Hire me!
Published: Monday, 27 August 2018 07:11:00 UTC