Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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

share|improve this question
1  
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. –  Groo Jan 26 '12 at 6:52
    
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. –  David Wallace Jan 26 '12 at 7:43
1  
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 :) –  Mithir Jan 26 '12 at 8:13
    
Any good readon you don't want to use an OR mapper like EntityFramework 4 or NHibernate? –  Lars-Erik Jan 26 '12 at 9:15
    
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... –  Mithir Jan 26 '12 at 9:29
show 8 more comments

2 Answers 2

up vote 18 down vote accepted

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);
share|improve this answer
add comment

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

share|improve this answer
    
You might want to add a "return" after insertion into the dictionary ;) –  Willem van Rumpt Jan 26 '12 at 11:50
    
Thank you, @WillemvanRumpt! The example was wrong, I've changed it :) –  palacsint Jan 26 '12 at 12:05
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.