Code Review — dos and don’ts

Code review has become an essential practice for many programmers. It comes with a variety of great outcomes, just to name a few:

• it’s a way to improve the quality of your codebase,
• it helps introducing the coding standards in your team or company,
• it can easily boost knowledge within your team,
• it allows junior developers to learn from seniors,
• it allows senior developers to make use of a fresh perspective of a junior.

But the question is, what makes a great code reviewer? How to conduct a review to make the best out of it?

Even though code reviews are widely used in almost all IT companies these days, there are some do’s and don’ts I find not so popular. In the following sections I gathered the best and worst practices I’ve seen in my day-to-day work

Do: Understand the purpose

Don’t: review code without the context

Reviewing code without the context, from my experience, tends to result in minor and low level comments.

The crucial part of the whole process is not only about pointing out a redundant import statement or a misspelled variable name. You want to focus on unreadability, potential bugs, bad architecture and design, lack of error handling, corner cases, and so on.

So, before you jump into the code, ask yourself a few questions:

• Is it a new feature or a bug fix?
• How important or urgent is it?
• What’s the business context, what are we going to achieve?
• Who is the expert? Who knows the latest agreements and details around this feature?

Don’t be afraid to talk about the purpose of the change with the colleague that asks for a review. Having all the answers will help you conduct a meaningful, thorough code review.

Do: checkout the code

Don’t: review it in your browser

This one might look a bit controversial. I know there are great tools to perform code reviews using just your web browser, for instance Atlassian’s Fisheye, JetBrains’ Upsource, or simply tools embedded in Gitlab or Github.

As a programmer I read code a lot. I’d say I read code way more than write it. You simply have to do it in order to decide what’s the best place to put the new feature into, to look for that nasty bug that keeps you awake at night, to check how the business process works, to learn good practices and new technologies, and so on.

I mostly use my favourite IDE to browse and read code, I’m accustomed to it. I know the navigation patterns, keyboard shortcuts, I have it customized to my preferences. The web-based code review tools feel too limited to me. Here’s what bothers me the most:

• they usually don’t show the whole file, but rather focus on highlighting changes in a line-by-line comparison.
  If as a reviewer you only focus on changes of a single line, you will likely miss the big picture. Perhaps the file you’re looking at has just become unmanageable — with tangled, spaghetti code and bad structure. You and your team had never realised that before, because it all happened line-by-line, over a couple of reviews, over a couple of months…
  Or maybe that new piece of code that’s being reviewed has nothing to do with the rest of the file. Before you know you’d end up with many responsibilities mixed together.
  My advice here is simple— look around the newly introduced changes, see how they fit into the bigger whole. It’s much easier in your IDE.
• you can’t easily navigate through the code and method calls, but need to jump around different files instead
  Having many different files and classes achieving some business purpose together it becomes quite a challenge to understand how things work only by reading.
  The web-based code review tools don’t know if you’re a programmer that likes to understand things with a top-down or bottom-up approach. They just show you a list of changed files and you need to figure out the correct order to browse them yourself.
  What helps me understand how code works is navigating between method calls, seeing how the flow of control moves from one method to another. IDEs are great at it, they usually offer a dozen of different ways to navigate through the code base. Make use of it!
• you can’t run tests in them
  Running unit tests is another way of understanding how code works. As TDD-practitioners say: an unit test is an executable code documentation that can never go out of sync with the code.
  When reviewing code in your IDE you can easily execute some tests, or even better, fiddle with one or two to see if your understanding is correct. I also find it useful to try out the code changes I’m about to suggest as a review comment — I want to make sure the tests still pass and my suggested approach works. Good luck doing any of it via the web-based review tool.

Do: mention minor things

Don’t: leave them unnoticed

Follow the famous boy-scout rule to leave your camp in a better condition than you’ve found it. This applies to your codebase too! Whenever you review some code changes take a good look around. There’s usually something that might be improved without much of a fuss — rename a variable, delete an obsolete import statement, or fix up some javadocs.

Once this approach becomes popular among your team, you’ll notice how your codebase quality improves over time, with less and less warnings during build, problem indicators in your IDE, or flaky tests.

Do: suggest changes

Don’t: tell others what to do

This one is about the style of your comments. I prefer suggesting rather than telling people what to do. I’d comment:

Consider using StringBuilder instead of String concatenation.

rather than:

Use StringBuilder, that’s the best way.

Might seem as a minor difference, but it leaves the decision up to the author and sounds friendlier.

What’s more, wherever possible I refrain myself from comments using adjectives like: easier, nicer, better, because they usually come with a personal sense of style, and their interpretation may vary among your colleagues.

From my experience people, especially programmers, don’t like being told what to do. They may feel under attack and automatically switch to defensive mode, trying to justify their decisions. If you ever find yourself in a fierce code review discussion because of that, don’t hesitate to simply talk to each other. Go to your colleague’s desk (or call him), listen to each other, and try to understand the other perspective. You want to collaborate, not fight with each other.

Do: make it a habit

Don’t: keep others waiting

Achieving goals with your team requires teamwork. That means you should keep everyone as productive as possible and limit the time spent on waiting for each other. That’s why I usually start my working day with a cup of coffee and a bunch of merge requests to review for my colleagues. I refrain myself from jumping straight to my coding tasks, even though writing code is much more exciting than reading it. Instead I rather do as much as possible to unblock my teammates. Believe me this approach pays back! If your team members follow the same approach you’ll get your tasks reviewed (and finished) faster.

A good starting point is to set aside some time for code reviews every day —try with a 30 minute slot in the morning. Starting with a fresh mind, not occupied by the tasks from your to-do list yet, will help you keep focused and be thorough while reviewing code.

Wrap up

Keep in mind that reviewing code is a skill that, as any other skill, requires practice and time to master.

Understand the context, use your IDE, be thorough, be nice and don’t keep others waiting. I hope that following these simple rules will help you become a great reviewer and avoid all the pitfalls I fell into.