Encodo White Papers: Code Reviews (2017)

A healthy and active review culture is essential for any team interested in building quality software. At Encodo, we’ve been doing reviews for a long time. They’ve become an essential part of everything we do:

• Analysis
• Estimates
• Design
• Architecture
• Coding

  • Style
  • Performance
  • Coverage
  • Security
• Deployment
• And more…

Definition

What we mean by review is not a formal process at all. It is simply that you prepare work you’ve done for an informal presentation to a team member. Explaining what you’ve done in a review is often a good way of collecting your thoughts—you should be able to explain what you’ve done. Getting a review from a colleague is an efficient and productive way of making sure you can do that.

Limitations

While there are many reasons to do reviews, we’ve also learned that reviews can’t do everything.

• They’re not very efficient at distributing know-how. A review helps another, well-versed team member learn what you’ve done, but it won’t help non-team members get on-board.
• If the reviewer has to learn too much from the review, then they cannot perform a useful review. They can be, at best, a sounding board.
• For similar reasons, a review is not a good way of mentoring junior team members.

Benefits

• The quality of anything that you expect to review with someone automatically increases. Just knowing that you will have to tell someone what you did and why increases the likelihood that you think about the solution clearly and consider potential questions.
• A major benefit of getting a review is that it makes you check and prepare your own work. You’ll often notice that there’s still work to be done all on your own—even if the reviewer doesn’t say one word.
• When you’ve spent a long time on a problem, a fresh perspective as offered by a reviewer may offer an alternative solution. Often we’re so deeply involved in a solution that we don’t notice when we’ve diverged from the requirements and are just making more work for ourselves.
• When applied to very early stages, like analysis or estimates, the savings in time, money and manpower can be enormous.

Scheduling

It’s important to get reviews often enough to avoid wasting time and effort but not so often that your work or the reviewer’s work grinds to a halt. It’s all about balance.

A good rule of thumb is about one review per task. If your task is longer than a day, then think about how to break up that work into phases in order to get a review of earlier phases.

That way, you’re more likely to catch issues before building on top of mistakes.

Synchronous versus Asynchronous

Encodo prefers live, face-to-face reviews.

This is the most efficient manner of reviewing as neither party has to prepare anything other than the work to be reviewed. Issues that come up can often be handled immediately—and such issues are far more likely to be mentioned and fixed. While in-person reviews are superior, video-chat/shared-desktop reviews work quite well, too.

If that’s not possible, then we have also used tool-based, asynchronous reviews, such as pull requests with review software. However, we find these to be not only less efficient but also less likely to find as many issues.

With a live code review, it’s relatively easy to ask the submitter to reorder, split or squash commits. It’s also easier to point out and quickly fix stylistic issues (like naming or interface usage, etc.). Because the turnaround time is much faster, a reviewer is far more likely to point out smaller fixes that would improve code quality, maintainability and so on.

However, in an asynchronous review, a reviewer must decide what is most important. Is it worth rejecting the whole pull request if it’s 95% correct with a few details? Do you reject it and ask the submitter to fix up spacing or formatting or missing documentation? Do you really write down every last little thing you would have said? Do you reject it and hope that the submitter understands all of your notes? Or do you accept it and just fix those things up yourself? How many iterations do you go through?

We prefer synchronous, face-to-face reviews because they’re much more efficient. Misunderstandings can be cleared up quickly, iterating until the submitter and reviewer find a consensus.

Rules of Thumb

• A review should not consist of more than 200 lines of code
• A review should not last longer than an hour
• One reviewer is usually enough; if necessary, pull it at most two reviewers
• For non-code, use your judgment to determine an appropriate amount of reviewable work

Getting Faster

We encourage reviews everywhere because we know how to make them faster.

• Use your IDE to help submitters improve their code before they ask for a review. Formatting, code-style preferences and other issues can be eliminated from review if the tools enforce them.
• Start by focusing on the changes made (diffs) and expanding from there if further context is needed. If the reviewer was chosen well, then that context is often already available
• Review related commits en masse rather than individually
• Smaller, focused commits are much easier to explain and to review
• Spend less time on commits that are purely refactoring (trust your tools and your tests)
• Practice makes perfect!

Getting Better

Both the reviewer and the submitter need to practice. A reviewer should practice diplomacy and formulate critique in a way that it will be accepted. A submitter must keep an open mind and prepare good arguments or justification for the code. Both sides should stay positive. A review shouldn’t be a competition: it’s about producing high-quality code together, as a team.

Encodo has done presentations on reviews, in both English and German.