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:  }

Let’s just run it:

image

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:

image

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/


Trackbacks

Constructor over-injection anti-pattern Posted on 1.20.2010 at 10:37 AM

You've been kicked (a good thing) - Trackback from DotNetKicks.com

Constructor over-injection smell – follow up Posted on 1.21.2010 at 12:03 PM

This is a follow up to my article: “ Constructor over-injection anti-pattern ”.&#160; I’ve title this a bit differently because as with any heuristic, there are degrees of goodness and smelliness.&#160; Constructor over-injection, therefore, is a code

Comments

Adam D. said on 1.19.2010 at 4:44 PM

Why not be more explicit and adopt CQRS with a core domain that does the validation. The explicit nature then comes from the messages (events) that are published.

Erik N said on 1.19.2010 at 5:23 PM

Just wondering would it be wise to lazy load the IOrderShipper dependancy rather then retrieving it from the container each time it is required. e.g. If the method was called in a loop.

Just some food for thought

Json said on 1.19.2010 at 6:05 PM

The math in this article seems incorrect. It doesn't seem possible that the total time can be LESS than .777 seconds the NEW way when there is an object with a constructor that takes .777 seconds to initialize. Even if you are using a singleton for the IOrderShipper instance, it should be .777 seconds or more of total time.

Following the logic used to prove it takes 7.77 seconds using the constructor-injection way, it should take at the least 5.4 seconds using the factory way. (7 valid orders * 7 IOrderShipper instances)

Jeffrey Palermo said on 1.19.2010 at 6:13 PM

None of the orders are valid here

Json said on 1.19.2010 at 6:30 PM

Oh come on. That's "convenient" math. If 30% of the orders are invalid, then average that out over scenarios where all orders are valid or invalid within a particular 10 order batch. You just can't state, "we go from 7 seconds to .004 seconds because of using the factory". That's misleading and not practical, and not what really happens. Treat every 10 order batch with a 30% failure rate and you get the real picture here. The fact that you are using two different ways to introduce dependencies to a component is about as confusing as the math.

Derek Greer said on 1.19.2010 at 6:37 PM

I would define an optional dependency as being one whose provision causes behavior otherwise not present. An optional logging component comes to mind. What is in view here is an inherent dependency whose invocation is determined at runtime. Whether it makes sense to lazy-load this dependency is something that should be based on performance metrics, not on whether a logical path exists without use of the dependency. As an example, we wouldn’t lazy load every component simply because a raised exception within the method of a transient object caused all the dependencies to go unused for that instance. As far as the example is concerned, I’m not sure I see a need for any of these objects to be transient, so a simpler solution to the problem would seem to be to just register the OrderProcessor as a singleton with the container.

Andy Hitchman said on 1.19.2010 at 8:14 PM

Rather than a factory, how about the constructor taking and Func<IOrderShipper>. Then provide a StructureMap (which seems to be your container of choice) convention to satisfy the dependency appropriately (i.e. a singleton for each call or a new instance for each call, etc).

Jason Meckley said on 1.19.2010 at 8:19 PM

Wouldn't instantiating the factory class within the OrderProcessor make it more difficult to test? why not inject create an interface for the factory and inject that into the ctor? this way it's still easy to test and you get the "lazy loaded" object from the factory.

Derek's comment also seems to make sense. ctor vs property injection is usually based on required vs. optional components. Shipper doesn't appear to be optional in this case.

Michael L Perry said on 1.19.2010 at 8:40 PM

By creating a new OrderShipperFactory, you've tightly coupled your code. It knows the name of a concrete class. Remember: no "new"s is good news.

Alwin said on 1.19.2010 at 10:36 PM

What about this: constructor:

public OrderProcessor(IOrderValidator validator, IOrderShipperFactory orderShipperFactory)

{

//..

}

Or in .Net4:

public OrderProcessor(IOrderValidator validator, Lazy<IOrderShipper> orderShipper)

{

//..

}

No tight coupling to OrderShipperFactory, no "new", and no expensive creation of OrderShipper when it is not needed.

Julian Birch said on 1.20.2010 at 4:52 AM

