How we conduct code reviews at globaldev

By: Simon Bingham

Tags:

  • code-reviews

I had never taken part in a code review before joining globaldev. However, after a year of experiencing code reviews both as an author of code and a reviewer, I have come to appreciate their value as part of the development process.

What is a Code Review?

“A code review is systematic examination of computer source code. It is intended to find and fix mistakes overlooked in the initial development phase, improving both the overall quality of software and the developers’ skills. Reviews are done in various forms such as pair programming, informal walkthroughs, and formal inspections.”

Source: Wikipedia

All code that gets deployed on to the White Label Dating™ platform must go through at least one phase of code review.

Why Perform Code Reviews?

Performing code reviews is an incredibly beneficial part of the development process. Just some of the benefits of code reviews include:

  • Helping everyone learn: not just about the system, but about development techniques in general.
  • Avoiding tunnel vision from the implementer: it’s all too easy to be blinkered when coding, and not consider the wider issues.
  • Heading off bad design decisions before they get too ingrained within the implementation, and therefore more expensive to change.
  • Ensuring adherence to coding and development standards.
  • Catching bugs that otherwise wouldn’t have been picked up until further in to the development cycle (or at all!). Research has shown that code reviews can accomplish at most an 85% defect removal rate with an average rate of around 65% (Wikipedia).
  • Ensuring adherence to security standards, and avoiding falling into any traps which may leave our systems vulnerable.

Types of Code Review

Someone may ask for a code review for a number of different reasons. In general, we break these reasons down in to the following three categories.

Thirty Percent

This type of review is less of a “code” review and more of an “approach” review. The developer has thought about a problem and has created an initial spike that demonstrates their thinking, but they want to get feedback before committing themselves. Asking for a review at this time is incredibly important because it can help steer the direction of their work before they’ve gone too far and have to backtrack. The earlier that a bug (or an incorrect approach) can be spotted, the cheaper it is to correct course.

The reviewer should ignore things like syntactic errors, inconsistencies with agreed coding standards etc, and assume that these will be fixed down the line, and instead focus on:

  • Reasoning
    • Has the developer understood the problem at hand?
    • Does the approach taken actually solved the problem?
  • Approach
    • Is the approach taken generally correct?
    • Are there any significant edge cases that need to be considered?
  • Architecture
    • Is the general code layout in keeping with the rest of the application?
    • Has a sensible breakdown of classes and modules been used?
  • Security

Ninety Percent

This type of review is all about catching problems before they get into production. The reviewer goes over the finer details and highlights any problems they find. The assumption here is that the approach has already been ratified.

  • Catching implementation bugs
  • Coding style inconsistencies, including breaking coding standards
  • Typos
  • Potential security holes
  • All the little details

Specific

Other than more general approach and code reviews, a developer might want to home in on one particular area that they feel they need assistance on. They’re more likely to want to do this if they are:

  • working on an area of the system that’s new to them
  • working on part of the system where they know one or two other people have very good knowledge
  • using a language that’s outside their normal remit (e.g. a ColdFusion developer doing JavaScript)
  • unsure of the security ramifications of the work they’re undertaking

Specific review points like this work via the same pull request as more general reviews. The developer simply @-mention one or more users and ask them the specific questions they need answering.

Requesting a Code Review

We use a branching model which follows GitHub Flow, and use GitHub’s Pull Requests as a means to perform code reviews. The creation of a new pull request is effectively a request for a code review of some format on what they’ve submitted.

Creating a Pull Request

Think of raising a pull request to be like crafting a commit message. It should include both a summary (title) and a detailed description of what the pull request contains. The title should be short and to the point: a good rule of thumb is that, if the GitHub interface line-wraps the title, then it’s too long.

The description should include the following pieces of information:

  • What the changes do. A reasonably detailed description of what the changeset does. A small change does not necessarily equal a small description. Background as to why a change has been implemented in the way it has. The developer should attempt to preempt any questions of “why have you done it like that?”

  • Why the changes are required. This is very important for the review process: it gives a reviewer, who might not be familiar with the requirements, the background they need to perform a good code review. Links to Jira tickets, GitHub issues and other pull requests are provided to add context.

  • What the developer expects to get out of the code review. This bit is often forgotten, but it’s very important for the developer to specify what it is they’re expecting to get out of a code review. At the very least they should highlight if they’re asking someone specific to review something, if they’re asking for an approach review (“Thirty Percent”), or a final pre-merge review (“Ninety Percent”).

  • Any other detail the developer thinks will help either a reviewer or someone looking at the pull request in the future to understand the reasoning behind the change.

Once a pull request is closed, it forms part of the documentation of the system, so ensuring the description includes all the details above is as much of a maintenance issue as it is for code review.

Performing a Code Review

The person completing the code review should:

  • Give it the time it needs: if they can’t review right away, they should let the submitter know.
  • Keep review points relevant to the type of review that’s been requested.
  • If criticising, be constructive. Don’t just say “this is wrong,” but explain why, and give an example of how the work could be improved.
  • Be patient, clear and concise. The submitter should assume that everyone’s trying to help, not that they’re just there to rubbish the submitters hard work.
  • Be positive and highlight things they like or have learned, rather than just problems with the submission.

Although we value our approach to code reviews, there are often times when something doesn’t quite fit into what’s described here. The important part is that the general principle of code review is applied. Also, how we implement code reviews is an open topic for discussion. We encourage each other to speak up when we have a question, or an idea about how to improve.

If you would like to work with us at Global Personals, check out our jobs page, we’re hiring.


About the Author

Simon Bingham