11

I have a factory class that creates a couple of different types of class. The factory is registered with the container. What is the recommended way of creating the classes inside the factory, given that they also have dependencies. I clearly want to avoid a dependency on the container but if I new those classes then they won't be using the container. e.g.

public class MyFactory
{
    public IMyWorker CreateInstance(WorkerType workerType)
    {
        if (workerType == WorkerType.A)
              return new WorkerA(dependency1, dependency2);
        
        return new WorkerB(dependency1);    
    }
}

So the question is where do I get those dependencies from.

One option could be to make them dependencies of the factory. e.g.

public class MyFactory
{
    private Dependency1 dependency1;
    private Dependency2 dependency2;

    public MyFactory(Dependency1 dependency1, Dependency2, dependency2)
    {
        this.dependency1 = dependency1; this.dependency2 = dependency2;
    }

    public IMyWorker CreateInstance(WorkerType workerType)
    {
        if (workerType == WorkerType.A)
              return new WorkerA(dependency1, dependency2);
        
        return new WorkerB(dependency1);

    }
}

Another could be to register the worker types and make those dependencies of the factory e.g.

public class MyFactory
{
    private IWorkerA workerA;
    private IWorkerB workerB;

    public MyFactory(IWorkerA workerA, IWorkerB, workerB)
    {
        this.workerA = workerA; this.workerB = workerB;
    }

    public IMyWorker CreateInstance(WorkerType workerType)
    {
        if (workerType == WorkerType.A)
              return workerA;
        
        return workerB;  
    }
}

With the first option I feel like I am leeching the dependencies of the workers into the factory. With the second option the workers are created when the factory is created.

Steven
  • 166,672
  • 24
  • 332
  • 435
Ian1971
  • 3,666
  • 7
  • 33
  • 61
  • 1
    I like the looks of your first solution, assuming you can find a way to produce the dependencies. You may need them to come from a factory. Your second solution doesn't create instances, and therefore is not a factory. – Magus Nov 13 '13 at 19:01
  • Plus side of injecting all factory products in the constructor of the factory is that you get dependency graph verification for free, which isn't the case when you let your factory request (unregistered) services from the container. – Steven Nov 14 '13 at 19:41

4 Answers4

18

I agree with @Phil, letting the factory take a dependency on the container is fine, but there's one peace of information missing in his answer.

You are probably trying to prevent taking a dependency on the container because you try to stay away from the Service Locator anti-pattern. I agree that Service Locator is an anti-pattern and should be prevented.

Whether or not a dependency on the container is an implementation of the Service Locator anti-pattern depends on where this consumer is defined. Mark Seemann explains this here:

A DI container encapsulated in a Composition Root is not a Service Locator - it's an infrastructure component.

So letting your factory depend on the container is fine as long as you define this MyFactory implementation inside your composition root.

When you do this you'll soon get into trouble since a class that is defined in the composition root can't be referenced from the rest of the application. But that problem is easily solved by defining an IMyFactory interface in the application and letting your factory implementation implement that interface (as you should do anyway to adhere to the Dependency Inversion Principle).

So your registration would become something like this:

container.RegisterSingleton<IMyFactory, MyFactory>();

And the implementation like this:

private sealed class MyFactory : IMyFactory
{
    private readonly Container container;

    public MyFactory(Container container)
    {
        this.container = container;
    }

