64

I am familiar with these patterns but still don't know how to handle following situation:

public class CarFactory
{
     public CarFactory(Dep1,Dep2,Dep3,Dep4,Dep5,Dep6)
     {
     }

     public ICar CreateCar(type)
     {
            switch(type)
            {
               case A:
                   return new Car1(Dep1,Dep2,Dep3);
               break;

               case B:
                   return new Car2(Dep4,Dep5,Dep6);
               break;
            }
     }
}

In general the problem is with amount of references that needs to be injected. It will be even worse when there are more cars.

First approach that comes to my mind is to inject Car1 and Car2 in factory constructor but it is against factory approach because factory will return always the same object. The second approach is to inject servicelocator but it's antipattern everywhere. How to solve it?

Edit:

Alternative way 1:

public class CarFactory
{
     public CarFactory(IContainer container)
     {
        _container = container;
     }

     public ICar CreateCar(type)
     {
            switch(type)
            {
               case A:
                   return _container.Resolve<ICar1>();
               break;

               case B:
                     return _container.Resolve<ICar2>();
               break;
            }
     }
}

Alternative way 2 (too hard to use because of too many of dependencies in tree):

public class CarFactory
{
     public CarFactory()
     {
     }

     public ICar CreateCar(type)
     {
            switch(type)
            {
               case A:
                   return new Car1(new Dep1(),new Dep2(new Dep683(),new Dep684()),....)
               break;

               case B:
                    return new Car2(new Dep4(),new Dep5(new Dep777(),new Dep684()),....)
               break;
            }
     }
}
NightOwl888
  • 55,572
  • 24
  • 139
  • 212
MistyK
  • 6,055
  • 2
  • 42
  • 76
  • 1
    A quick thought is to make a mapping class which takes a `type` as input, and returns the three `Dep#` you need. Then you can map all dependencies, into an instance of the mapping class in the bootstrapper, and then inject the mapping instance into the factory. – Anders Aug 11 '15 at 19:48
  • 1
    I think there is nothing wrong with `Alternative way 1` that you showed when implementation of that factory belongs to `Composition Root`. You shouldn't register DI container itself in your DI container – Arkadiusz K Aug 11 '15 at 20:56
  • What do you mean it belongs to Composition Root? Could you provide some code examples? – MistyK Aug 11 '15 at 21:02
  • Composition Root is place where you wire your's application DI container. So, for example this can be a `Bootstrap` class. – Arkadiusz K Aug 12 '15 at 05:31

6 Answers6

103

Having a switch case statement inside of a factory is a code smell. Interestingly, you don't seem to be focusing on solving that issue at all.

The best, most DI friendly solution for this scenario is the strategy pattern. It allows your DI container to inject the dependencies into the factory instances where they belong, without cluttering up other classes with those dependencies or resorting to a service locator.

Interfaces

public interface ICarFactory
{
    ICar CreateCar();
    bool AppliesTo(Type type);
}

public interface ICarStrategy
{
    ICar CreateCar(Type type);
}

Factories

public class Car1Factory : ICarFactory
{
    private readonly IDep1 dep1;
    private readonly IDep2 dep2;
    private readonly IDep3 dep3;
    
    public Car1Factory(IDep1 dep1, IDep2 dep2, IDep3 dep3)
    {
        this.dep1 = dep1 ?? throw new ArgumentNullException(nameof(dep1));
        this.dep2 = dep2 ?? throw new ArgumentNullException(nameof(dep2));
        this.dep3 = dep3 ?? throw new ArgumentNullException(nameof(dep3));
    }
    
    public ICar CreateCar()
    {
        return new Car1(this.dep1, this.dep2, this.dep3);
    }
    
    public bool AppliesTo(Type type)
    {
        return typeof(Car1).Equals(type);
    }
}

public class Car2Factory : ICarFactory
{
    private readonly IDep4 dep4;
    private readonly IDep5 dep5;
    private readonly IDep6 dep6;
    
