Yet another article about code review and in our last article, my colleague Eldar has taken a chunk of code and pointed out all the mistakes and all the questions one simple piece of code can generate. Basically what he did was a code review. 

Now we are going to present how our code review process looks like.

The value of code review

Code review is an essential part of development (as is writing unit tests by the way), so actual code writing is only a small part of the complete development cycle. First question is:  What is the value of code review? 

As you may assume we have the usual suspects:

• Better code quality – by commenting and resolving comments code will undoubtedly become better
• Knowledge sharing – multiple people with get familiar with the codebase
• Programmer’s ego – everyone will write as good code as it gets, knowing that other people will be reviewing it
• Consistency – idea is that code looks like it is written by one person, that kind of effects can be accomplished by code review (among others)

Prerequisites

For code review to run in an efficient flow, establishing some sort of Continuous Integration/Continuous Delivery system is an absolute must have. My personal recommendation is the Atlassian toolset, but whatever works for you.

Once Pull/Merge Request is created, the automatic builds are triggered. Builds include code-style check, code standard check, spell-checker, unit-tests check, functional-tests check. Even if you don’t have all of these checks setup, I would recommend their setting up even with minimum functionality. Then with time and resources they can be expanded to bring greater value. This way you are reducing things to look at as a reviewer, and you can skip silly indentation comments and things like that. Only thing you must focus on is functionality and if it’s possible even architectural decisions. Our ideal scenario is two reviewers, where one of them must have a deeper architectural knowledge on code under the review so he/she can understand the deeper implications caused by merging of the reviewed code.

Checklist

The Code review process is a flat hierarchy and no developer is spared from code review. Even if everything is perfect (very, very, very rare case), there is a learning opportunity for the reviewer. Obviously, not every check is applicable to every project, but here are some things we pay attention to, and I believe you can use some if not most of them.

Preparation

• Checkout code on local machine
• Grab a cup of coffee

Purpose

• First you read the issue code under review is resolving – Is it a bug fix, new feature, or something third. Commit messages should always contain in its description and a link to the issue.
• Check are commits aligned with your expectations. Are they clearly separated, small enough to grasp and descriptive enough for the reviewer to understand
• Go commit by commit and try to understand the author’s intentions
• Refactor and code style changes should not alter behaviour  – They should be a part of separate PR

Implementation

• Understand who’s the user of the code reviewed (library, service, database, API, end user)
• Are comments for the code clear and sufficient
• Are all scenarios covered (positive or negative, this is usually covered by some sort of tests (unit, integration) but still…)
• If something is unclear, and needs a lot of explanation to both reviewers, maybe some comment inside the code should be added explaining WHY (not WHAT, actual functionality should  be self explanatory from code itself)

Dependencies

• Is code a part of the public interface (used by other layers of code, other repositories…) or the private interface
• If it is a part of the public interface, are any dependent modules affected by this change
• Are any new dependencies introduced – can you see the reason behind it?
• Are any architectural or layer boundaries crossed?
• Is this code duplicated?

Maintainability

• Are there TODOs? Is there a need for TODO? Is it linked with some existing or newly created tickets or issues
• Is code covered with tests? Does it potentially break any integrated tests?
• If so, is it OK to merge the change at this point or should it be pushed into a later release? 
• If your project maintains a README, CHANGELOG, or other documentation, was it updated to reflect the changes? Outdated documentation can be more confusing than none, and it will be more costly to fix it in the future than to update it now

PR Comments

Since Pull Requests are a form of communication between two or more persons, some rules and guidelines need to be established, in order for the process to be as fast as possible. Here are some guidelines for both reviewer and code owner.

Reviewer:

• All PR comments made must be clear, and in case of problem with proposed solution how to resolve it (comments like this is not good should be discarded)
• Ask for clarification, rather than assuming ignorance
• Compliment someone if you think some implemented solution is something you like, and something you could use in the future
• Discuss via some faster form of communication (in person, private chat) if there is some misunderstanding (A lot of comments usually mean there is probably something that was not clear to all parties included)
• Minor (Nit pick) comments – stress them as such. They usually don‘t block pull request
• Don‘t leave it open for too long, then the author will not be sure if the code is still under open review

Code owner:

• All comments made must be answered by the PR owner so we make sure there are not any questions unanswered
• PR fixes commits shouldn’t be squashed until PR is approved (if needed), so reviewer can have a clear picture about what and how was resolved

Conclusion 

Once you are a code reviewer, code becomes your responsibility as well, so everything that gets merged that can cause problems on the main branch is shared by fault. Code review is not a 15 minutes task. It takes time and that time should be planned accordingly. Just as with coding, my suggestion is to take breaks, do not review for more than 60 minutes at a time.

Missed the first part of the blog series? Read Part 1 in our blog series about coding.