Jeffrey Palermo recently posted a blog post titled Constructor over-injection anti-pattern - go read his post first if you want to be able to follow my arguments.

His point seems to be that Constructor Injection can be an anti-pattern if applied too much, particularly if a consumer doesn't need a particular dependency in the majority of cases.

The problem is illustrated in this little code snippet:

bool isValid = _validator.Validate(order);  
if (isValid) 
{
    _shipper.Ship(order);
}

If the Validate method returns false often, the shipper dependency is never needed.

This, he argues, can lead to inefficiencies if the dependency is costly to create. It's not a good thing to require a costly dependency if you are not going to use it in a lot of cases.

That sounds like a reasonable statement, but is it really? And is the proposed solution a good solution?

No, this isn't a reasonable statement, and the proposed solution isn't a good solution.

It would seem like there's a problem with Constructor Injection, but in reality the problem is that it is being used incorrectly and in too constrained a way.

The proposed solution is problematic because it involves tightly coupling the code to OrderShipperFactory. This is more or less a specialized application of the Service Locator anti-pattern.

Consumers of OrderProcessor have no static type information to warn them that they need to configure the OrderShipperFactory.CreationClosure static member - a completely unrelated type. This may technically work, but creates a very developer-unfriendly API. IntelliSense isn't going to be of much help here, because when you want to create an instance of OrderProcessor, it's not going to remind you that you need to statically configure OrderShipperFactory first. Enter lots of run-time exceptions.

Another issue is that he allows a concrete implementation of an interface to change the design of the OrderProcessor class - that's hardly in the spirit of the Liskov Substitution Principle. I consider this a strong design smell.

One of the commenters (Alwin) suggests instead injecting an IOrderShipperFactory. While this is a better option, it still suffers from letting a concrete implementation influence the design, but there's a better solution.

First of all we should realize that the whole case is a bit construed because although the IOrderShipper implementation may be expensive to create, there's no need to create a new instance for every OrderProcessor. Instead, we can use the so-called Singleton lifetime style where we share or reuse a single IOrderShipper instance between multiple OrderProcessor instances.

The beauty of this is that we can wait making that decision until we wire up the actual dependencies. If we have implementations of IOrderShipper that are inexpensive to create, we may still decide to create a new instance every time.

There may still be a corner case where a shared instance doesn't work for a particular implementation (perhaps because it's not thread-safe). In such cases, we can use Lazy loading to create a LazyOrderShipper like this (for clarity I've omitted making this implementation thread-safe, but that would be trivial to do):

public class LazyOrderShipper : IOrderShipper
{
    private OrderShipper shipper;
 
    #region IOrderShipper Members
 
    public void Ship(Order order)
    {
        if (this.shipper == null)
        {
            this.shipper = new OrderShipper();
        }
        this.shipper.Ship(order);
    }
 
    #endregion
}

Notice that this implementation of IOrderShipper only creates the expensive OrderShipper instance when it needs it.

Instead of directly injecting the expensive OrderShipper instance directly into OrderProcessor, we wrap it in the LazyOrderShipper class and inject that instead. The following test proves the point:

[TestMethod]
public void OrderProcessorIsFast()
{
    // Fixture setup
    var stopwatch = new Stopwatch();
    stopwatch.Start();
 
    var order = new Order();
 
    var validator = new Mock<IOrderValidator>();
    validator.Setup(v => 
        v.Validate(order)).Returns(false);
 
    var shipper = new LazyOrderShipper();
 
    var sut = new OrderProcessor(validator.Object,
        shipper);
    // Exercise system
    sut.Process(order);
    // Verify outcome
    stopwatch.Stop();
    Assert.IsTrue(stopwatch.Elapsed < 
        TimeSpan.FromMilliseconds(777));
    Console.WriteLine(stopwatch.Elapsed);
    // Teardown
}

This test is significantly faster than 777 milliseconds because the OrderShipper never comes into play. In fact, the stopwatch instance reports that the elapsed time was around 3 ms!

The bottom line is that Constructor Injection is not an anti-pattern. On the contrary, it is the most powerful DI pattern available, and you should think twice before deviating from it.


Comments

Thanks for publishing this response. When I read the post yesterday evening, I had a lot of thoughts I was chewing on, and you tackled most of them. I couldn't tell if the "smell" was that the dependency was only sometimes needed, or that the dependency was only sometimes needed, and was expensive to create.

It seemed like the need to create a set of factories and have them make calls into the IOC container directly was a bigger smell. And the fact that the OrderShipper would be comming from a factory setup somewhere else, making code maintenance more difficult, and the application more prone to errors.

Decorating the implementation with a lazy load implementation is very clever. I hadn't thought of that solution, but the singleton lifecycle had.

Well done.
2010-01-20 16:50 UTC
I like this approach, but you've also made a contrived example. Your LazyOrderShipper directly instantiates an OrderShipper. What about the dependencies that OrderShipper might require? What if those dependencies are costly? Is it lazy turtles all the way down?
2010-01-20 17:14 UTC
Constructor over-injection is the problem. I rather like constructor injection
2010-01-20 17:20 UTC
Thank you all for your comments.

The point raised by Phil Haack is valid, so I've addressed it in a second blog post.
2010-01-20 18:09 UTC
SP
Maybe I'm missing something, but why isn't LazyOrderShipper thread safe as it stands? Your shipper member variable isn't static.
2010-01-20 18:44 UTC
Why isn't LazyOrderShipper thread-safe as it is?

When I wrote that, my thinking was that if the Ship method is hit by two or more threads simultaneously, more than one thread may evaluate the member field to null and start creating a new instance of OrderShipper. At least one of those instance will created, but never permanently assigned, since another thread may overwrite the value.

In this case, it may not lead to any exceptions (that I can think of), but it's inefficient use of resources.

However, every time I post something about threading on this blog, someone will point out my errors, so I'm looking forward to learn which mistake I made this time around :)

I'm no threading expert, in case you were wondering :)
2010-01-20 19:00 UTC
SP
As yes, your right -- I was thinking the calling code could lock against the object itself, but you would never know when LazyOrderShipper was writing to the property. (I'm not threading guru by any means, either).

Ok, back to DI... :-)
2010-01-20 19:35 UTC
Thomas
Great post as well as your book
2010-01-20 20:21 UTC
This would be a good solution if the problem wasn't so contrived. I've never run into a scenario where it would take me seven seconds to instantiate an object. It might take me 7 seconds to execute a method, but never instantiate the object itself.

My point is: It is like arguing that my unicorn needs a better saddle, and that saddle will let me ride it better.

I commend you anyways for your effort.
2010-01-22 21:52 UTC
I agree, but I just couldn't let the claim that Constructor Injection is an anti-pattern go by without reacting.
2010-01-22 23:56 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 Google Plus, or somewhere else with a permalink. Ping me with the link, and I may add it as a comment.

Published

Wednesday, 20 January 2010 16:28:03 UTC

Tags



"Our team wholeheartedly endorses Mark. His expert service provides tremendous value."
Hire me!