    public IMyWorker CreateInstance(WorkerType workerType)
    {
        if (workerType == WorkerType.A)
              return this.container.GetInstance<IWorkerA>();

        return this.container.GetInstance<IWorkerB>();
    }
}
Steven
  • 166,672
  • 24
  • 332
  • 435
  • Thanks for this. You are right it was trying to avoid the service locator that produced this situation. What you suggest makes sense. – Ian1971 Nov 13 '13 at 20:02
  • 2
    +1 for clarifying that the factory is part of the composition root. – Facio Ratio Nov 14 '13 at 19:19
  • It will get more interesting when this factory contains business logic (compared to this infrastructure logic). But again the answer to that problem is again simple: split the factory in two classes: only the infrastructural code belongs in the composition root. – Steven Nov 14 '13 at 19:33
  • 1
    -1 I don't think it's right because if somebody looks at the constructor of your factory, it won't tell them much. This factory is like a 'god' class that can resolve and do pretty much anything. Its intention and dependencies are no longer clear. –  Apr 07 '14 at 10:59
  • 1
    @CodeART Not true. The intention of the factory is perfectly clear, it implements IMyFactory, and therefore can create instances of the given `WorkerType`-enum-value. Not a 'god' class at all. The constructor is not visible to any other part of your application, since it is part of the Composition Root. So even if it was a 'god' class, not a single part of your application could see or use it. – Maarten Apr 07 '14 at 14:06
  • Ok, thank you for this. So far I was able to get away with having container in a composition root and it worked really well. I'm also worried about sending wrong message to developers - they might interpret this as "hey, it's fine to inject and use container all over the place" - I really don't want that. Finally it almost hides complexity as anybody looking at the constructor won't instantly know that it has four or more dependencies that get resolved further down the line. –  Apr 07 '14 at 14:21
  • 1
    @CodeART: I agree with you on being careful with sending the wrong message. That's why I referred to [this post by Mark Seemann](http://blog.ploeh.dk/2011/08/25/ServiceLocatorrolesvs.mechanics/) that does a really good job in explaining the difference, but that link might be overlooked. Hopefully this conversation makes it more clearer. – Steven Apr 07 '14 at 14:24
  • @Steven Thank you for the post. In the post, it says that "A DI container encapsulated in a Composition Root is not a Service Locator", but here we are talking about a factory which is not a composition root. Also from the unit testing perspective, I'm not keen on bringing IContainer (structuremap) into the unit testing project as it doesn't belong there. Now to unit test this, I have to stub IContainer when previously I could just stub/mock the dependencies that I pass into constructor. –  Apr 07 '14 at 14:33
  • @CodeART: But that factory *implementation* can be encapsulated in the Composition Root, in whcih case it is *not* a Service Locator. That's the point I try to make, and this is what Mark Seeman refers to. – Steven Apr 07 '14 at 14:36
  • @Steven Quote from the book "A COMPOSITION ROOT is a (preferably) unique location in an application where modules are composed together.". Surely by passing container into a factory we now have another composition root. –  Apr 07 '14 at 14:41
  • @CodeART: The Composition Root is not one method or one class. You should see the Composition Root as a layer; a thin layer, but still a layer. This layer may contain multiple infrastructural classes. That factory implementation is part of the infrastructure. You can create it as private nested class inside your Bootstrapper class and this way it is part of your composition root. – Steven Apr 07 '14 at 14:44
  • @Steven Bootstrapper normally lives in the application layer (global.asax, App.xaml.cs OnStartup etc). The factory might not even live in the application layer, so having factory logic in the composition root doesn't seem right to me. Either way, I'll give this a go. –  Apr 07 '14 at 14:57
  • @CodeART: It's usual to place the Composition Root in the same assembly as your MVC project is, but the Composition Root is still a different layer (multiple layers in the same assembly); Please [read this](https://stackoverflow.com/a/9505530/264697). And the factory abstraction should definately *not* be in the Composition Root, but your implementation should. You will often have more implementations in your composition root for abstractions defined in your base layer. Take for instance an `IUserContext` with an `MvcUserContext` and `WcfUserContext` implementations. – Steven Apr 07 '14 at 15:03
  • @Steven A bit more food for thought. In the book, Mark mentioned constructor over-injection which makes perfect sense. Solution to this is facade like implementation where we can logically group dependencies. By passing the container, don't we achieve the same thing as the facade? Facade seems like a more logical solution to this as by looking at the facade I can instantly see what dependencies it is grouping. –  Apr 07 '14 at 15:28
  • 3
    +1 For more information, I cover this particular topic in my [Implementing an Abstract Factory post](http://blog.ploeh.dk/2012/03/15/ImplementinganAbstractFactory). – Mark Seemann Apr 07 '14 at 15:39
  • @CodeART True, but a facade or aggregate is somethig else than a factory. A facade or aggregate will probably have logic, while the factory just news up stuff. If the factory creates 20 different types, you will have one hell of a constructor. That's when you use the container. – Steven Apr 07 '14 at 15:40
  • @Steven That makes more sense now - my factory doesn't just new up objects. It fetches data from other sources as well as initialises some of the properties. I should have started with this. Once again, thank you very much! –  Apr 07 '14 at 15:41
  • @CodeART, In that case you might be talking about 2 separate abstractions, where the factory part is inside your CR and the business logic part is in your application. – Steven Apr 07 '14 at 16:03
5

This is something of a classic question.

Both of your solutions could be problematic if the number of different implementations increases, especially if the dependencies for each of the implementations have a lot of variance. You could end up with a constructor that takes 20 parameters.

My preferred implementation is to just have the factory class reference the container, and resolve the needed instance that way.

Some might argue that this is no better than the Service Locator anti-pattern, but I don't feel like there is a perfect solution to this problem, and doing it this way seems the most natural to me.

Phil Sandler
  • 27,544
  • 21
  • 86
  • 147
  • Funnily enough, this is the way I was doing it until I read about the anti-pattern so I was trying to find a way of losing that container reference. Then I ran into this issue. – Ian1971 Nov 13 '13 at 19:56
  • I've gone around and around this issue myself, and tried it other ways, but this is the one I've come back to, and I'm perfectly comfortable with it. – Phil Sandler Nov 13 '13 at 20:01
  • Thanks Phil. I think with @Steven's reply as well this makes me more comfortable, and helps justify the container reference. – Ian1971 Nov 13 '13 at 20:05
  • how do I reference the container from the factory if the container is in the app assembly and the factory in the viewmodels one? – Sergio Nov 13 '13 at 20:52
  • One way to do it would be to have the interface in the viewModel assembly and the implementation in the app assembly. Only the app assembly should have any knowledge of the container. – Phil Sandler Nov 13 '13 at 21:57
  • what if I place container and interface in a separate assembly, that can be referenced from the VM one in order to reference the interface from VMs or Services and the use the container in the app project? – Sergio Nov 13 '13 at 22:07
  • 1
    I'm not sure I understand your setup. Maybe start a new question? – Phil Sandler Nov 13 '13 at 22:13
3

The solution depends highly on the lifetime of the two dependencies in my opinion. Can these dependencies be shared across objects, then You can register them in the container and pass them to the factory and reuse them. If every "product" of the factory should have it's own instance, then maybe You could consider building a separate factory for those dependencies (of course if they belong to the same “family” of objects) and pass it to Your factory, and ask for an instance whenever You are creating an instance of IMyWorker. Alternatively You could consider using a Builder instead of a Factory for creating each dependency before creating the end product – IMyWorker in Your case.

Passing the container is considered a code smell unless you are implementing the composition root like for instance in WCF.

If You end up with a factory taking many dependencies in the constructor then you should see it as a hint, that something is wrong - most likely something is violating the Single Responsibility Principle – it simply knows too much ;)

