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.

Intial 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).

singleton design pattern agreement

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.

</p>

Phase 2

After reading the feedback of our peer reviewer, We write the following class which allows other classes to access its single object via static property. In the static property, we will create the object if it does not exist and return the single object.

Singleton #2
class Singleton {
    private Singleton() {
    }
    public void DoWork() {
    }
    public static Singleton Instance {
        get {
            if (_instance == null) {         // <== A
                _instance = new Singleton(); // <== B
            }
            return _instance;
        }
    }
    private static Singleton _instance = null;
}

code review

When our peer reviewer examines the new version of the class he realizes that now there is no guarantee that only one object will be created.

In multi-threaded programs, there can be situations where the object will be created several times.

Suppose one thread wants to access the single object via the static property when the object does not exist yet. The thread executes the if statement (A) and finds out the object does not exist yet. It starts to create the object (B), but then gets suspended. Now another thread tries to access the single object via the static property. Now the second thread executes the if statement (A) and finds out the object does not exist yet (since the first thread did not finish to create the object). Eventually, the 2 threads will create an object.

Please see the attached program I write which demonstrates this problem and its output.

Singleton #2 - review #1
using System.Threading;
class Singleton {
    private Singleton() {
        System.Console.WriteLine("Singleton : Start");
        if (_first ) {
            _first = false;
            Thread.Sleep(5000);
        }
        _count++;
        System.Console.WriteLine("Singleton End. There are {0} objects", _count);
    }
    public void DoWork(int xx) {
        System.Console.WriteLine("DoWork {0}", xx);
    }
    public static Singleton Instance {
        get {
            if (_instance == null) {
                _instance = new Singleton();
            }
            return _instance;
        }
    }
    private static Singleton _instance = null;
    private static bool _first = true;
    private static int _count = 0;
}
class Program {
    static public void Main() {
        System.Console.WriteLine("Main Started");
        Thread s1 = new Thread(() => Singleton.Instance.DoWork(1)); s1.Start();
        Thread s2 = new Thread(() => Singleton.Instance.DoWork(2)); s2.Start();
        s1.Join(); s2.Join();
        System.Console.WriteLine("Main Ended");
    }
}
Singleton #2 - review #1 output
Main Started
Singleton : Start
Singleton : Start
Singleton End. There are 1 objects
DoWork 1
Singleton End. There are 2 objects
DoWork 2
Main Ended

Phase 3

Luckily, C# has a simple construct to synchronize threads using the lock block. The lock block guarantees that only one thread will execute the code inside it at a given time:

Singleton #3
using System.Threading;
class Singleton {
    private Singleton() {
        System.Console.WriteLine("Singleton");
    }
    public void DoWork() {
        System.Console.WriteLine("DoWork");
    }
    public static Singleton Instance {
        get {
            lock (_locker) {                    // <== A
                if (_instance == null) {        // <== D
                    _instance = new Singleton();// <== B
                }
                return _instance;
            }                                   // <== C
        }
    }
    private static Singleton _instance = null;
    private static object _locker = new object();
}
class Program {
    static public void Main() {
        System.Console.WriteLine("Main");
        Singleton.Instance.DoWork();
    }
}

Now there is guarantee that only one object will be created.

Suppose one thread wants to access the single object via the static property when the object does not exist yet. The thread tries to acquire the lock (A) and succeeds (since no other thread have the lock). It starts to create the object but then get suspended (B). Now other thread wants to access the single object via the static property, it tries to acquire the lock (A), fails (since the first thread already acquire the lock) and therefore get suspended. Only when the first thread finishes to create the object and releases the lock (C) - The thread get resumed - but now, the object already exist and there is no need to create the object (D).

We run the program our peer reviewer wrote with the new version of the class:

Singleton #3 output
Main Started
Singleton : Start
Singleton End. There are 1 objects
DoWork 1
DoWork 2
Main Ended

The issue has been resolved.

code review

Our peer reviewer notice a performance issue:

As we know “All magic comes with a price” - I noticed a performance issue. Every time we access the single object, we request a lock. This has performance impact since locking pieces of code takes some time.

Phase 4

The synchronization problem occurs only when the object does not exist yet. When the object exists, we can simply return _instance static field. Using this observation, our next approach is to request the lock only when needed – when we create the single object.

Singleton #4
class Singleton {
    private Singleton() {
        System.Console.WriteLine("Singleton");
    }
    public void DoWork() {
        System.Console.WriteLine("DoWork");
    }
    public static Singleton Instance {
        get {
            if (_instance != null) {
                return _instance;
            }
            lock (_locker) {
                if (_instance == null) {
                    _instance = new Singleton();
                }
                return _instance;
            }
        }
    }
    private static Singleton _instance = null;
    private static object _locker = new object();
}
class Program {
    static public void Main() {
        System.Console.WriteLine("Main");
        Singleton.Instance.DoWork();
    }
}

code review

Our peer reviewer find a tricky way to eliminate locks

We still need to lock the piece of code that creates the object.

I find a tricky way to eliminate locks. In the first version of the problem, the object has been created before it was needed. If we enclose this class in outer class and provide static property to access the object – the object will be created only when needed since static fields initializers are executed at most once in a given application domain when the class is first accessed.

Phase 5

We follow our peer feedback and write the following class:

Singleton #5
class Singleton {
    public class Inner {
        private Inner() {
            System.Console.WriteLine("Singleton");
        }
        public void DoWork() {
            System.Console.WriteLine("DoWork");
        }
        public static Inner _instance = new Inner();
    }
    public static Singleton.Inner Instance {
        get {
            return Inner._instance;
        }
    }
}
class Program {
    static public void Main() {
        System.Console.WriteLine("Main");
        Singleton.Instance.DoWork();
    }
}

And this will produce the desired output without locks:

Singleton #5 output
Main
Singleton
DoWork

code review

Our manager review the code. decide that we should use known environment builtins whenever possible.

The current version is a little tricky. You should use known environment builtins whenever possible.

C# 4.0 introduces the Lazy<TT> for lazy instantiation as part of BCL. This class allow to defer the instantiation of an TT object until the time it is needed to save memory and execution time.

When the Value property of Lazy<TT> is first accessed - the TT object is created using initialization method given in the constructor or by the default constructor of TT. In the default mode , the initialization is thread safe - locks are used to ensure that only a single thread can initialize a TT instance - similar to solution 4.

Phase 6

We write the following code based on the comments of our manager:

Singleton #6
class Singleton {
    private Singleton() {
        Console.WriteLine("Singleton");
    }
    public void DoWork() {
        Console.WriteLine("DoWork");
    }
    public static Singleton Instance {
        get {
            return LazyInstance.Value;
        }
    }
    private static readonly Lazy<singleton> LazyInstance = new Lazy<Singleton>( () => new Singleton() );
}
class Program {
    static public void Main() {
        System.Console.WriteLine("Main");
        Singleton.Instance.DoWork();
    }
}

Summary

We have seen how the continuous process of code reviews help to fix mistakes, bugs and issues. After 6 iterations:

  • The solution ensures that single object is created only when needed to save memory and execution time.
  • The solution is thread safe.
  • The solution allows accessing the object as fast as possible.
  • The solution use builtin classes to accomplish its task.

Code review is viable tool to share knowledge and to learn new techniques. It helps the developers to foresee issues that may arise when using their code and to understand the environment they are working with.