views:

55

answers:

3

Is this a good pattern? It has a code smell to me with having a factory class aware of the IUnityContainer...

My basic need was to resolve an ICalculationRuleProcess at runtime depending on an Id of a class. It could be based on something other than the Id, I am aware of that... basically I have a known set of Ids that I need to deal with because I bootstrapped the records into the database manually and there is no way to edit the records. With each Id I have a related class. I also have a varying number of constructor parameters within each class that implements the ICalculationRuleProcess, so using an IoC container is extremely helpful versus some crazy switch statement and variable constructor aguments using Activator.CreateInstance

Here is what I did:

  1. Registered the IUnityContainer instance within the container itself. I wasnt sure if this was even possible, but it worked.
  2. Registered all of the ICalculationRuleProcess classes with a unique identifier within the registration (basically just the Id.ToString() of each possible DistributionRule)
  3. Created a factory to determine the correct ICalculationRuleProcess, and had it use the IoC container to figure out the correct class to load.
  4. Registered the factory class (ICalculationRuleProcessFactory) to the IoC container
  5. Wherever the ICalculationRuleProcess needed to be used, I had the class take an ICalculationRuleProcessFactory in its constructor and have it call the Create method to figure out which ICalculationRuleProcess to use.

The code for the factory is here:

  public interface ICalculationRuleProcessFactory
  {
    ICalculationRuleProcess Create( DistributionRule distributionRule );
  }

  public class CalculationRuleProcessFactory : ICalculationRuleProcessFactory
  {
    private readonly IBatchStatusWriter _batchStatusWriter;
    private readonly IUnityContainer _iocContainer;

    public CalculationRuleProcessFactory(
      IUnityContainer iocContainer,
      IBatchStatusWriter batchStatusWriter )
    {
      _batchStatusWriter = batchStatusWriter;
      _iocContainer = iocContainer;
    }

    public ICalculationRuleProcess Create( DistributionRule distributionRule )
    {
      _batchStatusWriter.WriteBatchStatusMessage( 
        string.Format( "Applying {0} Rule", distributionRule.Descr ) );

      return _iocContainer.Resolve<ICalculationRuleProcess>(
        distributionRule.Id.ToString() );
    }
  }
+2  A: 

This seems okay to me, given the constraints you described. The most important thing is that all of your rules implement ICalculationRuleProcess and that all consumers of those rules only know about that interface.

It isn't inherently bad that your factory takes the container dependency, especially as an interface. Consider that if you ever had to change container implementations, you could create an IUnityContainer implementation that doesn't use Unity at all (just forward all the members of the interface to their corresponding methods in the replacement container).

If it really bothers you, you can add yet another layer of indirection by creating an agnostic IoC interface with the requisite Register, Resolve, etc. methods and create an implementation that forwards these to Unity.

Jay
+1 I always end up wrapping `UnityContainer` in my own interface, partially because some of the methods are extension methods and thus are not unit testable off `IUnityContainer` interface
Igor Zevaka
A: 

Hey Rob, I'm intending to use essentially the same pattern. I've got multiple types of shopping cart item that need to be associated with their own specific set of validator instances of varying class.

I think there is a smell about this pattern and its not that the factory has a reference to the IoC container, its that typically, an IoC container is configured in the application root which is typically the UI layer. If a crazy custom factory was created just to handle these associations then possibly it should be in the domain.

In short, these associations are possibly not part of the overall program structure that's set up before the application runs and so shouldn't be defined in the application root.

Ian Warburton
A: 

There is another way you can achieve this without factory taking dependency on IUnityContainer, which is not inherently bad in and of itself. This is just a different way to think about the problem.

The flow is as follows:

  1. Register all different instances of ICalculationRuleProcess.
  2. Get all registered ICalculationRuleProcess and create a creation lambda for each one.
  3. Register ICalculationRuleProcessFactory with a list of ICalculationRuleProcess creation lambdas.
  4. In ICalculationRuleProcessFactory.Create return the right process.

Now the tricky part of this is to preserve the Ids that the registrations were made under. Once solution is to simply keep the Id on the ICalculationProcess interface, but it might not semantically belong there. This is where this solution slips into ugly (which is more of a case of missing functionality in Unity). But, with an extension method and a small extra type, it looks nice when it's run.

So what we do here is create an extension method that returns all registrations with their names.

public class Registration<T> where T : class {
    public string Name { get; set; }
    public Func<T> CreateLambda { get; set; }

    public override bool Equals(object obj) {

        var other = obj as Registration<T>;
        if(other == null) {
            return false;
        }


        return this.Name == other.Name && this.CreateLambda == other.CreateLambda;
    }


    public override int GetHashCode() {
        int hash = 17;
        hash = hash * 23 + (Name != null ? Name.GetHashCode() : string.Empty.GetHashCode());
        hash = hash * 23 + (CreateLambda != null ? CreateLambda.GetHashCode() : 0);
        return hash;
    }


}

public static class UnityExtensions {
    public static IEnumerable<Registration<T>> ResolveWithName<T>(this UnityContainer container) where T : class {
        return container.Registrations
          .Where(r => r.RegisteredType == typeof(T))
          .Select(r => new Registration<T> { Name = r.Name, CreateLambda = ()=>container.Resolve<T>(r.Name) });
    }
}

 public class CalculationRuleProcessFactory : ICalculationRuleProcessFactory
  {
    private readonly IBatchStatusWriter _batchStatusWriter;
    private readonly IEnumerable<Registration<ICalculationRuleProcess>> _Registrations;

    public CalculationRuleProcessFactory(
      IEnumerable<Registration<ICalculationRuleProcess>> registrations,
      IBatchStatusWriter batchStatusWriter )
    {
      _batchStatusWriter = batchStatusWriter;
      _Registrations= registrations;
    }

    public ICalculationRuleProcess Create( DistributionRule distributionRule )
    {
      _batchStatusWriter.WriteBatchStatusMessage( 
        string.Format( "Applying {0} Rule", distributionRule.Descr ) );

      //will crash if registration is not present
      return _Registrations
        .FirstOrDefault(r=>r.Name == distributionRule.Id.ToString())
        .CreateLambda();
    }
  }

//Registrations
var registrations = container.ResolveWithName<ICalculationRuleProcess>(container);
container.RegisterInstance<IEnumerable<Registration<ICalculationRuleProcess>>>(registrations);

After I wrote this I realised that this is more creative lambda douchebaggery than a architecturally pretty solution. But in any case, feel free to get ideas out of it.

Igor Zevaka