5
\$\begingroup\$

I have a library that defines a bunch of ISprite interfaces, like IPieceSprite and ITileSprite. Then, it defines a global ISpriteFactory that should be supplied to the library upon initialization. ISpriteFactory should create concrete objects based on the interfaces mentioned.

I made this rough implementation, but it makes my skin crawl. What would be a better approach to this problem?

public class SpriteFactoryMock: ISpriteFactory {

    public SpriteType Create<SpriteType>() where SpriteType: ISprite {

        var type = typeof(SpriteType);
        if (type == typeof(ITileSprite))    return (SpriteType)(Object)(new TileSprite());
        if (type == typeof(IPieceSprite))   return (SpriteType)(Object)(new PieceSprite());

        return default(SpriteType);
    }
}

Considering other alternatives, I guess I could create a method for every ISprite interface, and that would work a bit better. I might do that if all hope for the previous approach is lost.

\$\endgroup\$
  • 2
    \$\begingroup\$ Great question. First code review I have posted on and made me think a bit. Cheers. \$\endgroup\$ – Nico Mar 3 '16 at 6:01
5
\$\begingroup\$

There is a very simple way to handle this but that is all dependant on the fact you have only one implementation of each ISprite impelementation per interface type. If thats the case (and they all have parameterless constructors) you can simply use reflection to get your type and then construct it.

class SpriteFactory : ISpriteFactory
{
    public T Create<T>() where T : ISprite
    {
        var type = typeof(T);
        var spriteType = AppDomain.CurrentDomain.GetAssemblies()
                                                .SelectMany(s => s.GetTypes())
                                                .Where(p => type.IsAssignableFrom(p) && p.IsClass)
                                                .FirstOrDefault();

        if (spriteType == null)
            return default(T);

        return (T)Activator.CreateInstance(spriteType);
    }
}

Now there is alot wrong with this implementation as it assumes alot.

  1. That there is only one implenetation of the ISprite type passed in.
  2. It does an assembly scan everytime you need to create a new instance of this object.
  3. Requires all implementations to have a default parameterless constructor on the implementation.

We could approach this by only scanning for the object once. But again we are stuck with the issues in point 1 and 3. This implementation is even uglier and (not tested) doesn't appear to be too thread safe.

class SpriteFactory : ISpriteFactory
{
    static object _locker = new object();

    Dictionary<Type, Type> registrations = new Dictionary<Type, Type>();

    public T Create<T>() where T : ISprite
    {
        var type = typeof(T);
        if (!registrations.ContainsKey(type))
        {
            lock (_locker)
            {
                var spriteType = AppDomain.CurrentDomain.GetAssemblies()
                                                        .SelectMany(s => s.GetTypes())
                                                        .Where(p => type.IsAssignableFrom(p) && p.IsClass)
                                                        .FirstOrDefault();
                registrations[type] = spriteType;
            }
        }
        var regType = registrations[type];
        return regType != null ? (T)Activator.CreateInstance(regType) : default(T);
    }
}

What you are really after is some sort of dependency injection and do away with the SpriteFactory all together. Dependency injection was made for this type of work and although the initial code is usually a bit more its typically more elegant. Especially when it comes to managing constructor injection, object disposal etc. The real power comes in when you need to start extending your application and adding more types.

Now without depenency injection you could define your registrations some where else. Where although it is similar to your first approach does abstract the implementation. Something like.

interface ISpriteFactory
{
    T Create<T>() where T : ISprite;
}

class SpriteFactory : ISpriteFactory
{
    static Dictionary<Type, Func<object>> registrations = new Dictionary<Type, Func<object>>();

    public static void AddRegistration<T>(Func<T> aquire)
    {
        registrations[typeof(T)] = aquire as Func<object>;
    }

    public T Create<T>() where T : ISprite
    {
        if (registrations.ContainsKey(typeof(T)))
            return (T)registrations[typeof(T)]();
        return default(T);
    }
}

Now what I have done is basically created static method to create registrations of your ISprite implementations. This can be used as such.

class Program
{
    static void Main(string[] args)
    {
        SpriteFactory.AddRegistration<IPieceSprite>(() => new PieceSprite());
        SpriteFactory.AddRegistration<ITileSprite>(() => new TileSprite());

        var factory = new SpriteFactory();

        var piece = factory.Create<IPieceSprite>();
    }
}

All in all without a proper framework in place to take care of your dependency resolution and life-time scopes you are stuck with either having our initial 3 problems or writing out your object creation methods manually.

\$\endgroup\$
  • \$\begingroup\$ Good answer, but replace every instance of the phrase "Dependency Injection" with "Inversion of Control Container". They're related, but not the same. \$\endgroup\$ – RubberDuck Mar 3 '16 at 10:16
  • \$\begingroup\$ Agreed, I always mix up the meanings of the two.. Will review the differences and update the answer. Cheers \$\endgroup\$ – Nico Mar 3 '16 at 12:24

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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