Write Better Code With Code Review [Example With 5 Code Review Phases]

Code review is a continuous process of reading and examining the code to improve the quality of the software. In each 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.

Reading and Understanding The Problem And Its Requirements

Suppose we need to implement the singleton design pattern. Our client defines 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.

From singleton in c# and the pattern on wikipedia

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 Solution

First, we need to create a mechanism that allows creating one and only one object. Let’s decide that the singleton class is responsible for creating its single instance.

Second, we need to restrict the class’s instantiation by entities of other classes (in their constructors, methods, field’s initializers, property’s getters, and setters and their static counterparts).

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.

C# Restrict Access Of Other Classes To Private 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 a 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.

Initial Solution
 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 Phase #1

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

Code Review Phase #1 – The Test Program
 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 Test, He Gets The Following 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 writes a program that demonstrates it:

Code Review Phase #1 – The Second Test Program
 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 Test , His Concerns Are Verified
 Singleton
 Main

He write the following comments in his code review:

Hi

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.

The Comments Of Code Review Phase #1

Fixing Code After Code Review Phase #1

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.

Fixing Code After Code Review Phase #1
 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 Phase #2

When our peer reviewer examines the new version of the class, he realizes that there is no guarantee that the application will create only one object. In multi-threaded programs, there can be situations where the application creates several objects.

He write the following comments in his code review:

Hi

I read your code and verified that the object created when needed. However, The programs do not meet the requirements anymore. The main requirement is that the application will create at most one object. However, In multi-threaded programs, there can be situations where the application creates several objects.

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. The second thread executes the if statement (A) and finds out the object does not exist yet (since the first thread did not finish creating the object). Eventually, the 2 threads will create an object.

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

The Comments Of Code Review Phase #2
Code Review Phase #2 – The Test Program
 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");
     }
 }
The Test Program Output
 {% code {"lang" : "text", "title" : "Singleton #2 - "} %}
 Main Started
 Singleton : Start
 Singleton : Start
 Singleton End. There are 1 objects
 DoWork 1
 Singleton End. There are 2 objects
 DoWork 2
 Main Ended

Fixing Code After Code Review Phase #2

We find that 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:

Fixing Code After Code Review Phase #2
 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 has the lock). It starts to create the object but then gets 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 acquires the lock) and therefore gets suspended. Only when the first thread finishes to create the object and releases the lock (C) – The thread gets resumed – but now, the object already exists 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 and verify that it works as expected and mark the issue as resolved.

Verify the problem raised on Code Review is Fixed
 Main Started
 Singleton : Start
 Singleton End. There are 1 objects
 DoWork 1
 DoWork 2
 Main Ended

Code Review Phase #3

Our peer reviewer notice a performance issue:

Hi

As we know, “All magic comes with a price,” and the magical lock construct is not an exception. I noticed a performance issue. Every time we access a single object, we request a lock. It has a performance impact since locking pieces of code takes some time.

Remember that we create the object only once but need to access it many times.

The Comments Of Code Review Phase #3

Fixing Code After Code Review Phase #3

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

Fixing Code After Code Review Phase #3
 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 Phase #4

Our peer reviewer find a tricky way to eliminate locks

Hi

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.

The Comments Of Code Review Phase #4

Fixing Code After Code Review Phase #4

We follow our peer feedback and write the following class

Fixing Code After Code Review Phase #4
 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();
     }
 }
Output
 Main
 Singleton
 DoWork

we verify this class produce the desired output without locks. However, It does not the right thing to do. We ask our manager to review our code.

Code Review Phase #5

When our manager review the code, he 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.

The Comments Of Code Review Phase #5

Fixing Code After Code Review Phase #5

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

Fixing Code After Code Review Phase #5
 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();
     }
 }

Code Review by Example – The Singleton Summary

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

Code Review help to fix mistakes, bugs, and issues.
Code Review 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.

What do you think? Is code review is part of process of writing code? Does Code review improve the quality of the code?

Leave a Reply

Your email address will not be published. Required fields are marked *