Code Review by Example - The singleton pattern

Code review is continuous process of reading and examination of code in order improve quality of the software. In code review phase the author of the code or the peer reviewers try to find mistakes, bugs and issues overlooked in the previous development phase. In this article we will see how code review can improve the quality of the code by simple example - implementing the singleton pattern.

The problem and its requirements

The singleton pattern is infamous design pattern. Our client define the problem by quoting from Wikipedia:

In software engineering, the singleton pattern is a design pattern that restricts the instantiation of a class to one object.

The first thing we need to do in order to solve this problem is to understand the requirements of the problem. Let's see what the requirements from the singleton class are:

  • The class should provide a mechanism to create one and only one object.
  • The class should provide a mechanism to access this single object.

Initial Phase

The first requirement - the mechanism to create one and only one object - can be achieved by deciding that the singleton class will be responsible to creating its single instance and restricting the instantiation of the class by entities of other classes (in their constructors, methods, field’s initializers, property's getters and setters and their static counterparts).

Luckily, C# language allows us to restrict access to constructors, methods, properties and fields. By declaring the singleton constructor as private, the only entities that can create this class are its members. The following code snippet demonstrates this restriction.

Singleton - restrict access to constructors
 class Singleton {
     // When we declare the class constructor as private, no other class can instantiate the class.
     private Singleton() {
     }
     public static void Main(string[] args) {
     }
 }

 class OtherClass {
     // when uncommenting the Singleton class instantiation in the following entities ,
     // the compiler will complain that Singleton constructor is inaccessible in the
     // current context.
     public OtherClass()        { /* Singleton singleton = new Singleton(); */ }
     static OtherClass()        { /* Singleton singleton = new Singleton(); */ }
     public void Method()       { /* Singleton singleton = new Singleton(); */ }
     static void MethodS()      { /* Singleton singleton = new Singleton(); */ }
     public int Property  { get { /* Singleton singleton = new Singleton(); */ return 1; }
                            set { /* Singleton singleton = new Singleton(); */           } }
     static int PropertyS { get { /* Singleton singleton = new Singleton(); */ return 1; }
                            set { /* Singleton singleton = new Singleton(); */           } }
     /* public Singleton _field  = new Singleton(); */
     /* static Singleton _fieldS = new Singleton(); */
 }

We also need the class to create one and only one instance. The obvious solution is to declare a public readonly static field and initialize it with new instance of the class.

This will ensure that only one object will be created since the static fields initializers are executed at most once in a given application domain. The other classes will access this object via this static field since it is public but cannot change its contents since it is readonly.

Singleton - version #1
 class Singleton {
     private Singleton()  {}
     public void DoWork() {}
     public static readonly Singleton Instance = new Singleton();
 }

This seems to work, we have a basic solution which fulfills the basic requirements of the singleton pattern.

code review

When our peer reviewer examines the first version of the class, he write a simple program to test it:

Singleton #1 , review #1
 class Singleton {
     private Singleton() {
         System.Console.WriteLine("Singleton");
     }
     public void DoWork() {
         System.Console.WriteLine("DoWork");
     }
     public static readonly Singleton Instance = new Singleton();
 }
 class Program {
     static public void Main() {
         System.Console.WriteLine("Main");
         Singleton.Instance.DoWork();
     }
 }

When he runs the program , he gets the following output:

Singleton #1 , review #1 output
 Singleton
 Main
 DoWork

He notices that the singleton object is created before it has been used. He realizes that in the current phase the singleton object is always created even if it not used. He write a program that demonstrates it:

Singleton #1 , review #2
 class Singleton {
     private Singleton() {
         System.Console.WriteLine("Singleton");
     }
     public void DoWork() {
         System.Console.WriteLine("DoWork");
     }
     public static readonly Singleton Instance = new Singleton();
 }
 class Program {
     static public void Main() {
         System.Console.WriteLine("Main");
         int xx = 5;
         if ( xx + 1 == 5 ) {
             Singleton.Instance.DoWork();
         }
     }
 }

When he runs the program , his concerns are verified:

Singleton #1 , review #2 output
 Singleton
 Main

I have noticed that in the current version of the code the singleton object is always created even if it not used. See the attached program I wrote that demonstrates this issue and its output

This issue may impact the performance of the program if the instantiation consumes precious execution time and memory resources.

The class should create the singleton object only when needed.