Constructor over-injection smell – follow up

This is a follow up to my article: “Constructor over-injection anti-pattern”.  I’ve title this a bit differently because as with any heuristic, there are degrees of goodness and smelliness.  Constructor over-injection, therefore, is a code smell, I believe.  Without concrete do’s and don’ts, I believe it doesn’t deserve the explicit title of “anti-pattern”.

I have expanded the code sample (and it is available as a zip or tar file here).

I’ve modified the OrderProcessor as shown here.  I believe this is more egregious and is more of an obvious example.  Small samples get so trivial, that the pain show doesn’t seem like a problem at all, but when injected constructor arguments increase (think 5, 10, 20), the pain is greater.

   1:  namespace DIAntiPattern
   2:  {
   3:      public class OrderProcessor : IOrderProcessor
   4:      {
   5:          private readonly IOrderValidator _validator;
   6:          private readonly IAccountsReceivable _receivable;
   7:          private readonly IRateExchange _exchange;
   8:          private readonly IUserContext _userContext;
   9:   
  10:          public OrderProcessor(IOrderValidator validator,
  11:                                IAccountsReceivable receivable,
  12:                                IRateExchange exchange, IUserContext userContext)
  13:          {
  14:              _validator = validator;
  15:              _receivable = receivable;
  16:              _exchange = exchange;
  17:              _userContext = userContext;
  18:          }
  19:   
  20:          public SuccessResult Process(Order order)
  21:          {
  22:              bool isValid = _validator.Validate(order);
  23:              if (isValid)
  24:              {
  25:                  Collect(order);
  26:                  IOrderShipper shipper = new OrderShipperFactory().GetDefault();
  27:                  shipper.Ship(order);
  28:              }
  29:   
  30:              return CreateStatus(isValid);
  31:          }
  32:   
  33:          private void Collect(Order order)
  34:          {
  35:              User user = _userContext.GetCurrentUser();
  36:              Price price = order.GetPrice(_exchange, _userContext);
  37:              _receivable.Collect(user, price);
  38:          }
  39:   
  40:          private SuccessResult CreateStatus(bool isValid)
  41:          {
  42:              return isValid ? SuccessResult.Success : SuccessResult.Failed;
  43:          }
  44:      }
  45:  }

Notice specifically that IRateExchange is NOT a dependency of OrderProcessor at all.  It is actually needed by an aggregate root, Order, so that it can do the price conversion based on exchange rates and prices.  This particular design attempted put put that logic as close to the domain model as possible (inside an aggregate).  This code works, but it requires the caller to pass through the dependency.  Here is the Order class:

   1:  using System;
   2:   
   3:  namespace DIAntiPattern
   4:  {
   5:      public class Order
   6:      {
   7:          public DateTime Placed { get; set; }
   8:          public string OtherInfo { get; set; }
   9:          public Price GetPrice(IRateExchange exchange, IUserContext userContext)
  10:          {
  11:              User currentUser = userContext.GetCurrentUser();
  12:              Currency currency = userContext.GetSelectedCurrency(currentUser);
  13:              int priceInSelectedCurrency = exchange.Convert(GetPrice(), currency);
  14:              var price = new Price{Currency = currency, Value = priceInSelectedCurrency};
  15:              return price;
  16:          }
  17:   
  18:          private int GetPrice()
  19:          {
  20:              //do work to aggregate prices from line items, and total up order.
  21:              return 1000;
  22:          }
  23:      }
  24:  }

There are plenty of options to fix this code, and I’m sure you will comment on the one you prefer.  The SMELL here is constructor over-injection.  The injected dependency isn’t a dependency, and another dependency is sometimes a dependency of a particular supported operation of a class.

However you fix this example, IRateExchange does not deserve to be constructor-injected into OrderProcessor.  Next, LineItem will need a dependency to calculate something, and then the dependency has to flow through from an object that is managed by the IoC container.

IoC containers are just tools, but we still have to be careful of constructor over-injection. 

I welcome the comments, and don’t forget to download the code (and fork it to post about your own counter example). 


Trackbacks

Constructor over-injection anti-pattern : Jeffrey Palermo (.com) Posted on 1.21.2010 at 12:05 PM

Pingback from Constructor over-injection anti-pattern : Jeffrey Palermo (.com)

Comments

Peter Ritchie said on 1.21.2010 at 12:31 PM

