Code Review - GCMLab/GCMLab-FEM GitHub Wiki

The code review process is an essential part of contributing tested and well-documented code to the development branch. It forms a part of the larger development workflow and is completed after a pull request is initiated on a feature branch. To ensure that code is adequately reviewed by the team before merging into the development branch, the following guidelines are prepared. The guidelines include a checklist of items to review, a template for the code review comment, and a list of steps in the code review process.

Checklist

Template

Steps in the review process

A list of items to check during a code review is provided below. This is not an exhaustive list of items to check in a code review, but rather the minimum that is required.

Purpose

• Does the feature have a narrow, well-defined, self-contained scope? If it is not clear, then communicate with the developer to figure this out. If it is too large of a scope, communicate with the developer to split the feature into multiple reviews.
• Does the code accomplish the author's purpose?

Implementation

• Could you solve the problem shorter/easier/faster/safer yet functionally equivalent?
• Do you see the potential for useful abstractions?
• Is the functionality already provided by the existing code?

Legibility and style

• Can the concepts be grasped in a reasonable amount of time? Request code clarification, if necessary.
• Does the code follow a consistent style?
• Praise concise/readable/efficient/elegant code

Maintainability

• Read and run the test(s)
• Make suggestions on other useful test cases that can be run
• Leave feedback on code-level documentation, comments, and commit messages
• Refactoring: look for places where code repetition occurs and can be replaced with a function
• Suggest places where a reference to a book/paper could be added to assist other developers in grasping concepts

Review Comments:

• Concise, friendly, actionable comments
• Critique the code, not the author
• Ask for clarification when something is unclear rather than assuming ignorance
• Avoid absolution judgment, i.e. "This can never work", "The result is always wrong"

Below is a sample code review comment, with example comments in brackets. Feel free to copy/paste it and modify it for each specific review.

- [x] Reviewed function(s): [Name of function(s)] 
- [x] Ran test case(s): [Name of test case(s)]
* Purpose of the code: 
* Notes on implementation: [N/A if none]
   * [Consider creating a function to perform [TASKS]]
   * [Consider grouping the [VARIABLES] into a structure array]
   * [Consider changing the [MATRIX] into a sparse matrix]
   * [Consider using the [EXISTING FUNCTION] to perform [TASK]]
* Notes on legibility and style: [N/A if none]
   * [Request comments on [FUNCTION] lines [A]-[B]]
   * [Request consistent variable naming in [FUNCTION]]
   * [Request making function header for [FUNCTION] more detailed]
* Notes on maintainability: [N/A if none]
   * [Request more concise commit messages]
   * [Consider making function to perform [TASK]]
* Suggested test cases: 
   * [Test case to review functionality of [TASK]]

The steps in the code review process are outlined below.

1. Copy the review template (provided above)

2. Go to the Files Changed tab in the pull request of interest. Here you can see all the differences between the develop branch and the feature branch.

   

3. Select the Review Changes button. Here you can paste the code review template and modify it in the comment box. To finalize the review, select one of the three options: Comment, Approve, or Request changes.

   Comment signifies that comments to your questions are required from the developer before the feature is approved.

   Request changes signifies that edits to the branch are required before the feature is approved.

   Approve signifies that the feature is ready to be merged into the develop branch.

   

4. The developer must respond to all reviewer's comments, even if it is just to acknowledge them with 'ok'. This is done in the Conversation tab of the pull request. An easy way to do this is to click on the menu options (three dots) on the right-hand side of the reviewer's comment and select 'Quote Reply'. The comments can be added directly below each of the reviewer's comments.

   

5. If the reviewer selects Comment or Request changes, the developer must make the requested changes or explain why the requested changes are not required. Once complete, request a re-review by selecting the button to the right of the reviewer's name in the Conversation tab.

   

6. Once the reviewer approves the feature, the reviewer must merge the changes into the develop branch and delete the branch. This can be done in the Conversation tab.