Implementing factory design pattern with generics

In this new project I’m working on I need to create objects on runtime by data from the DB, right now I have two groups of classes, each group implementing a different interface.

I started working on a factory class, which will map id to a type, an abstract one so I can extend with a factory for each group.

The constructor parameter is the type of the interface common to all implementors of the group.

abstract class Factory
   {
    private Dictionary<int, Type> idsToTypes;
    private Type type;

    public Factory(Type _type)
    {
        idsToTypes = new Dictionary<int, Type>();
        type = _type; 
    }

    protected Object GetProduct(int id, params object[] args)
    {
        if (idsToTypes.ContainsKey(id))
        {
            return Activator.CreateInstance(idsToTypes[id], args);
        }
        else
        {
            return null;
        }
    }

    public void RegisterProductType(int id, Type _type)
    {
        if (_type.IsInterface || _type.IsAbstract)
            throw new ArgumentException(String.Format("Registered type, {0}, is interface or abstract and cannot be registered",_type));

        if (type.IsAssignableFrom(_type))
        {
            idsToTypes.Add(id, _type);
        }
        else
        {
            throw new ArgumentException(String.Format("Registered type, {0}, was not assignable from {1}",_type,type));
        }

    }

Then I noticed both extending factories look the same, and in the end with the use of generics I got to this class:

 class GenericSingletonFactory<T> : Factory
    {
        static public GenericSingletonFactory<T> Instance = new GenericSingletonFactory<T>();

    private GenericSingletonFactory()
        : base(typeof(T)) { }

    public T GetObject(int id, params object[] args)
    {
        Object obj = base.GetProduct(id, args);
        if (obj != null)
        {
            T product = (T)obj;
            return product;
        }
        else return default(T);
    }
}

So I can just use like so:

GenericSingletonFactory<ISomething>.Instance.RegisterProductType(1, typeof(ImplementingISomething));
ISomething something = GenericSingletonFactory<ISomething>.Instance.GetObject(1);

It seems ok… but am I missing something here? is this a good way to do this kind of things?
I’m a little weary that this will fall apart on runtime somehow…

Several considerations:

1. Base Factory class does all the work, so I would simply merge it into the generic method.

2. Your singleton instance is stored in a public field. This means any part of your program can easily replace it. You should make it readonly, or (even better) expose it through a readonly property.

3. If you are not abstracting the factory (i.e. exposing the Instance property as an interface), it means there is no way to use any other factory in your program. This has two possible consequences:

   • you can either skip accessing Instance every time and use a plain static method (Factory<T>.Instance.DoStuff(...) becomes Factory<T>.DoStuff(...), which is slightly shorter)

   • or, you can decide that you want to hide the implementation of the factory behind an interface (IFactory<T>, for example), in which case a concrete factory will be stored in an instance of a separate class, which would completely change the call to something like DAL.GetFactory<T>().DoStuff(...) (DoStuff becomes an instance method). This leaves allows greater flexibility, since you can pass an IFactory<T> to a part of code which doesn’t need to know about your static DAL classes. But it would require additional redesign.

4. If you add a new generic parameter to the RegisterProductType method, you can use the where clause to limit the type to derived types at compile time. Getting a compile error is much better than getting a run-time one.

5. Are you sure that you need to pass the constructor parameters to the GetProduct method? This part might be slightly unusual (although there might be cases where you would want to instantiate your classes by passing a parameter). What it there is a specific derived type which accepts different parameters than other types? Activator.CreateInstance indicates that you are forcing all your callers to know which constructor your method will use.

First four points (simplified) would result in something like:

public class Factory<T>
{
    private Factory() { }

    static readonly Dictionary<int, Type> _dict = new Dictionary<int, Type>();

    public static T Create(int id, params object[] args)
    {
        Type type = null;
        if (_dict.TryGetValue(id, out type))
            return (T)Activator.CreateInstance(type, args);

        throw new ArgumentException("No type registered for this id");
    }

    public static void Register<Tderived>(int id) where Tderived : T
    {
        var type = typeof(Tderived);

        if (type.IsInterface || type.IsAbstract)
            throw new ArgumentException("...");

        _dict.Add(id, type);
    }
}

Usage:

Factory<IAnimal>.Register<Dog>(1);
Factory<IAnimal>.Register<Cat>(2);

// this is the "suspicious" part.
// some instances may require different parameters?
Factory<IAnimal>.Create(2, "Garfield");

Fifth point is worth considering. If you change your factory to this:

public class Factory<T>
{
    private Factory() { }

    static readonly Dictionary<int, Func<T>> _dict 
         = new Dictionary<int, Func<T>>();

    public static T Create(int id)
    {
        Func<T> constructor = null;
        if (_dict.TryGetValue(id, out constructor))
            return constructor();

        throw new ArgumentException("No type registered for this id");
    }

    public static void Register(int id, Func<T> ctor)
    {
        _dict.Add(id, ctor);
    }
}

Then you can use it like this:

Factory<IAnimal>.Register(1, () => new Dog("Fido"));
Factory<IAnimal>.Register(2, () => new Cat("Garfield", something_else));

// no need to pass parameters now:
IAnimal animal = Factory<IAnimal>.Create(1);

This delegates all construction logic to init time which allows complex scenarios for different object types. You can, for example, make it return a singleton on each call to Create:

// each call to Factory<IAnimal>.Create(3) should return the same monkey
Factory<IAnimal>.Register(3, () => Monkey.Instance);

I’m not too familiar with C#, so just two general notes.

1. Instead of structures like this:

   if (idsToTypes.ContainsKey(id))
   {
       return Activator.CreateInstance(idsToTypes[id], args);
   }
   else
   {
       return null;
   }
   

   you could write this:

   if (idsToTypes.ContainsKey(id))
   {
       return Activator.CreateInstance(idsToTypes[id], args);
   }
   return null;
   

   The else keyword is unnecessary in these cases.

2. Using guard clauses makes the code flatten and more readable.

   if (!type.IsAssignableFrom(_type)) {
       throw new ArgumentException(String.Format("Registered type, {0}, was not assignable from {1}", _type, type));
   }
   idsToTypes.Add(id, _type);
   

   References: Replace Nested Conditional with Guard Clauses in Refactoring: Improving the Design of Existing Code; Flattening Arrow Code