Factory class is not supporting SOLID principle

Question

I have code as shown below

 public interface ICar
{
    void Created();
}

public class BigCar : ICar
{
    public void Created()
    {

    }
}

public class SmallCar : ICar
{
    public void Created()
    {

    }
}


public class LuxaryCar : ICar
{
    public void Created()
    {

    }
}

public class CarFactory
{
    public ICar CreateCar(int carType)
    {
        switch (carType)
        {
            case 0:
                return new BigCar();
            case 1:
                return new SmallCar();
            case 2:
                return new LuxaryCar();
            default:
                break;
        }
        return null;
    }
}

In this code I have a factory which is returning the concrete instances. But every time I need to have a new implementation of the ICar interface, I have to change the CreateCar() method of the CarFactory. It seems like I am not supporting the Open Closed Principle of the SOLID principles. Please suggest is there a better way to handle this scenario.

Answer 1

You probably want to make it configurable, like this:

void Main()
{
    // configurable array
    var factories = new ICarFactory[] { new BigCarFactory() };

    // create factory
    var realfactory = new CarFactory(factories);

    // create car
    var car = realfactory.CreateCar(0);
}

public class CarFactory : ICarFactory
{
    private ICarFactory[] _factories;

    public CarFactory (ICarFactory[] factories)
    {
        _factories = factories;

    }   
    public ICar CreateCar(int carType)
    {
        return _factories.Where(x=>x.SupportCar(carType)).First().CreateCar(carType);
    }

    public bool SupportCar(int type) => _factories.Any(x=>x.SupportCar(type));
}

public interface ICarFactory
{
    ICar CreateCar(int type);
    bool SupportCar(int type);
}

public class BigCarFactory : ICarFactory
{
    public ICar CreateCar(int carType)
    {
        if(carType != 0) throw new NotSupportedException();
        return new BigCar();
    }

    public bool SupportCar(int type) => type == 0;
}


public interface ICar
{
    void Created();
}

public class BigCar : ICar
{
    public void Created()
    {

    }
}

public class SmallCar : ICar
{
    public void Created()
    {

    }
}


public class LuxaryCar : ICar
{
    public void Created()
    {

    }
}

Answer 2

There is a lot of confusion around the open/closed principle, and what it should mean to your code.

The fact is here, you're fine. Moving to a config file just divides the code from the data and hides the intent, which will lead to problems.

The answer? It depends. Be pragmatic. "Just f**king hardcode it". Make the intent of the factory clear by keeping your switch statement. Keep the data and the intent close together, as here. Make sure you start by writing a test for missing switch cases and always include a default case (even if it just throws an exception).

Your factory only has one reason to change, and only does one job. Your design as you have it is just fine for now. If you find you are regularly coming back, adding new cases, then perhaps you'll want to think about some other way around it (a config file is not the answer, it's just moving the editing to a config file from a code file) and refactor your solution. Ever-increasing switch statements are not good, but rather than figuring out how to do that a different way, spend effort trying to remove that problem in the first place. See addendum below:

Addendum

Another way out of this is to not have multiple classes implementing the same interface. Think about the other SOLID principles. Perhaps there is another consistency or functional boundary rather than "types of car"? Perhaps there are groups of functionality that could be specified in more, specific, fine-grained interfaces, and then maybe you use DI to have the implementations injected where you need them, rather than having to use a factory in the first place (which, let's face it, is just a trumped-up constructor anyway). See also: "favouring composition over inheritance".