On Code Reviews

Author: Lee Aplin

By now everybody knows the importance of code reviews. Regardless of the level of programmer, you’re prone to mistakes both trivial and critical, and code reviews help mitigate them.

However for the newcomer to code reviews here are some reasons to do them.

1. First and foremost code reviews catch bugs. It doesn’t matter how meticulously you check your code before merging a feature, there are likely to be things to improve. Sometimes somebody will point out a much more efficient approach to solving a problem which saves several lines of code.

Other times they may just point out that you’ve strayed from the house style of coding.

Both of these inputs are valid and help to keep a tidy and robust codebase.

2. Code reviews teach you things! Every person in your team will have expertise in different areas. You may be confident in your ability to complete a feature without any problems, but never assume that your approach is the best one. You can always learn something from your peers criticism and this is valuable.

Our approach to code reviews

Code reviews can be seen as a distraction, taking you out of your workflow to double check someone elses code, so at Substrakt we use some simple techniques to try and make the process a bit more efficient.

Firstly upon submitting a feature to git, the link to the Merge Request is posted into our internal QA slack channel. This is where the code review could sit indefinitely, lost amid the other slack chatter or ignored by busy people. But our process helps to keep the review moving along.

We flag the code review to the relevant people with @php, @js or @sass (or more generally @codereview).

Then we use our secret weapon – slack icons! Such a simple tool has made review progress easier to track. We mostly use three icons to indicate the progress of a code review.

The eyes. This icon indicates that someone has seen your merge request and is reviewing the code. As simple as it is, it’s really nice to know that someone is aware of your MR and is looking at it. It reduces the annoyance of waiting and wondering if anyone is checking the code.

The speech bubble. This refers to comments. If there are any problems or queries from the reviewer we flag this with the speech bubble, and keep all comments within the merge request itself in GitLab. This is useful because we can keep a record of which parts of the code need changing, and other members of the team can review code without flagging the same problems multi times.

After reviewing comments that have been left, we work through any changes and then bump the code review to check that everything we’ve changed is correct. If everything looks good we move onto…

The coveted green tick. This is the goal of our code reviews. When you see the green tick, merge it in and ship it. Your work here is done.

On occasion someone might make an obvious mistake. This is nothing to be ashamed of, it happens to the best of us, but it wont go unnoticed among the team! We have a slack command set up to highlight these “Classic Blunders”…

Other general tips

1. Don’t be scared of scrutiny. The whole point of a code review is to examine the code closely and identify problems before they go in a live codebase.

Any criticism isn’t personal, and you should look at it as a way of learning and improving rather than an attack on your code.

2. Keep merge requests concise if possible. Smaller chunks are much easier to digest and review quickly, and you’ll hopefully have less to change rather than refactoring a whole feature.

3. Finally never assume that a previously reviewed piece of code (from another project for instance) shouldn’t be checked again. There might be an even better way to write it, or somebody might spot something that was missed before. Code and techniques advance quickly, so it’s always prudent to be looking for a better way to do something you’ve done before.