I think "smell" is a much better term for this. It's a smell that is an indication of other design issues. Injecting "dependencies" via the c'tor that aren't always needed isn't directly counter-productive or ineffective as you'd expect from an Anti-Pattern. It does sometimes require some extraneous checks; but these are hardly counter-productive.

I would argue though, that if you start seeing 5, 10, 20 constructor arguments, you've got other design issues that you need to deal with.

Personally, I would tend to see OrderProcessor as a service and keep any business logic out of it. Right now it has got some important business logic in there; if that were abstracted away and some strategies were introduced, you'd probably be able to get away from the need for that many injections in the c'tor.

Neil Mosafi said on 1.21.2010 at 2:40 PM

I still completely disagree. Why does IRateExchange not deserve to be constructor-injected into OrderProcessor? It is required because for the Order to calculate its price you have to pass it on. It's a required dependency of some logic that exists in the class declaring the dependency. If it were null, the code would fail.

If the particular implementation of IRateExchange happens to be expensive to construct, then it's up to whichever code bootstraps your application to worry about that (e.g. use pooled resources, make it a single instance, etc). Your OrderProcessor should not be concerned with that at all.

This actually has nothing to do with IoC containers. Say you just had a simple console app with a main() function which passes new RateExchange() in as a dependency. Where is the code smell there?

Jeffrey Palermo said on 1.21.2010 at 3:11 PM

@Neil,

You are right about this having nothing to do with IoC containers. It does not; however, the smell is there regardless of where you are doing DI via abstract factories or an IoC tool.

How bad you let the smell get is certainly up to you.

I posit that OrderProcessor is merely acting as a dependency resolver for the Order class. OrderProcessor calls no methods on IRateExchange.

You are quite free to disagree. Thank you for the comment.

jonnii said on 1.21.2010 at 3:27 PM

Hi Jeff,

I've read this article a few times now and I'm not sure I agree. Your point that OrderProcessor is only acting as a dependency resolver for the order class is entirely due to the design/example you've chosen to use.

For example, instead of:

Price price = order.GetPrice(_exchange, _userContext);

You could write

Currency currency = _userContext.GetSelectedCurrency();

Price price = _orderPricer.GetPrice(order, currency);

All of a sudden your dependency in OrderProcessor disappears to be replaced by an IOrderPricer dependency, which you do call a method on thus justifying the dependency.

I would however agree that these pass through dependencies are an indication that code needs to be refactored and thought through more thoroughly.

Thanks,

jonnii

Jeffrey Palermo said on 1.21.2010 at 3:30 PM

@jonni,

Exactly!

>>I would however agree that these pass through dependencies are an indication that code needs to be refactored and thought through more thoroughly.

And your proposed solution would, indeed, be an improvement. The problem here is entirely around design (as is every code smell).

jonnii said on 1.21.2010 at 5:28 PM

@jeff

Going back to the original article I still don't think the solution to having the rarely used IOrderShipper is to add in a Factory.

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

For this you have lots of options, however I think the most elegant would be to have an event on the OrderProcessor which is raised when an order is sucessfully processed. Your OrderShipper can subscribe to the event through the container and ship your orders as required.

Anyway, this is an interesting topic for discussion, I'm glad you brought it up on your blog.

Brian Chiasson said on 1.21.2010 at 5:44 PM

I like these posts on DI as I am certainly guilty as charged. Part of the trouble, IMO, is that the IoC tools make it too easy.

In this example, it looks like the IAccountsReceivable's abstractions are leaking into the OrderProcessor and Order class. I would suggest refactoring this to make the IRateExchange and IUserContext dependencies in the IAccountReceivable's implementations. It seems more logical to me that IAccountReceivable's should be concerned about the IRateExchange and not the Order.

Creating an interface like IProduct that defines the GetPrice method makes this a little cleaner, because the code then becomes _receivable.Collect(order) followed by the calls to ship.

-Brian

Brian Chiasson said on 1.21.2010 at 5:47 PM

Sorry, I would keep the IUserContext dependency, otherwise, who are you collecting from ;)

Jimmy Bogard said on 1.21.2010 at 10:10 PM

@Brian

Remember, a class with too many dependencies is a class with too many responsibilities. It's the ALT.NET version of too many using statements. If you have a bazillion dependencies, it should be hard to test. Pay attention to the fragrance of your tests, that's an indication that the design is wrong. I still see the problem here isn't DI, it's a roles and responsibilities issue, which is what my test should be telling me. This would be a sweet article on test smells too.

