The true value of code reviews

For many, the practice of performing a review on the code being submitted to a software project is very well-known, a habit you could say.

For many others though, that’s not so clear, both in terms of how to do it and of who should do it.

I believe all of those in the second group, but also some in the first group, could benefit from understanding the why of doing it or, in other words, where lies the value of this process called “code reviewing”, at least in my humble opinion.

Let’s start describing how the process works, for sake of clarity for those who might have never heard about it.

Imagine you are a developer working on a software project (it shouldn’t be hard if you are reading this post) and you have just finished applying the code modifications necessary to implement the feature requested by your business partner:

public class Account	{	
  double	principal,rate;	int	daysActive,accountType;
  public static final int STANDARD=0, BUDGET=1, PREMIUM=2, PREMIUM_PLUS=3;
}

public static double calculateFee(Account[] accs)
{
  double totalFee	= 0.0;
  Account	account;
  for (int i=0;i<accs.length;i++)	{
    account=accs[i];
        if(account.accountType==Account.PREMIUM||account.accountType
== Account.PREMIUM_PLUS)	
           totalFee += .0125 * ( // 1.25%	broker's fee
              account.principal*Math.pow (account.rate,(account.daysActive/365.25))	
												- account.principal);	//	interest-principal
  }
  return	totalFee;
}

Now, before putting your code in production and make your business happy, the company you are working for has this insane procedure requesting someone else to have a look at your code and give you either a green tick or a list of potential improvements. You blindly follow the mandatory procedure, asking yourself why the company wants to waste time and money to have a second opinion on your perfectly working piece of software the business have already tested and confirmed “it works flawlessly“.

A few hours later you receive an email from Ben, another developer, with your code accompanied by the following comments:

// BEN: have you seen the class CorporateAccount? Do we really need a different class?
public class Account	{	
  // BEN: class variables should be private
  // BEN: indentation should be tab based
  // BEN: you should split those variable decalarations into separate lines and add a comment to explain their purpose
  double	principal,rate;	int	daysActive,accountType;
  // BEN: better use an enumeration here
  public static final int STANDARD=0, BUDGET=1, PREMIUM=2, PREMIUM_PLUS=3;
}

// BEN: convention is to open parenthesis on the same line
// BEN: I suggest to use the non abbreviated name for the parameter (eq. `accounts`)
public static double calculateFee(Account[] accs)
{
  // BEN: I'm not sure, should we use BigDecimal instead?
  double totalFee	= 0.0;
  Account	account;
  // BEN: wouldn't it be better if we use the implicit for-loop version `Account account : accounts`? I believe looks nicer
  for (int i=0;i<accs.length;i++)	{
    account=accs[i];
        // BEN: wouldn't be more readable if we move this to a `isPremium()` method?
        if(account.accountType==Account.PREMIUM||account.accountType
== Account.PREMIUM_PLUS)	
           // BEN: we definitely want to make the 0.0125 value a constant
           // BEN: comments on a multi-line will be prone to break code or mess up comments with reformatting,
           // I suggest we split this up into multiple statements, maybe even multiple methods, this is hard to read
           totalFee += .0125 * ( // 1.25%	broker's fee
              account.principal*Math.pow (account.rate,(account.daysActive/365.25))	
												- account.principal);	//	interest-principal
  }
  // BEN: wrong spacing here, a tab slipped through 😉
  return	totalFee;
}

Your first reaction might happen in the back of your mind only, even unconsciusly: “Who the hell he thinks he is in criticizing my code? He is even less experienced then I am!” You would probably never admit to yourself you thought it, but it happened nonetheless. It lasted only a few milliseconds, your brain recorded it, but your hacking abilities have already deleted any trace of it from any log record: your consciusness is safe and the system can keep going.

Ok, let’s see what the heck he wants from me now!” might be your second reaction, then you start reading his comments. What happens next strongly depends on your attitude to collaboration and team working.

What Ben is trying to do is not to:

  • shadow you
  • state he is a better coder than you are
  • state anything about the business requirement being satisfied or not

If you believe one of the points above is the reason for Ben’s comments, you have a problem in working within a team. Not a huge problem, unless you work in a team-oriented company, in which case you might want to start searching for an alternative job or seek specific training.

What Ben is really trying to do is to share with you what he has learned so far about:

  • coding standards
  • naming conventions
  • possibly unspoken habits ongoing in the company
  • coding tricks
  • skills availability

But above anything, Ben is looking to learn from you and your code! And that doesn’t depend on the fact your seniority is higher than his.

Obviously Ben is not aware of which specific business requirement you are implementing, he has probably never seen the application you are developing: that’s why none of his comments are about the feature itself or the business satisfaction. He though has spent a few more weeks than you in the company and received some code reviews, so he is here trying to help, and learn.

At this stage your pride might kick back in, once again you might not be able to admit it, but something like “The business needs this feature straight away, I have no time for those changes you suggested” can make it’s way through your consciousness and even reach the keyboard via your fingers.

Are you still unaware of it or are you able to recognize the consequences of this statement? Let me be totally clear on this:

  1. you are bossing Ben, as if your time is more precious than his, who actually already did all the hard work
  2. you are endangering the company you work for, because you are increasing the tecnical debt of the system and increasing its total cost of ownership
  3. you are pissing off all your colleagues, who perceive you as disrespectful of the conventions they established
  4. you are covering your back appealing to “the business needs”, but the business requested you a feature, it never requested a raise in maintenance costs nor it told you how to do your job
  5. you are appealing to time constraints and, at the very same time, you are probably wasting time writing a long winded email of complaints rather than applying the requested changes and start learning from Ben

Now, if you want my humble opinion, my advice is to cool down, breath in, dump the email and just try understand Ben’s reasons: you might learn how teamwork plays a role in a healthy software system.

And it might also happen you get acquainted with a simple fact:

code reviews are about knowledge sharing and team growth, not about code verification.

Now, this is how the code should end up looking after a proper review and all parties learning from each other, including Ben, who now knows the existance and purpose of the Account class in opposition to the CorporateAccount class he was advising:

/** An individual account. Also see CorporateAccount. */
public class Account {
	private double principal;
	/** The yearly, compounded rate (at 365.25 days per year). */
	private double rate;
	/** Days since last interest payout. */
	private int daysActive;
	private Type type;
	/** The varieties of account our bank offers. */
	public enum Type{ STANDARD, BUDGET, PREMIUM, PREMIUM_PLUS }
	/** Compute interest. **/
	public double interest() {
		double years = daysActive / 365.25;
		double compoundInterest = principal * Math.pow(rate, years);
		return compoundInterest – principal;
	}
	/** Return true if this is a premium account. **/
	public boolean isPremium() {
		return accountType == Type.PREMIUM ||
		accountType == Type.PREMIUM_PLUS;
	}
	/** The portion of the interest that goes to the broker. **/
	public static final double BROKER_FEE_PERCENT = 0.0125;
	/** Return the sum of the broker fees for all the given accounts. **/
	public static double calculateFee(Account accounts[]) {
    	double totalFee = 0.0;
        for (Account account : accounts) {
            if (account.isPremium()) {
				totalFee += BROKER_FEE_PERCENT * account.interest();
      		}
  		}
	return totalFee;
	}
}

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out /  Change )

Google photo

You are commenting using your Google account. Log Out /  Change )

Twitter picture

You are commenting using your Twitter account. Log Out /  Change )

Facebook photo

You are commenting using your Facebook account. Log Out /  Change )

Connecting to %s