A friend of mine recently sent me some code to review:

 

Hi Peter,

Do you have any suggestions on how to clean up this code?

In particular the creation of a particular singleton type doesn't look very pretty in this model:

 

private DoubleCheckedLock<SingletonClass> myInstance = new DoubleCheckedLock<SingletonClass>();

public SingletonClass Current {

    get {

        return this.myInstance.GetSingleton(

            delegate() {

                return this.CreateSingleton(OrderContext, "third");

            }

        );

    }

}

 

I’m sharing my comments on the code, as it demonstrates some important design principles. Here’s the original code:

 

public class DoubleCheckedLock<T> where T : class

{

    private volatile T myInstance;

    private static readonly object lockObject = new object();

 

    public T GetSingleton(CreateObject c) {

        if (this.myInstance == null) {

            lock (lockObject) {

                if (this.myInstance == null) {

                    this.myInstance = c();

                }

            }

        }

        Debug.Assert(this.myInstance != null);

        return this.myInstance;

    }

 

    public delegate T CreateObject();

 

}

 

public abstract class DoubleCheckedLock2<T> where T : class

{

    protected abstract T CreateObject();

    private DoubleCheckedLock<T> myInstance = new DoubleCheckedLock<T>();

 

    public T Instance {

        get {

            return this.myInstance.GetSingleton(this.CreateObject);

        }

    }

}

 

And here are my comments:

 

• The name DoubleCheckedLock describes the implementation. Names should reflect how the client will use a symbol, not how something is implemented. For clients, the class implements a lazily created, thread safe singleton object. Aka, a Singleton object that does the right thing. Naming the class 'Singleton' will make client code read much better.

 

• I'm not a fan of public nested types, so I'd recommend moving the CreateObject delegate to a top level type. That's a style choice, but I've found it to wear well in practice.

 

• Names of types, including delegate types, should be nouns, not verbs or verb phrases. CreateObject should be renamed Creator.

 

• Passing in the creator delegate is necessary for this pattern so don't sweat it. However, passing in the delegate to the 'GetSingleton' method allows for some surprising behavior. It allows different calls to 'GetSingleton' to supply different values which will surely cause confusion. This confusion can be designed out by passing in the creator delegate as an argument to the Singleton constructor.

 

• Once the GetSingleton parameter is removed, it can become a property named 'Value'. Again, this makes your client code read better.

 

• Add the 'sealed' and 'readonly' modifiers unless you are specifically expecting to use a class as a base or modify a field after construction. The additional modifiers self document the code, and make you think through your design more fully.

 

• The DoubleCheckedLock2 abstract base class is a classic example of inheriting for containment - representing a 'has a' relationship between the derived class and the DoubleCheckedLock2 base class. Inheritance should always represent an 'is a' relationship, never a 'has a' relationship. You will save yourself many headaches by avoiding base classes which force you to use your one precious base class to represent a 'has a' relationship. Get rid of the DoubleCheckedLock2 class. The fact that it has such a crummy name, and that there is no good name to give this thing is a clear indication that something is wrong with it.

 

Now the client code looks like this:

 

sealed class Client {

    private readonly Singleton<ExpensiveResource> expensiveValue;

    Client() {

        this.expensiveValue =

                new Singleton<ExpensiveResource>(

                    delegate()

                    {

                        return new ExpensiveResource();

                    }

                );

    }

 

    public ExpensiveResource ExpensiveValue {

        get { return this.expensiveValue.Value; }

    }

}

 

Which is pretty damn sexy, if I don't say so myself.

 

Here's the implementation:

 

public delegate T Creator<T>();

 

public class Singleton<T> where T : class {

    private static readonly object lockObject = new object();

    private volatile T myInstance;

    private readonly Creator<T> creator;

 

    public Singleton(Creator<T> creator) {

        Debug.Assert(creator != null);

        this.creator = creator;

    }

 

    public T Value {

        get {

            if (this.myInstance == null) {

                lock (lockObject) {

                    if (this.myInstance == null) {

                        this.myInstance = this.creator();

                        this.creator = null;    // allow creator object to be GC-ed as soon as possible

                    }

                }

            }

            Debug.Assert(this.myInstance != null);

            return this.myInstance;

        }

    }

}

 

Happy coding,

Peter