Bad Coupling with Constructor Injection

20 August 2014

In previous posts, I started to explain the difference between Dependency Injection (DI) and a simple factory. To leverage more of the potential of DI, I gave an example of using constructor injection and contrasted that with setter injection. Like all things, too much of a good thing can have horrible consequences. Hopefully, this post can help you to identify when things are going wrong.

A few years back, Jeff Palermo posted about the "Constructor over-injection anti-pattern." While I appreciate the details of his post and his solution, I think it is very important to look at what he said in his update: "We don't fret over 2 constructor arguments. We fret about 5, 10, 20 [or more constructor arguments]." You absolutely can overdo it with the number of parameters that you pass in via constructor injection. But, to me, the number of dependencies that you inject is more of a cohesiveness problem than a performance problem.

To have good cohesion, classes should have a small number of instance variables, which includes dependencies injected into the constructor. Every method should manipulate/use all (or at least most) of those variables. The greater the percentage of instance variables used by a method, the more cohesive that method is to its class.

Although the second method in the code below is a bit useless in its current implementation (because it is identical to the first method), there is execellent cohesion in this class because all of the methods are using all of the class-level variables (i.e. "validator" and "repo").

 1 public class OrderCommand : IOrderCommand
 2 {
 3     //Used by all methods
 4     private readonly IOrderValidator validator;
 5     private readonly IOrderRepository repo;
 6 
 7     public OrderCommand(IOrderValidator validator, 
 8                                  IOrderRepository repo)
 9     {
10         this.validator = validator;
11         this.repo = repo;
12     }
13     
14     public void PlaceOrder(Order order)
15     {
16         if (!validator.IsValidator(order))
17             throw new ApplicationException("Order not valid");
18 
19         repo.Save(order);
20     }
21     
22     public void UpdateOrder(Order order)
23     {
24         if (!validator.IsValidator(order))
25             throw new ApplicationException("Order not valid");
26 
27         repo.Save(order);
28     }
29 }

The updated implementation below, is very similar to the first implmentation. But, in this one, there is an additional dependency that is only used by the UpdateOrder method. This is still a very good level of cohesion because all of the methods are using a large percentage of the class-level variables.

 1 public class OrderCommand : IOrderCommand
 2 {
 3     //Used by all methods
 4     private readonly IOrderValidator validator;
 5     private readonly IOrderRepository repo;
 6     
 7     //Only used by UpdateOrder()
 8     private readonly IOrderNotifications notify;
 9 
10     public OrderCommand(IOrderValidator validator, 
11                                  IOrderRepository repo,
12                                  IOrderNotifications notify)
13     {
14         this.validator = validator;
15         this.repo = repo;
16         this.notify = notify;
17     }
18     
19     public void PlaceOrder(Order order)
20     {
21         if (!validator.IsValidator(order))
22             throw new ApplicationException("Order not valid");
23 
24         repo.Save(order);
25     }
26     
27     public void UpdateOrder(Order order)
28     {
29         if (!validator.IsValidator(order))
30             throw new ApplicationException("Order not valid");
31 
32         repo.Save(order);
33         notify.NotifyWarehouseManager(order);
34     }
35 }

While the implementations above have good cohesion, the implementation below is a fairly obvious case of bad cohesion. The PlaceOrder method for domestic orders does not use the dependencies for international orders and vise versa. Notice how much more difficult it is to read this code. Everything is cluttered together. And, at first glance, it is difficult to see how pieces of the class relate to one another.

 1 public class OrderCommand : IOrderCommand
 2 {
 3     readonly IDomesticOrderValidator domesticValidator;
 4     readonly IInternationalOrderValidator internationalValidator;
 5     readonly IDomesticOrderRepository domesticRepo;
 6     readonly IInternationalOrderRepository internationalRepo;
 7 
 8     public OrderCommand(IDomesticOrderValidator domesticValidator, 
 9              IInternationalOrderValidator internationalValidator, 
10              IDomesticOrderRepository domesticRepo,
11              IInternationalOrderRepository internationalRepo)
12     {
13         this.domesticValidator = domesticValidator;
14         this.internationalValidator = internationalValidator;
15         this.domesticRepo = domesticRepo;
16         this.internationalRepo = internationalRepo;
17     }
18     
19     public void PlaceOrder(DomesticOrder order)
20     {
21         if (!domesticValidator.IsValidator(order))
22             throw new ApplicationException("Order not valid");
23 
24         domesticRepo.Save(order);
25     }
26     
27     public void PlaceOrder(InternationalOrder order)
28     {
29         if (!internationalValidator.IsValidator(order))
30             throw new ApplicationException("Order not valid");
31 
32         internationalRepo.Save(order);
33     }
34 }

