A Factory Pattern that will satisfy the Open/Closed Principle?

Question

I have the following concrete Animal products: Dog and Cat.

I am using a parameterized Factory method to create said products. Depending on the AnimalInfo parameter that is passed to the Factory method, a concrete product will be created. The mapping logic is placed in the Factory method.

Here is my code:

 public abstract class AnimalInfo
    {
        public abstract String Sound { get; }
    }

    public class DogInfo : AnimalInfo
    {
        public override string Sound
        {
            get { return "Bark"; }
        }
    }

    public class CatInfo : AnimalInfo
    {
        public override string Sound
        {
            get { return "Meow"; }
        }
    }

    public abstract class Animal
    {
        public abstract void Talk();
    }

    public class Dog : Animal
    {
        private readonly DogInfo _info;

        public Dog(DogInfo aInfo)
        {
            _info = aInfo;
        }

        public override void Talk()
        {
            Console.WriteLine(_info.Sound);
        }
    }

    public class Cat : Animal
    {
        private readonly CatInfo _info;

        public Cat(CatInfo aInfo)
        {
            _info = aInfo;
        }

        public override void Talk()
        {
            Console.WriteLine(_info.Sound);
        }
    }

Here's my Factory method with its logic:

public static class AnimalFactory
{
    public static Animal CreateAnimal(AnimalInfo aInfo)
    {
        if (aInfo is DogInfo)
            return new Dog(aInfo as DogInfo);
        if (aInfo is CatInfo)
            return new Cat(aInfo as CatInfo);
        return null;
    }
}

The problem I'm seeing here is that the Factory method itself violates the Open/Closed principle in such a way that if I add a new Animal, I will need to modify the Factory method to reflect the new mapping.

Is there a way to make the creation more "dynamic" via reflection? More importantly, is there any anti-pattern in my design?

Garb

Is AnimalInfo intended to be unique per product, or per instance of product? – Bathelda

Out of interest, why not just do it all through configuration using an IoC container #2515624? – Unsuspected

@MarkH I believe it's unique per instance of a product. – Garb

@nonnb We thought about that but decided that an IoC container is an overkill for this. – Garb

Dynamic factory is good. Sometimes, for security or other reasons, it's ok to have the factory check what it's going to instantiate. But in either case, the client of 'Animal' is closed to the changes in Animals. OCP requires a reference of what's open and whaat's closed. – Clausius

Answer 1

The easy way out is to make AnimalInfo itself the factory:

public abstract class AnimalInfo<T> where T: Animal
{
    public abstract String Sound { get; }
    public abstract T CreateAnimal();
}

Implementation for DogInfo:

public class DogInfo : AnimalInfo<Dog>
{
    public override string Sound
    {
        get { return "Bark"; }
    }

    public override Dog CreateAnimal()
    {
        return new Dog(this);
    }
}

You could keep your current static Factory if you wanted to:

public static class AnimalFactory
{
    public static Animal CreateAnimal(AnimalInfo aInfo)
    {       
        return aInfo.CreateAnimal();
    }
}

Not exactly strict adherance to the Factory pattern, IMO, but no longer violates your open/close principle.

Posology

Not a bad answer. But consider now you have to get your DogInfo from somewhere. What knows how to create and return it versus a CatInfo, for example? Seems to me, this pushes the same "problem" to a different place. Ultimately, something has to know how to get you the appropriate type. (That something could be configuration or code. Nevertheless, that something exists.) – Illconsidered

@Anthony - I didnt doubt that for a moment, and assumed the OP could overcome that issue seperately. I merely answered the primary question of ridding the code of the if..then logic. OP will still need reflection (or some other mechanism) for determining which factory to create. – Posology

@Posology This is a clever solution. I'm marking this as an answer because it is the best solution presented. – Garb

Answer 2

Let me sidestep a bit. The SOLID principles are good. But realize at some point, the principles break down, a fact even the originator of the SOLID term acknowledges. Yes, you want to follow single responsibility, open/closed, etc., but when you do so, something has to know how to create all those things that are otherwise nicely decoupled with single responsibilities.

Think about one of the things Uncle Bob said regarding ifs and switches in your code. "Have it exactly once." It stands to reason that the long if or the switch will indeed be a violation of SRP and OCP. And that's OK, if you have that violation once.

So go ahead, have your

if (a)
   return x;
else if (b)
   return y;
else if (c)
   return z;
else
   throw new InvalidOperationException();

And have it once. Yes, it's a violation of OCP. Yes, it might violate SRP. But something somewhere has to. The key is reducing the number of those somethings and those somewheres.

Illconsidered

What a great answer; too many people are trying to apply principles like OCP as religious dogma. Programming is about being pragmatic, not dogmatic. – Tigre

Answer 3

If you are looking for a reflection based method, you could do something like the following...

public static class AnimalFactory
{
    public static Animal CreateAnimal(Type animalType)
    {
        return Activator.CreateInstance(animalType) as Animal;
    }

    public static Animal CreateAnimal(string animalType)
    {
        Type type = Assembly.GetExecutingAssembly().GetType(animalType);
        return Activator.CreateInstance(type) as Animal;
    }


}

Merciful

The mapping logic is based on the type of AnimalInfo not the Animal itself. – Garb