Skip to main content

Code Review Guideline

A code review intends to improve the quality of the code. Someone other than the author of the code performs the examination. Code review is mandatory for new code and code changes (pull request).

Code Review Goals​

  • Eliminate bugs at an early stage
  • Improve code quality
  • Increasing knowledge about the project
  • Learning from each other
  • Ensure that the story is fully implemented from a functional and technical point of view
  • Improve existing and new code
  • Verify the correctness of both code and unit testing

Code Review Rules​

  • Code review is mandatory for new code and code changes (pull request).
  • You may not approve your own changes
  • Offer as much positive feedback as possible
  • Review everything that can be read
  • It is a great place to learn something new
  • Find out whether your comment meets any goal. Comments that don't provide any value are really demotivating. Consider whether your change proposition really does matter
  • Try to understand why such a solution has been applied instead of writing that the code is wrong. You don't have to be always right, and a comment should be the beginning of a discussion, not just a change
  • Try to coach the author
  • If some part is missing, describe what should be added, e.g., what more needs to be changed, what unit tests need to be added
  • Explanations and discussion results should be noted either in the code comment, in the commit description, or in the implementation details design description
  • Automate as much as possible by introducing static code analysis tools
  • Do not make a merge to the master branch until the code review is completed
  • Proposed changes in functionality should be discussed and stored in separate work items. If the change is handled as part of future work, please add the corresponding work item

Code Review Checklist​

  • Check that the code implements the intended design (check the applicable design/architecture description)
  • Check that the code implements or follows the applicable mitigations described in the threat model
  • Look for possible automated tests, and if they are available, review them
  • Look for potential bugs
  • Check if all errors are handled gracefully
  • Check if code is implemented efficiently, e.g.:
    • Do not contain duplicates
    • Patterns and design principles are properly used
  • Check if the code introduces technical debt
  • Check if the code does not cause side effects
  • Check the rules in the selected coding standard that is not verified automatically (as defined in the secure coding verification strategy) Secure Coding Guideline
  • Check code readability
    • Is the code easy to understand for later maintenance if the need arises
    • Do method and variable names express what they are used for
  • Check if code is properly commented - is it difficult to understand parts or hacks should always be commented on to help other people understand the reason for the implementation
  • Check if all comments are addressed before merging to the master branch

How to run Code Review​

There are at least four types of code reviews. Below is a short description of each one.

Formal inspections​

Formal reviews are usually called "inspections" from Michael Fagan's seminal 1976 study at IBM regarding the efficacy of peer reviews. He tried many combinations of variables and came up with a procedure for reviewing up to 250 lines of source code. After 800 iterations, Fagan came up with a formalized inspection strategy named "Fagan Inspection."

In general, a formal review refers to a process review with three to six participants meeting together in one room with printouts and/or a projector. Someone is the "moderator" or "controller" and acts as the organizer, keeps everyone on task, controls the pace of the review, and acts as an arbiter of disputes. Everyone reads through the materials beforehand to properly prepare for the meeting.

When defects are discovered in a formal review, they are usually recorded in detail.

Over-the-shoulder reviews​

This is the most common and informal of code reviews. An "over-the-shoulder" review is just a developer standing over the author's workstation while the author walks the reviewer through a set of code changes. Typically, the author drives the review by sitting at the keyboard and mouse, opening various files, pointing out the changes, and explaining why it was done this way.

E-mail pass-around reviews​

This is the second-most common form of informal code review and the technique preferred by most open-source projects. Whole files or changes are packaged by the author and sent to reviewers via e-mail. Reviewers examine the files, ask questions and discuss with the author and other developers, and suggest changes.

The hardest part of the e-mail pass-around is in finding and collecting the files under review.

Tool-Assisted reviews​

This refers to any process where specialized tools are used in all aspects of the review: collecting files, transmitting and displaying files, commentary, and defects among all participants, collecting metrics, and giving managers some control over the workflow.

Pair Programming​

Pair programming is a development process that incorporates continuous code review. Pair programming is two developers writing code at a single workstation with only one developer typing at a time and continuous free-form discussion and review.

Studies of pair programming have shown it to be very effective at finding bugs and promoting knowledge transfer.

References​

Owner: Software Development Team