To solve this, consider breaking down your classes into smaller, more logically grouped classes. Low cohesion is an indicator that at least one other class is trying to get out of the larger class.

 1 // Example of class being split up into smaller more 
 2 // cohesive classes...
 3 public class DomesticOrderCommand : IOrderCommand<DomesticOrder>
 4 {
 5     private readonly IDomesticOrderValidator validator;
 6     private readonly IDomesticOrderRepository repo;
 7 
 8     public DomesticOrderCommand(IDomesticOrderValidator validator,  
 9         IDomesticOrderRepository repo)
10     {
11         this.validator = validator;
12         this.repo = repo;
13     }
14 
15     public void PlaceOrder(DomesticOrder order)
16     {
17         if (!validator.IsValidator(order))
18             throw new ApplicationException("Order not valid");
19 
20         repo.Save(order);
21     }
22 }
23 
24 public class InternationalOrderCommand 
25     : IOrderCommand<InternationalOrder>
26 {
27     readonly IInternationalOrderValidator validator;
28     readonly IInternationalOrderRepository repo;
29 
30     public InternationalOrderCommand(
31         IInternationalOrderValidator validator,
32         IInternationalOrderRepository repo)
33     {
34         this.validator = validator;
35         this.repo = repo;
36     }
37 
38     public void PlaceOrder(InternationalOrder order)
39     {
40         if (!validator.IsValidator(order))
41             throw new ApplicationException("Order not valid");
42 
43         repo.Save(order);
44     }
45 }

While cohesiveness can also be overdone, you do want to maintain a high level of it in your classes. The more dependencies that you inject into your class, the less likely it will be that your methods will be highly cohesive. By this I mean that despite the fact that methods are all in the same class and the developer thinks they are related, they are actually not related and therefore do not belong in the same class. If they did, they would depend on (mostly) the same things. In general, I think that 3 constructor injection variables is pushing it, and 4 or 5 is too many. But, I do not consider this a hard limit. Every situation is different.

Increased construction injection leads to the following:

  • Possible performance issues (see the Palermo article)
  • Increased test friction because of more dependencies to fake out
  • Readability issues
  • Maintainability issues

Do not think that you can mitigate this problem by using constructor injection to pass the dependency injection (DI) container into your class. This completely masks whether or not your class is cohesive and does not solve the problem. This is because there is no limit to the number of dependencies that can be pulled out of the DI container.

 1 /* !!!!!!!!!!!!! DO NOT DO THIS !!!!!!!!!!!!! */
 2 public class OrderCommand : IOrderCommand
 3 {
 4     private readonly IWindsorContainer container;
 5 
 6     public OrderCommand(IWindsorContainer container)
 7     {
 8         // Yes this looks cleaner, but is does not fix the problem.
 9         this.container = container;
10     }
11 
12     public void PlaceOrder(DomesticOrder order)
13     {
14         var validator = container
15             .Resolve<IDomesticOrderValidator>();
16         if (!validator.IsValidator(order))
17             throw new ApplicationException("Order not valid");
18         
19         var repo = container
20             .Resolve<IDomesticOrderRepository>();
21         repo.Save(order);
22     }
23 
24     public void PlaceOrder(InternationalOrder order)
25     {
26         var validator = container
27             .Resolve<IInternationalOrderValidator>();
28         if (!validator.IsValidator(order))
29             throw new ApplicationException("Order not valid");
30         
31         var repo = container
32             .Resolve<IInternationalOrderRepository>();
33         repo.Save(order);
34     }
35 }

Other indications of poor class cohesiveness are generic names. Names such as "OrderManager" are a dead giveaway that there is likely low cohesiveness in a class. Names should be specific and descriptive of everything that a class does. If you cannot clearly describe all that a class does, then it is likely too large and lacks cohesion. An extreme case of low cohesion can lead to the "Blob" anti-pattern.

A better way to breakdown "Manager" classes is to follow common design patterns and name your classes based on the pattern it follows (e.g. OrderProcessingCommand and OrderConfirmationStratgy). By following design patterns and naming the classes accordingly, you are not only corralled into splitting functionality up appropriately, but you also organize your classes in such a way that you can easily identify the appropriate place to find what you are looking for.

Conclusions

Injecting too many dependencies into a class is an indication that your class is trying to do too many things. Although this opens the possibility of performance issues, it absolutely causes other problems such as increased test friction and reduced maintainability. Consider breaking down your classes into smaller, more maintainable classes and consider following common design patterns.