Constructor over-injection anti-pattern

Check out http://jeffreypalermo.com/blog/constructor-over-injection-smell-ndash-follow-up/ for a follow-up

I’m interested in structure. We hear lots of talk about convention over configuration. I’m all for structure over convention (over configuration). This post is about laying out some code that is an anti-pattern. It uses 100% constructor injection. This code makes it easy for a container to bootstrap the construction of the objects, but let’s take a look at why this specific scenario is an anti-pattern and what design would be better.

Example

I have an order processing application that gets 10 orders at a time and tries to process them to shipping. There are some validation steps, and then there is some work to ship the order. Here is our Main() method:

   1:  public static void Main()

   2:  {

   3:      DateTime startTime = DateTime.Now;

   4:      Console.WriteLine("Begin: " + startTime.TimeOfDay);

   5:      IEnumerable<Order> orders = GetNextTenOrders();

6:

   7:      foreach (var order in orders)

   8:      {

   9:          IOrderProcessor processor =

  10:              ObjectFactory.GetInstance<IOrderProcessor>();

  11:          SuccessResult successResult = processor.Process(order);

  12:          if (successResult == SuccessResult.Success)

  13:          {

  14:              RecordSuccess(order);

  15:              continue;

  16:          }

17:

  18:          ReportFailure(order);

  19:      }

  20:      DateTime endTime = DateTime.Now;

  21:      Console.WriteLine("End:   " + endTime.TimeOfDay);

  22:      Console.WriteLine("Total time: " + endTime.Subtract(startTime));

  23:  }

