Back

MadCode Meetup: What Makes a Good Code Review

On February 10 we conducted our third Stanfy Madcode Meetup. This time Olexander Tereshchuk, our Android Engineer, talked about code review and how to do it right.

Here is what he shared:

What is code review? Code review is a systematic examination (often known as peer review) of computer source code aimed to find and fix mistakes overlooked in the initial development phase, improving both the overall quality of software and developers’ skills.

Formal (Fagan) code inspection

To start, let’s consider the oldest method — formal (Fagan) code inspection, named after Michael Fagan and developed at IBM in the 70s. A key feature is the clear division of the roles and phased process of review. In the beginning an author prepares all the materials that the project requires, distributes roles, and prints code. Team members are introduced to the project objectives and review goals. Then comes the inspection phase — a reviewer checks the code according to the requirements. Found differences are recorded and sent to the author for correction. Years later the new programming languages appeared and teams worked more remotely. Fagan Method is still alive in some forms but there are a bunch of cool new methods of finding defects.

Lightweight code reviews

With the advent of Agile everything turned upside down and a lot of new “light methods” appeared:

  • Over-the-shoulder — One developer looks over the author’s shoulder as the latter walks through the code.
  • Email pass-around — Source code management system emails code to reviewers automatically after check-in is made.
  • Tool-assisted code review – Authors and reviewers use software tools, informal ones such as pastebins and IRC, or specialized tools designed for peer code review.
  • Pair Programming — Two authors develop code together at the same workstation; this is common in Extreme Programming.

Why conduct code review?

  • To spot and fix defects early in the process.
  • Better-shared understanding of the code base.
  • Maintain a level of consistency in design and implementation.
  • A different perspective. “Another set of eyes” adds objectivity. Like the reason for separating your coding and testing teams, peer reviews provide the distance needed to recognize problems.
  • Pride/reward. Recognition of coding prowess is a significant reward for many programmers.

It is essential to detect defects during the development stage, when it is easier to fix them. During the review, the code has not even reached the main repository, so you can easily make edits without wasting time of the QA team and without affecting UX.

The key question is how much time should be spent on the code review? Studies show that reviewing 200—400 lines of code within 60—90 minutes can detect 70—90% of defects.

There are 3 Main Roles in Code Review

There are 3 main roles in code review

Author:

  • Always check the code before submitting.
  • Accept the criticism.
  • Defend your decisions.
  • Maintain the coding standards.

Reviewer:

  • Critique the code instead of the author.
  • Ask questions rather than make statements.
  • Remember that there is often more than one way to approach a solution.

Observer (3rd person):

  • Moderates review process.
  • Can check the code just to be aware of changes.
  • Can bring some special technical expertise.

Tools for code review can be found here. Next we’ll consider:

  • Collaborator — nice premium solution with various kinds of reports. It has integration with 11 version control systems. Clients for Windows, Mac, Linux. Tight integration of all in one place — tasks, comments, reviews, check-lists and so on.
  • GitHub gained a great popularity because of its nice web interface and free git repositories. It has clients for all popular operating systems and excellent integration with IDE. The subject of review is pull request — code + issue +comments. Herewith team can work in one repository, creating pull requests based on personal branches; or just fork the whole repository in your personal space and at the end of the work — do pull request to synchronize changes.
  • Gerrit — an important feature is that it is an independent product. You must deploy it on your infrastructure. Thus you solve the question of security, as you don’t pass code to third parties. The authors focused on supporting system scaling. Confirmation of the reliability of the product is its active use by Google.

The Stanfy’s way:

  • Github/Gitlab as in instrument.
  • Author requests a review — after the publication of the code in the repository, the author defines reviewer among the team members.
  • “Two fingers” rule — eventually we came to the conclusion that there should be two reviewers to avoid the problem of carelessness on the part of one reviewer.
  • Share the knowledge.
  • Automating everything — this is the key to success. We use Jenkins for automatic validation of code.

Summary

  • Tools do not matter, but people do.
  • Regular code reviews help to avoid about 60% of bugs.
  • Accept the criticism.
  • Use checklists.
  • Treat review comments as a document.

So, you should keep in mind that code review helps to catch more than half of all bugs in early development stages and saves a lot of resources. But the most important thing is — relations inside the team. It does not matter what tools you use, just be constructive and treat all team members as equal.

February 13, 2015
  • While people certainly matter more than tools do, good tools can make reviews easier and more effective. You might want to check out https://reviewable.io to see what a modern tool can do.

ConferencesMadCode Meetup