Overview

While this document is for reviewers, it’s also important for coders to know what reviewers look for in code changes. “You” refers to the reviewer.

Everyone, including the most senior members, has their code reviewed and approved before it may be added to the codebase.

Even if you aren’t specifically the reviewer for the change, you are still encouraged to ask questions about the changes! You can preface your comments to make it clear that it isn’t part of a formal review (e.g. “Out of curiosity, …”, “Just for interest, …”).

Background

In a collaborative environment with multiple contributors, higher standards are required than in a single contributor environment. Code is read far more often than it is written, so being able to clearly communicate intent and reasoning is important for coordination. Low quality code slows down overall development, even if it is easier and momentarily faster to just add to the codebase directly.

Code reviews provide an additional point of view to catch any mistakes or structural issues that the coder may have made, and ensures that only high quality code is added to the codebase. Additionally, code reviews are an opportunity to learn about coding practices and to learn more about the codebase.

Code quality

Code quality describes how easy it is for a 1st time coder to understand what the code does and the design reason, assuming that they have the required theoretical knowledge and coding skills.

The coder starts off not knowing anything, so they pick some entry point. From there, the coder expands the search by following variable names, functions that are called, and functions that call the current code. The coder has a limited view (e.g. they see the codebase through a cardboard tube), and so the amount of time they spend understanding the code is proportional to the amount of code they have to look at.

High quality code minimizes the amount of code the coder has to go through. The clearer the interfaces and implementation, the easier it is for the coder to immediately understand which code is relevant and what will be impacted if a change is made (and therefore which code to skip reading). Additionally, high quality code is easy to test, which catches bugs early and makes it easy to find the source of bugs, minimizing the coder’s time. High quality code also makes it easy to write new code and for coders to coordinate (e.g. tasks don’t impact other tasks, merge conflicts are easy to resolve).

Low quality code is unintuitive in purpose, implementation, and scope. Since the code is unclear, the coder lacks confidence in their understanding of the code. The amount of code they must go through increases to infinity as they have to backtrack multiple times due to potential hidden side effects (i.e. code that seems unrelated is somehow actually affected). As the impact of changes are unknown, it is harder for the coder to write new code or improve existing code through fear of breaking something. Additionally, low quality code is difficult to test, which ends up pushing the discovery of bugs and crashes into production (e.g. flight tests and competition), which wastes even more time.

Coders, especially new coders, follow the convention of existing code. This is why it is important to have existing high quality code and to prevent low quality code from entering the codebase. Having high quality code in the codebase also reduces the frequency and scope of refactoring old code, which is good because refactoring is generally tedious, unrewarding, and causes development to halt.

Conducting a review

A review is a professional and non confrontational criticism, following the team charter’s expectations of member behaviour: Expectations of Members . A review improves both the quality of code that would enter the codebase and the knowledge and skills of the coder requesting the review.

Google is an additional resource on how to conduct a code review (their name for code changes is changelist (CL)): https://google.github.io/eng-practices/review/reviewer/

Timeline

The response time from request to review is no more than 3 days. For example:

• Wednesday morning: Request is made

• Saturday evening: The code has been reviewed by this time

A request includes rerequests after a review.

If there has been no response after 3 days, the coder can send a reminder ping.

Suggested order

Every reviewer has their own style of reviewing code, so this is just a suggestion.

For the 1st review of a particular change:

1. Get an overview of what’s changed by going through the changes without making any comments

   1. Figure out the general code path, don’t check specifics

   2. Make some notes if you see any obvious errors, don’t comment yet

2. Go through each file and line in order, and make comments in order

   1. By keeping comments in order, it makes it organized for the coder and any future reviewers (e.g. you when you rereview)

   2. Each comment is an independent topic; there can be multiple comments for a single line and a single comment for multiple lines

Rereviewing:

1. Change the view so that the changes are from the last time you reviewed

2. Go through your previous comments and either resolve them or add additional detail

   1. It is preferable to resolve the comment and then make a new comment for a new topic

3. Change the view so that all changes are visible (i.e. what would be merged)

4. Follow the steps for 1st review for new comments

Generally, comments start broad and become more specific on every rereview. For example: It doesn’t make sense for you to make comments on formatting if you ask for that particular code to be removed.

GitHub workflow

Reviewing:

1. In the PR, click on Pull requests at the top

2. Click on the open PR to review

3. Click on Files changed under the title to the right

4. Click on the line number to open the comment box

   1. You can also click and drag to select multiple lines to comment on, but they have to be contiguous

5. Make the comment

   1. These comments are not visible until your review is submitted

6. Click on Start a review

   1. This will change to Add review comment for additional comments

