Espirit D’Escalier

A quick lesson on why code is never finished until it’s finished. I was just sitting eating lunch and this morning’s code example ran through my mind. Check it out on this link. The crux of it was a generic collection, which used lazy initialization of singleton instances of strategy types that were fed into it with keys. The calling code looked like this:

selector = new StrategySelector<Options,IStrategy>();
selector.Register(Options.Blue, typeof (BlueStrategy));
selector.Register(Options.Red, typeof (RedStrategy));

and the instantiation of the Lazy object looked like this:

   public void Register(KEY key, Type strategyType)
   {
... CUT CODE


        // add lazy initializing instance of strategy
        lookupTable.Add(key, new Lazy<T>(() => ConstructFrom(strategyType)));
... CUT CODE


    /// <summary>
    /// Invoke the default constructor - used by the lazy initializor
    /// </summary>
    /// <param name="t">type</param>
    /// <returns>new object, case as our strategy type</returns>
    private T ConstructFrom(Type t)
    {
        ConstructorInfo ci = t.GetConstructor(Type.EmptyTypes);
        return ci.Invoke(new object[] {}) as T;
    }

Not especially nice, but all encapsulated away.

What bothered me was the transition from a type-safe world to the reflection world where anything could happen. I also didn’t like passing Type objects around when I’ve got a type-safe compiler that should surely be able to do more for me. I pondered over the creation pattern and felt that “new” is a much better keyword than constructorMethod.invoke.

This is where I realised that I hadn’t entirely used the TDD mantra: Red, Green, Refactor. I’d just hacked out some code that worked. Now I had the unit tests (with good coverage) and working code, I could refactor and see if something could be done to improve the pattern.

I considered my ideal method of constructing:

// add lazy initializing instance of strategy
lookupTable.Add(key, new Lazy<T>(() => new STRATEGYTYPE()));

You’ve got to love the new operator. That’s going to do the job. In the above STRATEGYTYPE would be a generic which has both new() and also implements T.

Then what about how to register the strategies with the selector/registry object?

selector = new StrategySelector<Options,IStrategy>();
selector.Register<BlueStrategy>(Options.Blue);
selector.Register<RedStrategy>(Options.Red);

I honestly can’t tell you whether I worked forward from the ideal calling code, or backwards from what would be needed to let me use new() in the internals. A bit of both probably.

This is where I think Occam’s razor, or the principle of parsimony comes into view. Basically, and this is only a rule of thumb so it doesn’t always apply, if it takes less code, it’s probably better.

The smaller selector class now looks like this:

    public class StrategySelector<KEY, T> where T : class
    {
        private Dictionary<KEY, Lazy<T>> lookupTable = new Dictionary<KEY, Lazy<T>>();


        public void Register<STRATEGYTYPE>(KEY key) where STRATEGYTYPE : T, new()
        {
            if (lookupTable.ContainsKey(key))
            {
                throw new ApplicationException("Double registration of " + key.ToString());
            }

            // add lazy initializing instance of strategy
            lookupTable.Add(key, new Lazy<T>(() => new STRATEGYTYPE()));
        }

        // accessor
        public T this[KEY red]
        {
            get { return lookupTable[red].Value; }
        }
    }

Fewer lines of code, less obscure syntax (not that I’m a fan of the Lambda expression), and it’s even a bit more readable.

It’s also type safe, which was what was bothering me over lunch.

The unit tests still pass.

The above code is still, in my view, unfit for production, but the method is very useful. Write the tests, make some code that gets them to pass, have a think, make that code better and simpler.

Advertisements

3 comments

  1. Now here’s a challenge for you JJ. You have an anti-pattern in your code:

    private readonly Dictionary _instanceControl = new Dictionary();
    private readonly Dictionary<Enum, Lazy> _singletonControl = new Dictionary<Enum, Lazy>();
    private readonly Dictionary _typeControl = new Dictionary();

    Three collections, each of which is keyed by the same key and which exist in parallel to collect a different aspect of the same data. How about a tuple to represent the three attributes?

  2. I introduced a nested class ->
    class StrategyStorage {
    internal InstanceType InstanceMode { get; set; }
    internal Lazy Singleton { get; set; }
    internal Type StrategyType { get; set; }
    }

    I prefer to access the values by name instead of Item1, Item2 and Item3…..

    Code is under the same url -> http://pastebin.com/8Yxeyv9G

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out /  Change )

Google+ photo

You are commenting using your Google+ account. Log Out /  Change )

Twitter picture

You are commenting using your Twitter account. Log Out /  Change )

Facebook photo

You are commenting using your Facebook account. Log Out /  Change )

Connecting to %s