I'm with Alwin and Michael here. All your example does is replace the dependency on an order shipper with a dependency on a order shipping factory. Taking as red there's nothing special about factories, Alwin's code is the best implementation.

The CreationClosure is just another way of making your container a global variable, which is unnecessary. I'd argue an abstract factory that wraps your container (and injected into your container) is more elegant.

Dave Erwin said on 1.20.2010 at 8:43 AM

Thanks for the post. I've been teetering on the edge of this problem for a week or so but have not fallen into the pit yet. It gives me something to think about and refer back to when refactoring.

Bill Pierce said on 1.20.2010 at 10:23 AM

Modifying all consumers of IOrderShipper increases the smell, it does not clean it up. You code to interfaces because you do not care about the implementation (including initialization). If initializing the implemenation is the bottle neck, you have multiple options:

1) Optimize the initialization so it is faster.

2) Delay the initialization (as you pointed out above).

3) Don't initialize anymore than you have to (implement an object pool).

If your ObjectFactory is mature it will support instancing strategies beyond new/singleton or be extensible enough to supply additional instancing strategies (pooled strategy).

Don't burden my code because your implementation is sub par.

If you ignore the above advice and continue down the path you have described, why do I need an additional factory? If your ObjectFactory is mature it will implement an interface and configure itself as a resolvable instance. So your OrderProcessor ctor would take an IOrderValidator and an IObjectFactory. At least with this approach I can see that the class has additional dependencies simply by looking at the ctor.

Mark Seemann said on 1.20.2010 at 10:29 AM

I strongly disagree with most of this post. Read my response here: blog.ploeh.dk/.../RebuttalConstru

Chris Holmes said on 1.20.2010 at 10:48 AM

Why are you getting a new IOrderProcessor from your container EVERY SINGLE TIME in the foreach loop? I mean, don't we all assume that if you're asking an IoC container for something, it might take it a while to build it? Why would you not ask for the IOrderProcessor ONCE - before jumping into the IoC loop?

This doesn't seem like an IoC or constructor injection problem to me. This seems like a bad procedural coding problem to me. Expensive operations should be done once...

I don't get it.

Hightechrider said on 1.20.2010 at 10:50 AM

I'm with the naysayers. In addition: you should nearly always use DateTime.UtcNow never DateTime.Now as a matter of habit because DateTime.Now can go backwards or leap forwards by an hour (probably not while you are running this example but it's a good habit to be in). And, in any case System.Diagnostics.Stopwatch would be a better way to time it.

Will Rogers said on 1.20.2010 at 10:57 AM

Chris Holmes' question points out what I first noticed when scanning the code. Why do you need a new processor instance for each order? That seems like a more fundamental source of inefficiency than anything else you proposed.

Jeffrey Palermo said on 1.20.2010 at 11:36 AM

This is all good discussion. Any of these ideas ought to be implementable without an IoC container.

Jimmy Bogard said on 1.20.2010 at 12:29 PM

@Jeffrey

All of these ideas _can_ be implemented w/o a container, it just takes a lot more code. I think one of the benefits of a container is to expressiveness and power of IoC and DIP without the pain of manual wiring.

An IoC container is a sharp tool that allows for more advanced scenarios with far less code. I could pretend a container doesn't exist, but why would I try and build a house with a regular hammer? I'd rather use a nail gun.

Yes, I realize that this is an example, but every instance of poor man's DI, service location, new'ing up things in an object is a more opaque design. But there really isn't such a thing as over-injection. Either an object is a dependency, or it isn't. If instantiation performance was an issue, you could take a dependency on the factory.

But as Mark pointed out, now you've leaked the concerns of the construction of a _specific implementation_ into the order processor. That's at least a couple of SOLID principles violated. The best option I still see here is to do normal IoC, DI, and let our container handle responsibilities of wiring everything up for specific implementations.

If you didn't want to have a container involved, that's where I'd make an OrderProcessorFactory, and not let the order processor's design be compromised by other concerns. But since I have a sharp tool, I'm not going to manually hammer these things together.

Jeffrey Palermo said on 1.20.2010 at 1:56 PM

@Jimmy,

There are no SOLID principles violated here. Dependency on a _specific implementation_ is just fine as long as that concretion is in the same layer. Not every class needs an interface. Adding indirection within a single layer may also be another anti-pattern.