7. Repeat steps 4-6 for each comment

8. Click on Review changes in the top right

9. Make a summary comment (e.g. Reviewed. , Approved. , Did you test this with Mission Planner? )

10. Click on Submit review

11. Autonomy subteam: In the #auto-pr-reviews Discord channel, respond to the coder who requested the review with a ping stating that you have reviewed/approved

    1. Follow up with a call if desired

You can also make independent comments, which are visible immediately.

Making comments

Every review comment states or implies the action is required by the coder. Be specific and clear about what you expect to see in the code. If asked for the reasoning behind the comment, you are able to justify the reasoning in a technical manner or existing convention. For example:

• Can this be replaced with a single for loop and break out early?

• Add units

• Use [class] as it is better suited for [reason]

• Remove [operation] because [reason]

• Replace with [code example]

• Add empty line above this line

• Remove empty line

• 2 empty lines between imports and global constants

If there is still disagreement or the coder is not doing what you expect, schedule a call to clarify what is happening. Listen to the coder and have them explain what they think of what is expected and/or the reason for their coding approach. Most of the time, it is simply a misunderstanding, either on the coder side (e.g. they did not realize what you wanted done) or the reviewer side (e.g. you did not know that what you are asking for is impossible).

If this is not resolved in the call, you can ask senior members, project managers, and/or leads for their opinion. Also useful: Resolving Engineering Disagreements

Using nit: Prefacing the comment with Nit: means that the code can be approved and merged even if the comment is not addressed or resolved.

Responding to review comments

“You” refers to the coder in this section only.

Reviews are not personal! The goal of the reviewer is to help you improve your code. Reviews are a learning experience and it is normal to have a lot of comments asking for modification at the start. Anyone being onboarded, including experienced software developers, have some ramp up time as they get used to the team’s collaborative environment (e.g. codebase, tools, processes).

If you are unsure, ask the reviewer for clarification by replying to the reviewer’s comment. If the reviewer is unsure about something, it’s worth adding a comment in your code, since others reading your code in the future might have that same question/suggestion (and review comments are not the appropriate location for documentation). You do not need to respond directly to every comment (i.e. changing your code can be sufficient for addressing the reviewer’s comment).

Comments are only resolved by the reviewer, never by the coder. Otherwise, you will confuse the reviewer, since they use the resolved/unresolved state to confirm that the code meets the quality to their satisfaction. Instead, you can use a reaction or respond to the comment for your own tracking purposes.

More information about responding:

Approval

Criteria

Use this criteria as a starting point for your reviews. Summarized and modified from here:

Verification

While not mandatory, it is recommended for you to checkout the coder’s branch and run the unit tests, integration tests, and programs to verify functionality. There are some bugs that are not apparent until the program is actually run.

Approving and merging

There are 3 states of approval for a change:

• Not approved: The code hasn’t been reviewed yet, there are still unresolved comments for the coder to address, and/or tests are failing

  • GitHub: When submitting a review, use Comment or Request changes

    • Request changes will block merging, even if other reviewers have approved the PR

• Conditionally approved: There are unresolved comments, but you trust the coder to be able to correctly address them without requiring you to rereview before merging

  • GitHub: When submitting a review, use Approve button

• Approved: The code is reviewed and there are no unresolved comments so the code can be merged

  • GitHub: When submitting a review, use Approve button

After approval, only the coder merges the changed code. This allows the coder to do additional minor cleanup and/or optimization that they thought of after approval. The coder is trusted to rerequest a review if the changes are large enough.

Broken codebase

If the codebase is broken, merging is blocked unless the change is specifically to fix the codebase.

The codebase is considered broken if there is a bug or error that causes a compile time or run time failure in test(s) and/or the program. Code that does not work is automatically low quality, regardless of how understandable or organized it is. It increases the amount of code for the coder to go through by increasing the uncertainty of where the error originates, and it prevents the coder from running the tests and programs for verification.

Emergencies

Bypassing code review and merging changes directly into the codebase is not allowed. No exceptions. Instead, use an intermediate branch for temporary code that hasn’t been approved. If a change is blocking another task, the other coder can base off of the intermediate branch, although this is not recommended as it makes the codebase disorganized by entangling the code changes and making it unclear which branch is the authoritative source.

Coders do not base off an intermediate branch unless approved by the project manager or lead.

Reverting

If the change is undesirable for any reason (e.g. broken but somehow not caught until after it was merged), it can be reverted. Once reverted, the review process to get the change merged back in is repeated.

Reverting can cause dependency issues, so it is usually better to fix the code in a new change and prioritize that review.