Best Practices for Performing Efficient Code Review [Part 1]

Code review might seem not as exciting as coding. However, it’s a surefire way to streamline the process of product development and enhance software quality. In today’s blog, we start a three-part series on proven practices for efficient code review. In the first installment, we’ll share the tried and tested peer code review approach that has proven to be highly effective when developing Oro products. We’re sure our Community coders, as well as any other open-source product developers, can implement these guidelines to produce functioning, readable and maintainable code.

Code Review Workflow

The review begins after a code developer has completed writing the code. A reviewer checks the code and returns it for any fixes. After the code developer has changed the code, the reviewer verifies the changes. The code doesn’t pass the review until it is issue-free. Here is a typical workflow for code review:

You can customize this workflow according to your needs (e.g. by adding multiple review steps), but the goal of the review process is detecting and fixing the malfunctioning code.

Defining the Review Scope

The first step in a review is to define the scope. This is normally a GitHub pull request related to a respective task in your bug tracking system (we use JIRA, so we’ll refer to it later in the post) but sometimes these might be a code patch, separate files, or a plain code. Once it’s clear what element is to be inspected, the reviewer either checks out or copies all files locally and starts examining the code. If the code successfully passes the review, the reviewer should communicate this via a comment in the pull request or a JIRA ticket. If not, the code developer should make the changes per the fix requirements.

Preparation for the Code Review

Before inspecting the code, the code reviewer can draft a simple review scenario. It might include:

Investigating the review scope for a better understanding of the functionality to be affected;
Listing most common use cases for a fix to cover them all;
Listing possible bottlenecks to ensure a fix will not create another problem;
Creating a checklist of steps to follow (Stay tuned. We’ll share our checklist in the next installment of the Code Review series.);
Discussing critical issues with the code developer;
Documenting items to be checked in the next review session.

It’s always a good idea to set up either a local or remote ‘clean’ environment to cover the full scope of code tests.

Code Commenting

When the reviewer detects an issue, they report it by leaving a detailed description in the pull request comment. Comments should be:

Simple, intelligible, and brief. Depending on the volume of information to be reported, it may be better to break large issues into several comments.
Precise, explicit, explanatory. Each comment should address only one issue. It should describe it precisely and give a very clear picture of the changes the code developer must make. The reviewer should provide links to relevant documentation, JIRA tickets, code listings, and other related information where necessary.
Friendly and positive. Even if the team is fine with informal communication, keep comments professional and cordial.

Here’s an example of an effective comment on an issue:

This code is quite hard to read, and it might take up to 30 mins to understand the way it works. Please use self-explanatory variable names (e.g., $entity instead of $e) and split this big method into several smaller ones according to the stages of the implemented algorithm. This refactoring will also reduce the method’s complexity.

Occasionally, the code developer may request a further explanation in a counter comment. If this request requires extensive or time-consuming clarifications, it’s better to discuss the issue verbally or via instant messages. This saves the time and energy of the reviewer and the developer. Here’s an example of a clarifications comment: “Could you please explain what you mean by “Refactoring this algorithm”? What would be your suggestion?’’

Once the issue is fixed, the code developer should leave a comment notifying the reviewer of the solution. A confirmation comment might be as brief as: “Good point, I’ve fixed the code and added the full class description, too.”

The reviewer should not add recommendations that are not directly relevant to the code being reviewed (e.g., suggestions related to the code refactoring). It’s better to submit them to JIRA.

Communication

The code developer and reviewer should discuss the review not only during the code review but during planning sessions as well. If the reviewer and developer are teammates or share workspace, they can exchange information during the task implementation. By the time review begins, the reviewer will be aware of all task-related nuances. As a result, the review runs faster and smoother. You may also use knowledge sharing techniques such as Pair Programming or Ping-Pong Programming. Both practices increase code quality and decrease the number of code defects.

Source: http://geek-and-poke.com/

Amount of Code Under Review

The recommended amount of code to review in one pull request is about 200-400 lines. If you must review a greater amount of code, split it into smaller pieces. If you must review a pull request referencing the implementation of a major feature containing separate code fragments that previously were reviewed, focus on where these fragments relate to each other as well as to the feature requirements.

Time Required To Review Code

If code review takes more than 30% of the overall implementation time, it could indicate an underlying problem.

Here are the most common challenges and ways to overcome them:

Too many code defects. Consider improving team skills via training, webinars, and sharing best practices that cover the issues in question.
Architectural problems. Pay more attention to architecture during the planning sessions, build classes diagram, and component dependencies.
Requirements not met. Make sure everyone understands acceptance criteria of a feature before starting implementation.

Code review may require more time than usual if the code must be inspected by several people with different expertise. This should be taken into account during the planning to properly estimate the time needed to complete the task. Ideally, the reviewer should spend up to 2 consecutive hours and review about 200-400 lines per hour (the slower is the better). Don’t review more than 800 lines of code at a stretch.

Post-Review

After the review, the code developer might need to get into a face-to-face discussion of the detected issues. This is a faster and easier way to get on the same page and avoid misunderstanding. All decisions made during this discussion should be reflected in either new or existing comment. This will ensure consistency and enable other developers to understand the pull request, if necessary.