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...


    Have you considered checking NHibernate for your DAL? It has its own constructs for mapping ids to type instances. Apart from that, your generic class shouldn't inherit from the non generic one. Instead, make your Dictionary a member of this class with T as type parameter to avoid casting. Sry for typos, writing this on a phone.

    I think that if you write and run some tests for this, you'll know whether or not it works, with far more confidence than you'll ever get from the well-meaning words of random strangers.

    Hi David, of course I tested it. it works. I'm asking if this kind of implementation won't fall to some obscure end-cases or parallelism issues, maybe it will be hard to maintain, maybe it works really slow and there is a faster way to achieve the same thing... anyway, this is why I am asking it here :)

    Any good readon you don't want to use an OR mapper like EntityFramework 4 or NHibernate?

    Unfortunately our main project, which is in production has no ORM, this new project needs to interact with it, and I don't know if it's a good idea to use ORM in satellite projects and not in the main project. and changing to an ORM in the main project will never pass management...

    As long as you keep clear boundaries, there's nothing in the way of using a mapper in a sattelite assembly. The entities can implement the interfaces needed and interact as intended. Give it a go. Especially if you can use .net 4 and EF 4.2. (biased opinion ;) )

    @Groo - what if I still want to keep the abstract Factory for other uses?

    @Mithir: I cannot think of a scenario where it would allow you to do something which a generic one couldn't do. Do you have an example?

    @Groo actually you may be right :) . you should make this an answer

    It looks good to me. It's certainly a perfectly valid way of doing things, and easier to maintain than the alternatives. My earlier comment was because you seemed to be asking whether it was going to work.

    @Groo although I would still need to cast from Activator, it returns an object.

    Ok, I had an hour of spare time waiting for something to finish, so I wrapped up an answer. Yes, maybe I didn't express it clearly enough. I wanted to say it can be merged. I'll post in a minute.

    How did this happen? This question was missing and I asked about it at meta, now it reappeared? Weird...

  • Groo

    Groo Correct answer

    9 years ago

    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);

License under CC-BY-SA with attribution


Content dated before 7/24/2021 11:53 AM