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: }
.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; }
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: }
.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; }
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).