Code Reviews Can Make or Break Your Team

This article is the second in the Tech Hiring & Team Building series. You can find the third installment here.

Alice just finished some code and needs it reviewed so it can get committed.

“Ugh, another code review” ~ Alice

First, she has to set up a meeting. At the meeting, everyone sits around a table. Bob points out that Alice’s variable names are 1 char too long for the naming convention. Jake says Alice needs to use a 2 space not 3 space tab stop. Heather points out that there are 3 spelling mistakes in the code comments and she shouldn’t be ending her sentences with a preposition. No one notices that Alice is passing unsanitized SQL straight to the database.

Many people fear reviews because of this

I recently wrote a surprisingly popular article on a method I’ve used to eliminate bad tech hires. [TL;DR pay candidates to do a simple, self contained project and then do a review of the project as if they were a team member]. Most of the questions about it revolved around 2 central themes : what the review process should be, and the mechanics of administering the problem.

In this article I’m going to tackle the review process by discussing what a good code review should look like. I’ll be mostly discussing the internal review process. Since you should be using the same process for the coding challenge review, the rules apply to both.

I drown kittens in my spare time

Let’s begin with a somewhat obvious observation: 100% of candidates will tell you they support code reviews. Saying you don’t like reviews is tantamount to saying “I drown kittens in my spare time”. A good number of them are lying to you. Why? Because they have had horrible experiences with code reviews, and because those experiences happened when they were very junior, they think that’s what code reviews are. Hopefully this article will help fix that.

Getting back to Alice

So what’s the end result of Alice’s experience in the code review?

• Alice is more concerned about the length of variable names than if they make sense in the context.
• Alice stops putting comments in her code so she doesn’t have to deal with Heather’s “Spelling and Grammar Nazi” attitude.
• Alice is more concerned with spaces than readability.

And of course this:

Image Credit: XKCD 327

Ok, but what’s the point of a code review then?

A code review has 4 purposes:

1. To make everyone on the team better
2. To make everyone on the team better
3. To make everyone on the team better
4. To catch errors or code smells before they become bugs or tech debt

That’s not me forgetting to change the 2nd and 3rd reasons. The first 3 reasons are the same because it’s that much more important than anything else you do at a code review.

Making knowledge communal makes everyone better

Making everyone on the team better means that everyone understands the codebase. Everyone understands the data flow. Everyone understands the reasoning behind not just the mechanics of why something is the way it is. At least someone at the review learns something. This may not necessarily be the person whose code is being reviewed.

This may not always happen, but it should always be the goal.

What should we review?

Everything. A review doesn’t need to be a big formal process. Grab another person or 2 and go over what you’re about to commit, or if you follow a team merge process what you’re about to merge. This can happen in an office, at your desk, or over a video call.

Smaller is better

Small chunks of code are much easier to review than large ones. Large bodies of code scare reviewers away for a number of reasons. Aside from the fact that smaller chunks are easier to make sense of quickly, people you pull in to review your code also need to complete work of their own. Small code reviews respect their time.

What does a good review look like?

A proper code review should follow these rules:

1 - Don’t discuss anything that can be fixed by an automated process (like a code formatter)

Just run the formatter. Don’t waste a single breath talking about tab spaces or line lengths. These types of discussions aren’t productive and turn people off of code reviews.

2 - Don’t discuss things that are just “slavery to a process”

These can be things like “variables must be less than 8 characters”. Really? This matters more than a clear variable name? As a corollary of this, don’t talk every name to death. If it makes reasonable sense move on. If the loop variables in a 4 deep nested loop are counter_a, counter_b, counter_c, and counter_d then that may be appropriate to discuss.

3 - Do try to keep it short

Short reviews, both in number of lines (200–300 lines max if possible) as well as number of minutes, allow you to get the most out of the time. The person whose code is being reviewed should be doing most of the talking (walking you through the code, and any design decisions they made). If you find a serious issue, stop the review. Let the person fix the issue before continuing. The reason for this is that a serious issue will likely result in enough changes in the code to require a new review anyway.

4 - Do discuss code smells

Code smells are things like “why are you attaching a function to window” in your JS file or “why are you using a malloc instead of new” in some C++ code. There’s a right way and a wrong way to challenge someone to explain such a decision.

Wrong way

Sigh. What were you thinking here? This is terrible. Go fix this.

Right way

You’re using malloc here, normally I’d use new as we use new for all our other dynamic memory allocations. If someone deletes this later instead of freeing it, it’s going to cause problems. Is there a reason you chose malloc over new?

There may be a perfectly valid reason, and by approaching it as a discussion you may learn something (even if that something is that you need to explain the difference between new and malloc).

5 - Do discuss architecture

Making sure that things are architected in a consistent and correct way is really important. This doesn’t just benefit the piece of code being added, but also future hires. If things are consistent in terms of architecture it’s easier for new people to understand how things fit together. Again it’s important to do this the right way (see above).

6 - Do look for errors that can’t be easily found with tests

Look for errors where bad data can result in not just a crash but storage corruption, etc. Things like unsanitized SQL being passed to the DB are a great example of this.

Summary

Code reviews should be as integral a part of your process as committing to version control, but how you do them can make or break your team. It can either be a fantastic means to share and increase communal knowledge, create solid products and make each other better, or it can be a dystopian nightmare of peer shaming, chest beating and bullying. The rules above will hopefully keep you closer to the former than the latter.

About Me

I’m Amir Yasin, a polyglot developer deeply interested in high performance, scalability, software architecture and generally solving hard problems. You can follow me on Medium where I blog about software engineering, follow me on Twitter where I occasionally say interesting things, or check out my contributions to the FOSS community on GitHub.

If you enjoyed this post, I’d really appreciate a recommend (just click the heart below).

Published in #SWLH (Startups, Wanderlust, and Life Hacking)

-