.csharpcode, .csharpcode pre
{
font-size: small;
color: black;
font-family: consolas, “Courier New”, courier, monospace;
background-color: #ffffff;
/*white-space: pre;*/
}
.csharpcode pre { margin: 0em; }
.csharpcode .rem { color: #008000; }
.csharpcode .kwrd { color: #0000ff; }
.csharpcode .str { color: #006080; }
.csharpcode .op { color: #0000c0; }
.csharpcode .preproc { color: #cc6633; }
.csharpcode .asp { background-color: #ffff00; }
.csharpcode .html { color: #800000; }
.csharpcode .attr { color: #ff0000; }
.csharpcode .alt
{
background-color: #f4f4f4;
width: 100%;
margin: 0em;
}
.csharpcode .lnum { color: #606060; }

Let’s just run it:

Hmm. This little example code took over 7 seconds to run. Given that I’ve stubbed out all implementations, it should just breeze through. Here is the code of OrderProcessor, which is the class that is wired up to IOrderProcessor.

   1:  public class OrderProcessor : IOrderProcessor

   2:  {

   3:      private readonly IOrderValidator _validator;

   4:      private readonly IOrderShipper _shipper;

5:

   6:      public OrderProcessor(IOrderValidator validator, IOrderShipper shipper)

   7:      {

   8:          _validator = validator;

   9:          _shipper = shipper;

  10:      }

11:

  12:      public SuccessResult Process(Order order)

  13:      {

  14:          bool isValid = _validator.Validate(order);

  15:          if (isValid)

  16:          {

  17:              _shipper.Ship(order);

  18:          }

19:

  20:          return CreateStatus(isValid);

  21:      }

22:

  23:      private SuccessResult CreateStatus(bool isValid)

  24:      {

  25:          return isValid ? SuccessResult.Success : SuccessResult.Failed;

  26:      }

  27:  }

Ok. Standard constructor injection. The anti-pattern is that one of these constructor arguments is really not a class dependency. The problem is that the constructor of OrderProcessor is too greedy. It doesn’t need IOrderShipper all the time, but it requires it ALL THE TIME. In fact, this company finds that 30% of orders are not valid the first time around. This particular implementation if IOrderShipper is a little bit expensive to create (like ISessionFactory of NHibernate is expensive to create). Forcing the creating when the false branch of the IF statement would render it useless is the smell.

Just for transparency, here is OrderShipper:

   1:  public class OrderShipper : IOrderShipper

   2:  {

   3:      public OrderShipper()

   4:      {

   5:          Thread.Sleep(TimeSpan.FromMilliseconds(777));

   6:      }

7:

   8:      public void Ship(Order order)

   9:      {

  10:          //ship the order

  11:      }

  12:  }

If you are truly coupling yourself to an abstraction (the interface), you should now care how the abstraction is implemented.

You might say: “What? Just move that long-running work to the Ship()

method!”

I would agree with that if you were designing the OrderShipper class. That only puts very elusive semantic coupling on all your classes. What you are really assuming is: I will code to interfaces. . . AND. . . as long as all of the implementations have constructors that don’t do much work, my whole application should work.

I am not decrying constructor injection as a useful method. In fact, we at Headspring do more constructor injection that abstract factory resolution. However, constructor arguments are API artifacts that shout loudly: “I can do NOTHING unless you pass these things in.” If that isn’t true, then don’t make the constructor announce a lie.

Cleaning up the smell

We can modify OrderProcessor to use an abstract factory and only require an IOrderShipper if the order is valid. Here is the new run:

Down from 7 seconds to less than 1. Why? Because the invalid order did not need to be shipped.

Here is the new OrderProcessor:

   1:  public class OrderProcessor : IOrderProcessor

   2:  {

   3:      private readonly IOrderValidator _validator;

4:

   5:      public OrderProcessor(IOrderValidator validator)

   6:      {

   7:          _validator = validator;

   8:      }

9:

  10:      public SuccessResult Process(Order order)

  11:      {

  12:          bool isValid = _validator.Validate(order);

  13:          if (isValid)

  14:          {

  15:              IOrderShipper shipper = new OrderShipperFactory().GetDefault();

  16:              shipper.Ship(order);

  17:          }

18:

  19:          return CreateStatus(isValid);

  20:      }

21:

  22:      private SuccessResult CreateStatus(bool isValid)

  23:      {

  24:          return isValid ? SuccessResult.Success : SuccessResult.Failed;

  25:      }

  26:  }

Because we don’t require any implementation of IOrderShipper (line 15) unless the order is valid, our code is simpler. The class itself has 1 fewer class-level dependency, and only one method has the extra dependency. We employ a new type, OrderShipperFactory to keep the Inverted Control where OrderProcessor still doesn’t know the concrete implementation. There is one more step because factories need start-up configuration. If you are currently using an IoC container, you are familiar with start-up configuration because you configure the container’s global factory at start-up time. Here is the code for the factory:

   1:  public class OrderShipperFactory

   2:  {

   3:      public static Func<IOrderShipper> CreationClosure;

   4:      public IOrderShipper GetDefault()

   5:      {

   6:          return CreationClosure();//executes closure

   7:      }

   8:  }

And here is the method that configures this factory at start-up time:

   1:  private static void ConfigureFactories()

   2:  {

   3:      OrderShipperFactory.CreationClosure =

   4:          () => ObjectFactory.GetInstance<IOrderShipper>();

   5:  }

Notice that the IoC container is still in control over creating the concrete implementations, but the abstract factories keep the production code dependencies inverted without being greedy toward optional dependencies.

Counter Argument

Ok, well, then I’ll just create ONE factory like this and use it everywhere:

   1:  public class GenericFactory

   2:  {

   3:      public static Func<object> CreationClosure;

   4:      public T GetDefault<T>()

   5:      {

   6:          return (T) CreationClosure();//executes closure

   7:      }

   8:  }

This factory will ONLY work with an IoC container that has the same semantics as the one you are currently using. For instance, you still need to unit test the code that uses the factory, so you will have to implement a stub for the factory (and unit tests don’t have expensive dependencies like IoC containers). Bottom line: this API is hard to implement by coding by hand. Any API that is hard to implement is a bad API. You want factories to be explicitly, and a method or class that can create anything????

Hmm. Isn’t that Activator.CreateInstance()? But even that is limited because it doesn’t do constructor argument chaining.

Bottom Line

Architecture and structure are agnostic of tools. IoC containers are just tools. Your design should be robust with or WITHOUT them. Taking away the IoC container should not make the structure of the application crumble.

Why do we use an IoC container, then? Because of the DRY Principle. It keeps us from having to write lots of abstract factory implementations that are essentially the same except for one line. It keeps each class from having a factory call from the no-arg constructor passing in the dependency to the greedy constructor. IoC containers have limited the need for hand-coded factories, but they have not eliminated the need.

The inspiration for this article came while I was sitting in a workshop where Matt Hinze was teaching a class about Inversion of Control (one of the public workshops that Headspring puts on each month). I thought about the response to Robert Martin’s recent blog post (which I quite like). Then a student in the class recalled a project where constructor injection was used everywhere. They felt the pain that is sure to come from this constructor over-injection anti-pattern.

One final word: Don’t be in a hurry to agree or disagree with me. Take the time to ponder.

UPDATE: Good discussion below. I’ve expanded the code sample in an attempt to make this code smell more obvious (because in reality, we don’t fret over 2 constructor arguments. We fret about 5, 10, 20). New post here: http://jeffreypalermo.com/blog/constructor-over-injection-smell-ndash-follow-up/

Programming with Palermo

Jeffrey Palermo, Microsoft MVP, Author, Speaker, Clear Measure Chief Architect, Azure DevOps Expert