This isn't about performance either. Proxying the interface for lazyloading would solve that as well (as pointed out by another commenter). It's about being explicit.

Also, if the scope of IOrderShipper needs to be managed, cached, or a singleton, that would be the responsibility of the factory to choose that, not OrderProcessor.

Yes, IoC containers can do instance scoping, and we have done that to our own detriment. It has resulted in application behavior being tied up in the global IoC container configuration. The problem is, that global configuration is hard to debug, and instance scoping problems rarely show themselves when just running the unit tests.

I love the discussion resulting from this post. 5 years ago, I was one of the people singing the praises of IoC containers. I have learned much since then, and I am coming back from the pendulum swing to the extreme. I'm trying to stop, balanced in the middle rather than continue on the pendulum ride.

In everything, moderation.

Chris Holmes said on 1.20.2010 at 4:25 PM

I think all you've done here Jeffrey is create complexity where none was warranted. You've added another layer of indirection (the factory) and buried a concrete dependency inside a class (which isn't necessary, nor desirable; in an era when we're trying to convince developers that concrete dependencies are bad - and convince them WHY they are bad - this makes no sense).

If this is a performance based problem then simply moving the construction of the processor OUTSIDE the scope of the loop fixes that (and really, that's the #1 flaw I see with this code, and I would expect any junior developer to see that and remedy it).

The rest of this seems like an exercise in obfuscation.

Your goal with this post might be to illustrate "moderation in everything". My message would be "simplicity of design trumps all".

Jeffrey Palermo said on 1.20.2010 at 4:36 PM

@Chris Holmes

> in an era when we're trying to convince developers that concrete dependencies are bad

I was at that stage in 2007, but I've backed off from that position. Concrete dependencies are necessary. Indirection is necessary across layers, not everywhere.

>If this is a performance based problem

It's not a performance-based problem. If it were, I'd just proxy the interface and add some caching.

One of the goals is to minimize the IoC containers configuration and keep it simple. I want the IoC container to be a dependency added only when needed instead of right away.

MJM said on 1.20.2010 at 11:47 PM

But aren't you just adding another layer of configuration to the system - by minimizing the IoC container's configuration, you are requiring that these factories need to be configured somewhere. Why not just keep the configuration of the system in one place - the IoC configuration file?

Nicholas Blumhardt said on 1.21.2010 at 5:06 AM

So much fuss yet the most obvious solution is overlooked?

Just make the constructor dependency on IOrderShipper lazy, i.e. with Lazy<T>.

public OrderProcessor(IOrderValidator validator, Lazy<IOrderShipper> orderShipper)

Example using Autofac2 here: nblumhardt.com/.../lazing-around-w

@Jeffrey, the problem's not depending on concrete types (although it is nice to code to an abstraction.)

The problem is the hidden dependency on ambient state. Your default order shipper will probably go out and hit the database, plus a bunch of services - none of which can be anticipated by its callers. This makes code much less flexible and harder to maintain.

Tomas said on 1.21.2010 at 5:19 AM

Great discussion.. I'm bit new to the "new" C#, could you please explain me why its better to use Func<IOrderShipper> delegate instead of plain static IOrderShipper property?

Chris Holmes said on 1.21.2010 at 10:30 AM

"But aren't you just adding another layer of configuration to the system - by minimizing the IoC container's configuration, you are requiring that these factories need to be configured somewhere."

Yep - moving the configuration around doesn't actually change the fact that you still need to configure something. I don't see the benefit. I only see more unnecessary complexity.

"The problem is the hidden dependency on ambient state. "

Bingo, we have a winner.

Sorry Jeffrey. I respect you and I've read your blog for years, but I don't agree with you one bit on this one.

Oh well :-)

Jeffrey Palermo said on 1.21.2010 at 10:50 AM

@Chris,

No problem. It's a good thing this blog isn't meant to get people to agree. When everyone agrees, one becomes irrelevant.

Erich Eichinger said on 1.25.2010 at 11:28 AM

I do think this discussion is missing an important aspect - see my blog post eeichinger.blogspot.com/.../danger-of-misle