Code Reviews

The idea of code reviews is fairly well known.  Many businesses practice them regularly, many others agree that it is a good idea.  But, there aren’t a lot of resources out there to help a business understand how to do good reviews.

Why do them?

Let’s start off with a quick review seeing as most of us understand why.

The most common reason people state for doing code reviews is to catch bugs.  When you have a second set of eyes look at a piece of code, they may see some edge cases that you did not account for, or see other silly mistakes that we all make.  This is important, but I think there are other very important benefits of doing code reviews.

Code reviews are an excellent learning platform for both the developer and the reviewer. It is pretty clear why code reviews are a learning opportunity for the developer. The developer gets a chance to gain feedback from his/her colleagues on the code they produced, which sometimes reveals new techniques and new “best practices” that they may not have known.

However, perhaps less clear is why this can be a learning opportunity for the reviewer.  All of the benefits that the original developer gets from having someone review their code can be applied to the reviewer as well.  They may see new things they have not encountered before.  Additionally, the reviewer gets an opportunity to learn about the new feature that was added.  This leads to shared understanding of the code, and greater collective code ownership.

Code standardization can also be a benefit of code reviews, if you decide to have people review on this.  There are a lot of tools that can help make sure you are applying coding standards as well.

Catching defects at this stage will allow us to fix them with minimal effort.  Sometimes a design defect will be built upon and then it could impact a much wider part of the system so when we go back to fix it, it may cause ripple effects throughout the code-base.

What should we review?

There are a lot of discussions about what should be reviewed in a code review.  One that often comes up is coding standards.  As I said above, there are a lot of automated tools that can help ensure that standards are being applied across the board, but they won’t catch things like bad method names or bad variable names to name a few.  I think this is valuable input.  Since many of these “violations” are easily fixed, I think it is reasonable to ask that they are fixed before the task is considered complete.

An obvious think to look for in a code review is bugs.  If you see a bit of code that may cause an error in certain circumstances, please call it out, even if you aren’t sure.  It is better to have the conversation about the piece of code and decide that it is fine than it is to let it slide and later find out that it was an issue all along.

Lastly, if you see some code that could be improved, I think it is acceptable to mention this in your review.  Those changes may not need to happen at this time, but it is a learning opportunity.  If there isn’t time to address the refactor (and people agree that this is a good change), make sure to note it down somewhere (preferrably your ticket tracking software).

Who should review?

I hear this one quite often. I think any developer that might touch the software in question should be involved in the review.  I often hear pushback that this will take too long for the indivdual developers, or that the review might sit out there too long and we will not be able to merge the code.

Doing the code review before the code goes into production may take a little longer upfront, but there are a lot of benefits (as I described above) to doing so.  We can potentially catch errors before we start building on top of them and relying on the faulty code for other parts of the system.

As for the review sitting out too long, there are lots of things that you can put in place to ensure that this doesn’t happen.  Daily stand-ups (or at the very least team check-ins) allow the team to mention that a code review is waiting for reviewers.  You can set up rules for how many people need to review and approve before the code is merged so that you don’t have to wait for the entire team.  Generally speaking, this is usually not an issue because the team wants to get work done.

What about pairing?

In many cases pairing can take the place of the traditional code review.  You get two sets of eyes on the code as it is being written, which can avoid many of the issues we are trying to catch in a code review.

However, I would recommend that code reviews are good practice when pairing especially in a few key circumstances:

a) When the pair is two junior developers.   A more experienced set of eyes might be able to shed some light on potential issues the junior pair would not have known to look for.

b) The new feature is very complex.  If a new feature is very complex having additional eyes on it is a good thing.  Doing this could catch potential bugs the developers did not see, but it also helps the entire team to understand the new code.

How should we review?

This is probably the most important part of this whole article.  We are all a little scared of the code review.  We don’t want to look dumb or bad in front of our colleagues and showing our work can be a little intimidating.

I have heard a couple of really good pieces of advice on this front.

  1. Always assume best intentions.  This goes for the reviewer and the reviewee.  The person who wrote the code is trying to do the right thing…perhaps there is a good reason it is written the way it is.  The person doing the reviewing is also trying to do the right thing, tone doesn’t carry well in written language.
  2. When you are commenting on a piece of code, be nice.  Use “we should” instead of “you should”.
  3. Ask questions about the code rather than accusing them of doing something wrong. “Why did you do this?  Would this other thing have been better?”
  4. Talk to the person face to face.  Sometimes a written discussion leads you in circles, or inflames one or both parties.  Have a face to face if there is an issue that you are unable to resolve.