    public Car2Factory(IDep4 dep4, IDep5 dep5, IDep6 dep6)
    {
        this.dep4 = dep4 ?? throw new ArgumentNullException(nameof(dep4));
        this.dep5 = dep5 ?? throw new ArgumentNullException(nameof(dep5));
        this.dep6 = dep6 ?? throw new ArgumentNullException(nameof(dep6));
    }
    
    public ICar CreateCar()
    {
        return new Car2(this.dep4, this.dep5, this.dep6);
    }
    
    public bool AppliesTo(Type type)
    {
        return typeof(Car2).Equals(type);
    }
}

Strategy

public class CarStrategy : ICarStrategy
{
    private readonly ICarFactory[] carFactories;

    public CarStrategy(ICarFactory[] carFactories)
    {
        this.carFactories = carFactories ?? throw new ArgumentNullException(nameof(carFactories));
    }
    
    public ICar CreateCar(Type type)
    {
        var carFactory = this.carFactories
            .FirstOrDefault(factory => factory.AppliesTo(type));
            
        if (carFactory == null)
        {
            throw new InvalidOperationException($"{type} not registered");
        }
        
        return carFactory.CreateCar();
    }
}

Usage

// I am showing this in code, but you would normally 
// do this with your DI container in your composition 
// root, and the instance would be created by injecting 
// it somewhere.
var strategy = new CarStrategy(new ICarFactory[] {
    new Car1Factory(dep1, dep2, dep3),
    new Car2Factory(dep4, dep5, dep6)
    });

// And then once it is injected, you would simply do this.
// Note that you could use a magic string or some other 
// data type as the parameter if you prefer.
var car1 = strategy.CreateCar(typeof(Car1));
var car2 = strategy.CreateCar(typeof(Car2));

Note that because there is no switch case statement, you can add additional factories to the strategy without changing the design, and each of those factories can have their own dependencies that are injected by the DI container.

var strategy = new CarStrategy(new ICarFactory[] {
    new Car1Factory(dep1, dep2, dep3),
    new Car2Factory(dep4, dep5, dep6),
    new Car3Factory(dep7, dep8, dep9)
    });
    