A very good book talking about Dependency Injection is the book “Dependency Injection in .NET” by Mark Seemann which I recommend :)

1

While this question is subjective (the answer will be as well), I would say that your first approach is the appropriate one.

When you utilize Dependency Injection you have to understand what is an actual dependency. In this case, WorkerA and WorkerB are not really dependencies, but clearly Dependency1 and Dependency2 are dependencies. In a real-world scenario, I have used this pattern in my Micrsoft Prism applications.


Hopefully an example of my application will give you a better understanding of the pattern to use. I utilized the ILoggerFacade dependency. I had some view-models that resided in a separate assembly (the factory resided in that assembly as well). My individual IPlayerViewModels were not dependencies (which is why I didn't go the second route).

ShellViewModel.cs:

[Export]
public sealed class ShellViewModel : NotificationObject
{
    public ShellViewModel()
    {
         Players = new ObservableCollection<IPlayerViewModel>();

         // Get the list of player models
         // from the database (ICollection<IPlayer>)
         var players = GetCollectionOfPlayerModels();

         foreach (var player in players)
         {
             var vm = PlayerViewModelFactory.Create(player);

             Players.Add(vm);
         }
    }

    [Import]
    private IPlayerViewModelFactory PlayerViewModelFactory { get; set; }

    public ObservableCollection<IPlayerViewModel> Players { get; private set; }
}

IPlayerViewModelFactory.cs

public interface IPlayerViewModelFactory
{
    IPlayerViewModel Create(IPlayer player);
}

IPlayer.cs

public interface IPlayer
{
    // Sport Enum
    Sport Sport { get; set; }
}

Separate assembly / PlayerViewModelFactory.cs

[Export]
public sealed class PlayerViewModelFactory : IPlayerViewModelFactory
{
    [Import]
    private ILoggerFacade Logger { get; set; }

    public IPlayerViewModel Create(IPlayer player)
    {
        switch (player.Sport)
        {
            case Sport.Basketball:
                return new BasketballViewModel(Logger, player);

            case Sport.Football:
                return new FootballViewModel(Logger, player);

            // etc...

            default:
                throw new ArgumentOutOfRangeException("player");
        }
    }
}
myermian
  • 31,823
  • 24
  • 123
  • 215
  • Thanks for this. My worry is that I could end up with a lot of dependencies in the factory. I think Phil & Steven's answer solves that issue. – Ian1971 Nov 13 '13 at 20:07