I remember one way we solved an issue like this recently was to make our processor just be a generic pipeline, and just loop through a bunch of generic DI'd pipeline steps. This let us scale what was essentially procedural code and make sure the design of each step did not affect the others. Orthogonality ftw!

Brian Chiasson said on 1.22.2010 at 6:50 AM

@Jimmy

Before I used StructureMap's auto mocking, my tests definitely told me when there were too many dependencies. But the auto mocking stuff makes it easy to ignore the behaviors/interactions I am not interested in most of the time. This discussion is making me question my use of the auto mocking helpers.

And thanks for the great discussion!

-Brian

Tom Janssens said on 1.22.2010 at 11:29 AM

IMHO, I think this might be a textbook example of IOC & how not to do it.

You are injecting a dependency/Service through a functioncall here;

order.GetPrice(_exchange, _userContext);

Either Order is a domain object, so it should not have any dependencies, or order is a service, so you should inject both _exchange and _usercontext it in the order constructor by doing what Jimmy said : splitting it up in several separate pipelines.

This also makes your code a lot more compact an reliable...

If the interface is irrelavnt to the code it should not be there, it is as simple as that....

In my personal experience, by the time I get to more then 5 to 8 dependencies, I start refactoring out some stuff...

Jeffrey Palermo said on 1.22.2010 at 11:34 AM

@Tom,

Absolutely. When the code starts to look like this, it smells. Thanks for the suggestions. All good improvements.

Derek Greer said on 1.22.2010 at 2:39 PM

Since the context has changed from one of discussing optional dependencies to one of transitive dependencies, it feels a bit like the cart got before the horse. If the argument is that having more than three dependencies for a class is a smell then the theme holds, but in that case I would suggest that the name “Numerous Dependency Smell” would make for a better description.

A few seem to have become overly focused on the fact that this dependency came by way of injection which seems to be caused by the title and the method in which you chose to implement the example. All of these dependencies could have been resolved by separate factories and still have illustrated the same issue. The issue illustrated here has nothing to do with dependency injection, whether using a container or not.

Jeffrey Palermo said on 1.22.2010 at 3:02 PM

@Derek,

I can see your point that there are numerous things that could cause similar smells.

Because IoC containers make it so easy to stream constructor arguments all the way down, I've seem this manifestations become common with new users of IoC.

Derek Greer said on 1.22.2010 at 5:04 PM

DI makes bad design decisions easier in the same way an automatic transmission makes bad driving easier.

Mark Seemann said on 1.24.2010 at 2:41 AM

I think I agree more with this post than the previous one. As a general rule of thumb I would say that when your reach about four dependencies it's a code smell.

However, I would say that the smell has nothing to do with Constructor Injection - rather, it smells of a violation of the Single Responsibility Principle. In the current example, the OrderProcessor class simply does too much.

As other have pointed out, there's a whole interaction between dependencies going on in the Collect method that might be better modeled as an Aggregate Service - in short, encapsulate this whole interaction behind an interface that is injected instead of two or three single dependencies.

In short, Constructor Injection is neither an anti-pattern or a code smell in itself, but I certainly agree that injecting too many dependencies is a code smell. However, it is not a DI smell, but a general design smell.

Yazid said on 1.29.2010 at 5:22 PM

How about If I let the validator class create the OrderShipper if the order is valid.

replace the code below:

public SuccessResult Process(Order order)

{

bool isValid = _validator.Validate(order);

if (isValid)

{

Collect(order);

IOrderShipper shipper = new OrderShipperFactory().GetDefault(); shipper.Ship(order);

}

return CreateStatus(isValid);

}

public SuccessResult Process(Order order)

{

IOrderShipper shipper = _validator.Validate(order);

if (ishipper !=null)

{

Collect(order);

shipper.Ship(order);

}

return CreateStatus(isValid);

}

Yazid said on 1.30.2010 at 2:23 AM

Hello again,

Sorry got it wrong, can someone remove the above comment.

TIA

Yaz

Mark Seemann said on 2.02.2010 at 3:00 PM

For thoughts on how this code smell is addressed by a (more or less) formal refactoring, see blog.ploeh.dk/.../RefactoringToAg