var car1 = strategy.CreateCar(typeof(Car1));
var car2 = strategy.CreateCar(typeof(Car2));
var car3 = strategy.CreateCar(typeof(Car3));
NightOwl888
  • 55,572
  • 24
  • 139
  • 212
  • Wow, I didn't think about it! Letting the factory decide is a great idea. That's my favourite answer! – MistyK Aug 12 '15 at 17:40
  • 4
    But isn't using the new operator considered as a no-go when it comes to IoC? Could the factories be implemented differently (without using new and without resorting to the service locator pattern)? – Boris May 10 '16 at 09:36
  • 4
    When it comes to IoC, one use of the abstract factory pattern is for abstacting the `new` operator out of the rest of the code to isolate it, making it DI friendly. This makes it loosely coupled from the code that uses the factory. There is no requirement that you have to go back to the container to resolve the instance - in fact, this will perform much faster than using the container. That said, injecting the container into abstract factory is a common technique to [allow 3rd parties to integrate with a framework](http://blog.ploeh.dk/2014/05/19/di-friendly-framework/) using DI. – NightOwl888 May 10 '16 at 19:23
  • 1
    @Boris - See also [Is there a pattern for initializing objects created via a DI container](http://stackoverflow.com/questions/1943576/is-there-a-pattern-for-initializing-objects-created-via-a-di-container/1945023#1945023) and [Why do we need Abstract factory design pattern?](http://stackoverflow.com/questions/2280170/why-do-we-need-abstract-factory-design-pattern) for more information about using Abstract Factory in conjunction with DI. – NightOwl888 May 10 '16 at 19:44
  • 1
    I'm working on a similar solution based on an enum. Where you have a method `Create(Type)` I've got `Create(SomeEnum)`. The thing that troubles me is that this seems to present an unwanted unknown precondition when calling Create. If I add an item to the `enum SomeEnum` without adding the corresponding factory a client could call `Create(SomeEnum.SomeNewValue)` and cause an exception violating encapsulation as outlined here http://blog.ploeh.dk/2015/10/26/service-locator-violates-encapsulation/ – Craig Oct 09 '17 at 16:51
  • 1
    @Craig - I wouldn't recommend using an `enum` for strategy selection. Not just because it violates encapsulation, but when you use `enum` the code has to change in 2 different places when adding a new service to the strategy. If you look at the implementation above, both the logic on how to select the factory and the implementation of the factory are in the same class. You can add or remove one without changing any other class. Also, with a slight change, you could make a strategy run multiple operations from a much larger set as in [this example](https://stackoverflow.com/a/34331154). – NightOwl888 Oct 09 '17 at 17:06
  • @Craig - Typically, I use string as the data type for a strategy. I only used `System.Type` in this example because it is more along the lines of what you would expect when using a service locator and the OP had the type handy to select with. `enum` is a non-starter because it automatically drags in an unwanted type dependency - and the dependency goes both ways since the enum could potentially have items that are not registered with the strategy. – NightOwl888 Oct 09 '17 at 17:09
  • Thanks, I see your point. I had chosen to use the enum as it was a part of the implementation in this existing code-base I'm trying to do some refactoring on. Looks like I should taking a look at refactoring from an earlier point. Need to put some more thought into this. – Craig Oct 09 '17 at 17:53
  • 18
    Your CarStrategy isn't really a strategy, but an AbstractFactory. And you are switching/branching through the FirstOrDefault method. Branching code in a factory **is not a code smell**. Branching statements when creating objects are the raison d'etre for the Factory Method/Abstract Factory patterns. – Alexander Pope Nov 09 '18 at 20:16
  • 1
    @AlexanderPope `CarStrategy` combines strategy AND factory. Abstract fatories allow to produce families of classes, not to decide which one to use. It just forwards the call to the injected abstract factory to produce the right type of item. – gfache Sep 30 '19 at 10:00
  • @gfache Strategy is a behavioral pattern while Factory is a creational one. From a pure implementation perspective, they look the same. A lot of patterns are a variation of the same theme, the name comes from how you use them. Given that the CarStrategy creates an object, it is actually a factory. – Alexander Pope Oct 08 '19 at 14:35
  • 2
    It's hilarious to me that you poo-poo the idea of branching code in a factory when finding the correct implementation, yet use FirstOrDefault to find yours... which is exactly the same thing. The improvement here is that you're taking in all implementations via DI, which is good, but not much else. – Ed S. Jan 31 '20 at 17:15
  • 1
    No, it isn't the same. The above code requires only 1 class to change when adding or removing a factory from the strategy. If a switch case were involved the strategy class would always need to change in addition to the class that is added or removed. There is nothing *technically* wrong with using a switch case statement, but it causes extra undesired maintenance - which is why it is considered a code smell. – NightOwl888 Feb 02 '20 at 17:37
  • 1
    please show how this would be registered via DI in startup – Seabizkit May 04 '20 at 18:19
  • 1
    Type checking is more of a code smell than branching. – Colm Jul 09 '21 at 09:03
  • @Colm - The fact that the above implementation uses type checking for the comparison is only because the OP's question included `type` as a parameter. There is nothing about the strategy pattern that limits the parameter types of the `AppliesTo()` method or the expression used. It could just as well be an `int` that corresponds to a constant. – NightOwl888 Jul 09 '21 at 10:16
16

Answering your comment about code example with Composition Root. You can create following and this is not a Service Locator.

public class CarFactory
{
    private readonly Func<Type, ICar> carFactory;

    public CarFactory(Func<Type, ICar> carFactory)
    {
       this.carFactory = carFactory;
    }

    public ICar CreateCar(Type carType)
    {
        return carFactory(carType);
 }

and this is how look your Composition Root using Unity DI container :

Func<Type, ICar> carFactoryFunc = type => (ICar)container.Resolve(type);
container.RegisterInstance<CarFactory>(new CarFactory(carFactoryFunc));
CJBS
  • 15,147
  • 6
  • 86
  • 135
Arkadiusz K
  • 1,797
  • 1
  • 17
  • 18
2

I answered a similar question some time ago. Basically it's all about your choice. You have to choose between verbosity (which gives you more help from a compiler) and automation, which allows you to write less code but is more prone to bugs.

This is my answer supporting verbosity.

And this is also a good answer that supports automation.

EDIT

I believe the approach you consider wrong is actually the best. Truth being said, usually there won't so many dependencies in there. I like this approach because it's very explicit and rarely results in runtime errors.

Alternative way 1:

This one is bad. It's actually a service locator, which is considered an anti-pattern.

Alternative way 2

Like you wrote, it's not easy to use if mixed with IOC containter. However in some case a similar approach (poor man's DI) can be useful.

All in all, I wouldn't bother having "many" dependencies in your factories. It's a simple, declarative code. It takes seconds to write and can save you hours of struggling with runtime errors.

Community
  • 1
  • 1
Andrzej Gis
  • 13,706
  • 14
  • 86
  • 130
  • Your solution seems to be example from my question which I consider as wrong (at the top), am I right? – MistyK Aug 11 '15 at 21:25
  • I like it mostly but there is one problem with it. What happens if Dep1 contains state? Every time I create car I get different instance of Car but the same Dep1 which may cause some problems. – MistyK Aug 11 '15 at 21:47
  • @Zbigniew How about passing Dep1Factory instead of Dep1? – Andrzej Gis Aug 11 '15 at 21:50
  • It would solve that problem. I am not sure which approach I will choose but thank you for help :) – MistyK Aug 11 '15 at 21:53
  • @gisek: the reason #1 is bad is not because it is the SL as it is not. This is a perfect example of the **Local Factory** pattern (aka Dependency Resolver) which doesn't have the problem the SL has - you can't misuse it. The reason #1 is bad is because it introduces an unwanted and improper dependency to an infrastructure class from a domain class. – Wiktor Zychla Aug 12 '15 at 13:17
  • @WiktorZychla Can you elaborate on why it's not a SL? Btw. I kind of like your implementation of Local Factory. I haven't seen it before, but it seems to be useful in some cases. Grab a plus ;) – Andrzej Gis Aug 12 '15 at 14:02
  • @gisek: SL has an open contract that allows one to create barely anything without control. This one has a narrow, precise contract that allows creation of the very specific type. Local Factory takes the Abstract Factory as a starting point but instead of creating multiple concrete implementations, you just have an injectable provider which is configurable from the Composition Root. – Wiktor Zychla Aug 12 '15 at 14:31
  • @WiktorZychla From #1: "_container.Resolve()" - this looks exactly like "open contract that allows one to create barely anything without control". Am I not getting something? – Andrzej Gis Aug 12 '15 at 14:36
  • @WiktorZychla I guess there's a misunderstanding. When talking about Service Locator I was referring to this specific usage of the IContainer. Not the CarFactory as whole. – Andrzej Gis Aug 12 '15 at 15:12
  • @gisek: the container is not exposed from the factory. Also, the factory doesn't allow you to misuse it. Thus, it is not the sl. The way the container is used doesn't matter. A class is a sl or not because of its contract not the implementation. – Wiktor Zychla Aug 12 '15 at 15:49
  • @WiktorZychla Yes, you are right. Again, I was **not** referring to the class. I didn't like the idea that the factory uses SL. Which is basically what you wrote in your first comment. – Andrzej Gis Aug 12 '15 at 15:54
  • @gisek: it doesn't use the sl, it uses the container. Following your point here, every possible use of a container could be called the sl. And you surely know this is not the case. – Wiktor Zychla Aug 12 '15 at 16:56
  • @WiktorZychla Nope, not **every** use. In my opinion passing the container outside the composition root makes is a SL. – Andrzej Gis Aug 12 '15 at 18:34
  • @gisek: I get your point and could agree. Passing bare container outside CR is an antipattern. I never called it SL though. – Wiktor Zychla Aug 12 '15 at 19:19
