The benefits of the codereview process

The benefits of having a code-review process in your team is that your entire codebase is getting maintainable. Let’s jump into a code review session.

Say that somebody has committed new functionality in our flight.cs class. The flight can now tell if it is a non-stop flight or not, have a look at the code before the code-review:

public virtual bool IsNonStop()
{
  bool nonStopFlight = true; 

  if (this.OutBoundLegs != null)
  {
    foreach (Leg leg in this.OutBoundLegs)
    {
      nonStopFlight = leg.IsNonStopLeg();
      if (nonStopFlight) break;
    }
  }

  if ((this.InBoundLegs.Count != 0) && nonStopFlight)
  {
    foreach (Leg leg in this.InBoundLegs)
    {
      nonStopFlight = leg.IsNonStopLeg();
      if (nonStopFlight) break;
    }
  }

  if (this.SelectedOutboundLeg != null && nonStopFlight)
  {
    nonStopFlight = this.SelectedOutboundLeg.IsNonStopLeg();
  }

  if (this.SelectedInboundLeg != null && nonStopFlight)
  {
    nonStopFlight = this.SelectedInboundLeg.IsNonStopLeg();
  }

  return nonStopFlight;
}

With around 35 lines not an overly-complex method. But, some if-statements are rather clumsy and it seems the developer wanted to keep track of the return-value all along the method.

Looking more closely at this piece of code: Apparently, we can determine if the flight is a non-stop flight by looking at its flight-legs. Whatever that is. The flight apparently has sets of outboundlegs and inboundlegs (flying from- and towards the destination, perhaps?) , and the last lines of the method even tell us that each “Leg” itself can already determine if it is a non-stop one or not.
All fine.

However, as said the function is setting its final state (nonStopFlight) within the first loop, and than uses the result further on in the method. Also, it runs two loops. Could that have been done simpler?

But what are we looking at here? When doing a code-review we must always keep the user-interface in mind. So let’s have a look at that user interface right now:

Now that we know that a flight consists of several possible outbound-legs (colour red) and possible inbound-legs (coloured orange), we can look more closely to the code that determines if the whole flight consists of nonstops.

Apparently, the customer still has to choose -what- outbound and inbound flights he wants to choose from, based on the departure times of the flights. And if the selection of the time the user makes is a non-stop flight, we mark the flight as such.

Let’s do a code-review on this method, and simply put remarks in the code -without changing the code itself-. We do not really know the domain of this method, but we can at least ask the original developer for the choices he has made. So let’s rumble:

Here is the code after the reviewer went over it with his hawk-eyes..

public virtual bool IsNonStop()
{
  //CR do not initialize on true, assume negative at the start
  // of the method. Please see if you can remove initialization 
  // altogether it should not be
  // needed, see other comments below.
  bool nonStopFlight = true; 

  if (this.OutBoundLegs != null)
  {
    foreach (Leg leg in this.OutBoundLegs)
    {
      nonStopFlight = leg.IsNonStopLeg();
      //CR below do not only 'break', but you should also
      // immediately return false; when that is possible. Because than 
      // you also do not need the ....&& nonStopFlight in 
      // the if-statements below.
      // so if you have determined a 'final' result 
      // (false in this case) feel free
      // to immediately return and
      // stop execution of this method.
      if (nonStopFlight) break;
    }
  }

  // Check on .Count is possible
  // null-ref exception or
  // are we sure InBoundLegs 
  // always exist? Think one-way-flights.
  if ((this.InBoundLegs.Count != 0) && nonStopFlight)
  {
    //CR: do we really need two loops in this method?
    foreach (Leg leg in this.InBoundLegs)
    {
      nonStopFlight = leg.IsNonStopLeg();
      //CR: same comment as above. return immediately if false.
      if (nonStopFlight) break;
    }
  }

  // CR: remove the nonStopFlight from if-statement below.
  // btw: if the SelectedOutboundLeg is part of the 
  // this.OutboundLegs 
  // list, than is this if-statement needed at all?
  if (this.SelectedOutboundLeg != null && nonStopFlight)
  {
    nonStopFlight = this.SelectedOutboundLeg.IsNonStopLeg();
  }

  // CR: same comment as above..
  if (this.SelectedInboundLeg != null && nonStopFlight)
  {
    nonStopFlight = this.SelectedInboundLeg.IsNonStopLeg();
  }

  return nonStopFlight;
}

Well, a much longer method now, but at least the original developer, who knows the component and domain he is working on has something to work on. As you can see, all code-remarks are prefixed with a nice “//CR…” so that it is clear that those are CodeReview remarks, and not remarks that explain the code itself.

The reviewer bounces the issue back to the developer, who takes up the work of refining the method. This is what the original developer, who knows the domain, did with the method:

public virtual bool IsNonStop()
{
  List allLegs = new List()
  if (this.OutBoundLegs != null)
  {
    allLegs.addRange(this.OutBoundLegs);
  }
  if (this.InBoundLegs != null)
  {
    allLegs.addRange(this.InBoundLegs);
  }
  // Loop through all the legs
  foreach (Leg leg in allLegs)
  {
    if (!leg.IsNonStopLeg())
    {
      return false;
    }
  }
   

  return true;
}

Wow. The original developer did a good job, handled the review and chopped off a whopping HALF of the method! (well, almost, we’re not counting empty lines now).

Also notice that the original developer suddenly checks both lists for NULL. That is probably preventing a nasty nullreference exception during program execution. Good catch!

But that is not all. Now the OO-guro (object oriented guru) comes by. And he says: “Well, if legs is something of this flight class, than why is “flight.AllLegs” not simply a property of this class itself? It will definitately make this method even smaller. And maybe I do want to use allLegs in other methods as well.

“Fine, OO-Guru.. change that code!” you say. And there he goes, encapsulates the allLegs and the method becomes even smaller.

public class Flight
{
... 

  private List AllLegs
  {
     get {
       List allLegs = new List()
       if (this.OutBoundLegs != null)
       {
          allLegs.addRange(this.OutBoundLegs);
       }
       if (this.InBoundLegs != null)
       {
          allLegs.addRange(this.InBoundLegs);
       }
       return allLegs;
     }
   }

...

public virtual bool IsNonStop()
{
   // Loop through all the legs
   foreach (Leg leg in this.AllLegs)
   {
     if (!leg.IsNonStopLeg())
     {
        return false;
     }
   }
   return true;
}

So.. all the logic you need in the method. Flight.IsNonStop() suddenly is a small, clear method that is easy to read and understand. Just as we want to read code. Compare this with what we first had in the beginning!

And really, this is not just an imaginary example. Now imagine what this means if you can do this kind of work to YOUR codebase. It greatly reduces clutter, enhances readability, caches bugs early, makes sure that future changes and enhancements are done quicker.

In all, the benefits of doing codereviews are:
– The second pair of eyes of the codereviewer are catching bugs early
– More readable code
– Less time spent reading the code by developers
– Easier maintainence
– Faster future enhancements

In all, codereviews reduce time to ship and thus costs. So, does your developmentteam already uses codereviews between developers?