CODEWITHSUNDEEP
c#.netdependency-injectioninversion-of-control
Steve Wright
Steve Wright

Reputation: 2511

IoC and constructor over-injection anti-pattern resolution

This question is a result of a post by Jeffery Palermo on how to get around branched code and dependency injection http://jeffreypalermo.com/blog/constructor-over-injection-anti-pattern/

In his post, Jeffery has a class (public class OrderProcessor : IOrderProcessor) that takes 2 interfaces on the constructor. One is a validator IOrderValidator and an IOrderShipper interface. His method code branches after only using methods on the IOrderValidator interface and never uses anything on the IOrderShipper interface.

He suggests creating a factory that will call a static method to get the delegate of the interface. He is creating a new object in his refactored code which seems unnecessary.

I guess the crux of the issue is we are using IoC to build all our objects regardless if they're being used or not. If you instantiate an object with 2 interfaces and have code that could branch to not use one of them, how do you handle it?

In this example, we assume _validator.Validate(order) always returns false and the IOrderShipper.Ship() method is never called.

Original Code:

public class OrderProcessor : IOrderProcessor
{
    private readonly IOrderValidator _validator;
    private readonly IOrderShipper _shipper;

    public OrderProcessor(IOrderValidator validator, IOrderShipper shipper)
    {
      _validator = validator;
      _shipper = shipper;
    }

    public SuccessResult Process(Order order)
    {
      bool isValid = _validator.Validate(order);
      if (isValid)
      {
          _shipper.Ship(order);
      }
      return CreateStatus(isValid);
    }

    private SuccessResult CreateStatus(bool isValid)
    {
        return isValid ? SuccessResult.Success : SuccessResult.Failed;
    }
}

public class OrderShipper : IOrderShipper
{
  public OrderShipper()
  {
      Thread.Sleep(TimeSpan.FromMilliseconds(777));
  }

  public void Ship(Order order)
  {
      //ship the order
  }
}

Refactored Code

public class OrderProcessor : IOrderProcessor
{
    private readonly IOrderValidator _validator;

    public OrderProcessor(IOrderValidator validator)
    {
      _validator = validator;
    }

    public SuccessResult Process(Order order)
    {
      bool isValid = _validator.Validate(order);
      if (isValid)
      {
          IOrderShipper shipper = new OrderShipperFactory().GetDefault();
          shipper.Ship(order);
      }
      return CreateStatus(isValid);
    }

    private SuccessResult CreateStatus(bool isValid)
    {
        return isValid ? SuccessResult.Success : SuccessResult.Failed;
    }
}   

public class OrderShipperFactory
{
    public static Func<IOrderShipper> CreationClosure;
    public IOrderShipper GetDefault()
    {
        return CreationClosure(); //executes closure
    }
}

And here is the method that configures this factory at start-up time (global.asax for ASP.NET):

private static void ConfigureFactories()
{
    OrderShipperFactory.CreationClosure =
        () => ObjectFactory.GetInstance<IOrderShipper>();
}

Upvotes: 12

Views: 2737

Answers (2)

eduncan911
eduncan911

Reputation: 17604

I'm running late for a meeting, but a few quick points...

Sticking to the code branching of using only one dependency, there are two branches to propose:

  • Applying DDD practices, you would not have an OrderProcessor with a dependency on IOrderValidator. Instead, you'd make the Order() entity be responsible for its own validation. Or, stick to your IOrderValidator, but have its dependency within the OrderShipper() that is implemented - since it will return any error codes.
  • Ensure the dependencies being injected are constructed using a Singleton approach (and configured as Singleton in the IoC container being used). This resolves any memory concerns.

Like someone else that mentioned here, the point is to break the dependency on concrete classes and loosely couple the dependencies using Dependency Injection with some IoC container in use.

This makes future refactoring and swapping out legacy code far easier on a grand scale by limiting the technical debt incurred in the future. Once you have a project near 500,000 lines of code, with 3000 unit tests, you'll know first hand why IoC is so important.

Upvotes: 1

Mark Seemann
Mark Seemann

Reputation: 233150

I just posted a rebuttal of Jeffrey Palermos post.

In short, we should not let concrete implementation details influence our design. That would be violating the Liskov Substitution Principle on the architectural scale.

A more elegant solution lets us keep the design by introducing a Lazy-loading OrderShipper.

Upvotes: 22

Related Questions