Is Reflection in a base class a bad design idea?

Question

In general reflection in a base class can be put to some good and useful ends, but I have a case here where I'm between a rock and a hard place... Use Reflection, or expose public Factory classes when they really should be private semantically speaking (ie. not just anyone should be able to use them). I suppose some code is in order here:

public abstract class SingletonForm<TThis> : Form 
    where TThis : SingletonForm<TThis>
{
    private static TThis m_singleton;
    private static object m_lock = new object();
    private static ISingletonFormFactory<TThis> m_factory;

    protected SingletonForm() { }

    public static TThis Singleton
    {
        get
        {
            lock (m_lock)
            {
                if (m_factory == null)
                {
                    foreach (Type t in typeof(TThis).GetNestedTypes(BindingFlags.NonPublic))
                    {
                        foreach (Type i in t.GetInterfaces())
                        {
                            if (i == typeof(ISingletonFormFactory<TThis>))
                                m_factory = (ISingletonFormFactory<TThis>)Activator.CreateInstance(t);
                        }
                    }

                    if (m_factory == null)
                        throw new InvalidOperationException(string.Format(
                            CultureInfo.InvariantCulture,
                            "{0} does not implement a nested ISingletonFormFactory<{0}>.",
                            typeof(TThis).ToString()));
                }

                if (m_singleton == null || m_singleton.IsDisposed)
                {
                    m_singleton = m_factory.GetNew();
                }

                return m_singleton;
            }
        }
    }
}

Now, this code works for me, but is it a horrible kludge and/or a really bad idea? The other option is passing in the Factory's type as a type paramater, but then due to visiblity limitations, the Factory class must be public which means anyone can call it to make instances when they shouldn't be.

Answer 1

When dealing with generics, you frequently have to use reflection. In that regard I think you're fine.

That said, I see two forms of code smell here. They may be due to code sanitation, however, so I'll just comment on them:

First, your static property is of a generic item. I'm 99.999% sure this will not even compile. If it does, it's bad form.

Second, you appear are returning a new instance for every call to Bar. This is also considered bad form for a getter. I would instead have a method called CreateBar() or something similar.

Answer 2

If it's possible, I'd try to avoid using reflection if you can get away with it.

To do this, you could try using an Abstract Factory pattern to get around the problem of directly exposing a public factory type, depending on your situation.

The way the Wikipedia example puts it (in Java) is that you create a factory interface, which your factory class implements, then you have something in your code generate the factory you need and return it as the factory interface.

Another way of doing this would be to create an abstract class instead of an interface for your abstract factory, then create a static method within this abstract class for returning the type factory you need.

Answer 3

Just an FYI, the following can be reduced from

foreach (Type i in t.GetInterfaces())
{
    if (i == typeof(ISingletonFormFactory<TThis>))
     m_factory = (ISingletonFormFactory<TThis>)Activator.CreateInstance(t);
}

to

if( typeof( ISingletonFormFactory<TThis> ).IsAssignableFrom( t ) )
    m_factory = Activator.CreateInstance( t ) as ISingletonFormFactory<TThis>;

Answer 4

This situation is also better handled with custom attributes where you can explicitly define the type to use.

[AttributeUsage( AttributeTargets.Class, AllowMultiple = false )]
public sealed class SingletonFactoryAttribute : Attribute
{
    public Type FactoryType{get;set;} 
    public SingletonFormAttribute( Type factoryType )
    { 
     FactoryType = factoryType; 
    }
}

Your singleton property now becomes

public static TThis Singleton
{
    get
    {
        lock (m_lock)
        {
            if (m_factory == null)
            {
    var attr = Attribute.GetCustomAttribute( 
                   typeof( TThis ), 
                   typeof( SingletonFactoryAttribute ) ) 
                   as SingletonFactoryAttribute;

                if (attr == null)
                    throw new InvalidOperationException(string.Format(
                        CultureInfo.InvariantCulture,
                        "{0} does not have a SingletonFactoryAttribute.",
                        typeof(TThis).ToString()));

                m_factory = Activator.CreateInstance( attr.FactoryType );
            }

            if (m_singleton == null || m_singleton.IsDisposed)
            {
                m_singleton = m_factory.GetNew();
            }

            return m_singleton;
        }
    }
}