1

First, you have a concrete factory, an IoC container could be an alternative rather than something to help you there.

Then, just refactor the factory to not to expect a full possible parameter list in the factory constructor. This is the primary issue - why are you passing so many parameters if the factory method doesn't need them?

I would rather pass specific parameters to the factory method

public abstract class CarFactoryParams { }

public class Car1FactoryParams : CarFactoryParams
{
   public Car1FactoryParams(Dep1, Dep2, Dep3) 
   { 
      this.Dep1 = Dep1;
      ...
}

public class Car2FactoryParams 
      ...

public class CarFactory
{
    public ICar CreateCar( CarFactoryParams params )
    {
        if ( params is Car1FactoryParams )
        {
            var cp = (Car1FactoryParams)params;
            return new Car1( cp.Dep1, cp.Dep2, ... );
        }
        ...
        if ( params is ...

By encapsulating the parameter list in a specific class you just make the client provide exactly these parameters that are required for specific factory method invocation.

Edit:

Unfortunately, it was not clear from your post what are these Dep1, ... and how you use them.

I suggest following approach then that separates the factory provider from actual factory implementation. This approach is known as the Local Factory pattern:

public class CarFactory
{
   private static Func<type, ICar> _provider;

   public static void SetProvider( Func<type, ICar> provider )
   {
     _provider = provider;
   }

   public ICar CreateCar(type)
   {
     return _provider( type );
   }
}

The factory itself doesn't have any implementation, it is here to set the foundation to your domain API, where you want your car instances to be created with this API only.

Then, in the Composition Root (somewhere near the starting point of the app where you configure your actual container), you configure the provider:

CarFactory.SetProvider(
    type =>
    {
        switch ( type )
        {
           case A:
             return _container.Resolve<ICar1>();
           case B:
             return _container.Resolve<ICar2>();
           ..
    }
);

Note that this example implementation of the factory's provider uses a delegate but an interface could also be used as a specification for an actual provider.

This implementation is basically #1 from your edited question, however, it doesn't have any particular downsides. The client still calls:

var car = new CarFactory().CreareCar( type );
NightOwl888
  • 55,572
  • 24
  • 139
  • 212
Wiktor Zychla
  • 47,367
  • 6
  • 74
  • 106
  • These dependencies are objects registered in IOC -> services, builders, readers etc. not value objects. CarFactoryParams sounds good for dataholder objects not for dependencies that actually do something. I want to get them from container that's why I wanted to inject them with constructor. Ioc would be helpful to create these instances for me using dependencies declared in Ioc container. – MistyK Aug 11 '15 at 20:37
  • @Zbigniew: edited my answer as you provided more details. – Wiktor Zychla Aug 12 '15 at 13:24
1

I would consider giving the dependencies a good structure so you can utilize something similar to Wiktor's answer, but I would abstract the Car factory itself. Then, you don't use the if..then structure.

public interface ICar
{
    string Make { get; set; }
    string ModelNumber { get; set; }
    IBody Body { get; set; }
    //IEngine Engine { get; set; }
    //More aspects...etc.
}

public interface IBody
{
    //IDoor DoorA { get; set; }
    //IDoor DoorB { get; set; }
    //etc
}

//Group the various specs
public interface IBodySpecs
{
    //int NumberOfDoors { get; set; }
    //int NumberOfWindows { get; set; }
    //string Color { get; set; }
}

public interface ICarSpecs
{
    IBodySpecs BodySpecs { get; set; }
    //IEngineSpecs EngineSpecs { get; set; }
    //etc.
}

public interface ICarFactory<TCar, TCarSpecs>
    where TCar : ICar
    where TCarSpecs : ICarSpecs
{
    //Async cause everything non-trivial should be IMHO!
    Task<TCar> CreateCar(TCarSpecs carSpecs);

    //Instead of having dependencies ctor-injected or method-injected
    //Now, you aren't dealing with complex overloads
    IService1 Service1 { get; set; }
    IBuilder1 Builder1 { get; set; }
}

public class BaseCar : ICar
{
    public string Make { get; set; }
    public string ModelNumber { get; set; }
    public IBody Body { get; set; }
    //public IEngine Engine { get; set; }
}

public class Van : BaseCar
{
    public string VanStyle { get; set; } 
    //etc.
}

public interface IVanSpecs : ICarSpecs
{
    string VanStyle { get; set; }
}

public class VanFactory : ICarFactory<Van, IVanSpecs>
{
    //Since you are talking of such a huge number of dependencies,
    //it may behoove you to properly categorize if they are car or 
    //car factory dependencies
    //These are injected in the factory itself
    public IBuilder1 Builder1 { get; set; }
    public IService1 Service1 { get; set; }

    public async Task<Van> CreateCar(IVanSpecs carSpecs)
    {
        var van = new Van()
        {
           //create the actual implementation here.
        };
        //await something or other
        return van;
    }
}

I didn't list it, but you can implement multiple types of cars and their corresponding factories now and use DI to inject whatever you need.

  • Please read my comment to Viktor answer. Dep1, Dep2.. are not data objects, that are services, builders etc.. – MistyK Aug 11 '15 at 20:43
  • In my interpretation of his response, he is simply saying to create a parameters object (Specs in my example). It doesn't matter if they are value objects or not. I think he and I are both trying to organize dep1, dep2, etc, without knowing the specifics of what those dependencies are. The primary difference with my approach is that I would not only abstract these params objects, I would abstract the factories themselves, so you can use DI for them instead of the switch statement in both yours and his examples. –  Aug 11 '15 at 20:46
  • I've added where the services, readers, etc you were talking about would go. Those sound less like car dependencies and more like car factory dependencies. –  Aug 11 '15 at 21:07
0

Many DI containers support the notion of named dependencies.

E.g. (Structuremap syntax)

For<ICar>().Use<CarA>().Named("aCar");
Container.GetNamedInstance("aCar") // gives you a CarA instance

If you use something like a convention, a rule how the name is derived from the concrete car type itself, you have a situation where you don't need to touch the factory anymore when you extend the system.

Using this in a factory is straightforward.

class Factory(IContainer c) {
  public ICar GetCar(string name) {
    Return c.GetNamedInstance(name);
  }
}
flq
  • 22,247
  • 8
  • 55
  • 77
  • I see your point but it is not exactly what I am asking for. In general I don't feel that using IContainer dependency is appropriate in factory.If it is then problem is solved because it doesn't make a difference if I use named dependencies or I use switch statement using Resolve. It solves problem of creating interfaces for all resolved object because when I name them I can get rid of them. But the biggest problem here is IContainer as dependency. – MistyK Aug 11 '15 at 21:01
  • In my book, factories are pretty much the only place where I'd allow a direct dependency on the container, as they are usually infrastructure-related. – flq Aug 11 '15 at 21:46
  • @flq: still, you could have configurable internal provider in the factory which could be configured from the Composition Root. This way the factory would be unaware of a possible configuration where a container is used. On the other hand, having the container dependency in your factory constructor sounds like an antipattern. Factories are important part of the API, their implementation is usually infrastructure-related, the API should not be, imho. – Wiktor Zychla Aug 12 '15 at 13:00