simon

Code Review Guide#

Created: June 22, 2021 5:47 PM

Based on Google Code Review Practice and Zaihui Practice

Terms#

To simplify the description, let's standardize the terminology abbreviations.

• TLDR - Too long, Don't Read
• Author - The author of this PR
• CR - Code Review, also referred to as Review
• PR - Pull Request, a request to merge code, including Change List
• Reviewer - The person reviewing the code
• WIP - Working In Progress, under development, cannot be merged
• Comment
  • There are two types of comments in this article
  • Comments in the code, which are for annotations
  • Comments left during the CR process, which are for improvement suggestions
• Code Base - Code repository
• Deadline - Due date
• Judgement Call - A personal choice made based on your engineering conscience
• LGTM - Looks good to me
• NIT
  • NitPick
  • Refers to improvement suggestions that can be ignored
• SG - Style Guide, code style specifications
• UT - Unit Test
• Emergency - Urgent situations where some CR standards can be waived
  • Hotfix
  • When unreasonable deadlines occur, but after the deadline, the missing parts should still be addressed

Purpose#

• To conduct high-quality and rapid Code Reviews to continuously improve the quality of the Code Base
• To impart or learn new knowledge about languages, frameworks, design patterns, etc., through reading code or leaving comments

Standards#

• Ensure the overall health of the Code Base
• Approve PRs as much as possible, even if they are not perfect, as long as they meet principle 1
• Continuous optimization is more important than perfection
• Leave as many improvement suggestions as possible; if the improvement is not mandatory, start with 'nit:' to indicate it is not essential.
• Review comments should ideally provide some improvement suggestions but should also encourage the author to discover better solutions.
• Unless it is an urgent PR, PRs should not compromise the overall quality of the Code Base; what constitutes an emergency will be described later.

What to look for#

Summary-TLDR#

• Code needs to be well-designed
• Functionality should be user-friendly for code users
• Any UI changes must be reasonable and well-executed
• Any concurrent programming must ensure its safety
• Code should be as simple as possible
• Developers should not write features that are not needed yet
• Code should have appropriate UT
• UT needs to be well-designed
• Developers should give everything a concise and clear name
• Code comments should be concise and useful, explaining why rather than what
• Code should be appropriately documented
• Code should comply with style guide specifications

Design#

• Is the interaction design reasonable with other code?
• How is the integration with the overall system?

Functionality#

• What is the author's purpose for the PR?
• Is this purpose good or bad for the system?
• Is concurrent programming necessary, and will it trigger deadlocks or race conditions? Avoid using concurrent models that could cause deadlocks or race conditions.

Complexity#

• Is the PR excessively complex compared to what it should be?
• Definition
  • If the current design makes it easier to introduce bugs when using or modifying this code, then its design is too complex.
  • Over-engineering
    • Developers design the code to be more generic than necessary
    • Or add features that the system currently does not need
    • Future features should be left for future resolution because when this requirement is genuinely needed, the relevant objective environment and scenarios may have changed, making the prematurely written code unsuitable.
    • PS: It is unnecessary to develop ahead of time, but it is necessary to leave room in the design for future requirements.
• Analysis perspective
  • Independent lines of code
  • Functions
  • Classes

Testing#

• Generally, tests must be included in the same PR as the added functionality code unless it is an emergency.
• Test design
  • Guidelines
    • Correct
    • Reasonable
    • Useful
    • Effective
  • Questions to consider
    • Is the data preparation completely consistent with real situations? Do not just construct partial data.
    • Are all successful paths covered?
    • Are all possible 400 errors tested? Is the returned error information validated against expectations?
    • For successful requests, besides the returned data being consistent, is the data in the database accurate?
    • When the code is broken, will the tests fail normally?
    • If the code changes, will false positives occur?
      • For example, setting special data to pass UT, superficially passing UT while having bugs.
    • Does each test have simple and effective validation?
    • Are tests reasonably divided into different test methods?
    • Tests are also code, so do not allow unnecessary complexity just because they are not the main branch.

Naming#

• Are all developer names good?
• Good naming needs to:
  • Contain enough information to describe what it is or what it does
  • But not be so long that it becomes difficult to read

Comments#

• Principles
  • Concise
  • Understandable
  • Necessary
• Content
  • Content that cannot be expressed by code
  • Should include:
    • Reasons behind decisions
    • Why these codes exist
    • todo for future tasks, starting comments with todo
  • Should not include:
    • Should not explain what the code is doing
      • Code functionality should be expressed by the code itself
      • If the code is not concise enough and requires textual descriptions to explain its functionality, then the code needs to be simplified.
      • Exception: Regular expressions or complex algorithms that rely on detailed comments for explanation.
    • Should not explain how the code should be used or what its performance is
      • That is what documentation is for.

Style#

• Within the company, each major language used must have a corresponding enforced and reasonable style guide (SG).
• SG is the absolute authority. PRs should conform to SG, and SG should not adapt to PRs.
• If you want to propose a code style improvement that does not exist in the SG, you need to add "nit" to indicate that it is not mandatory. Personal code style preferences should not slow down the PR approval process.
• Mandatory styles should be added to the SG.
• If a large number of style modifications are made, they must be proposed in a separate PR and not mixed with other functional code.

Consistency#

• Is the PR consistent with the SG?
• If the content of the SG is merely a recommendation rather than a mandatory requirement, then whether to modify it depends on a Judgement call.
  • But still, prioritize conforming to the SG unless the changes would reduce code readability.

Documentation#

• When is documentation needed?
  • When the PR changes the usage, testing, interaction, or release process of the code.
  • If the PR deletes or deprecates code, consider removing related documentation.
  • If documentation is found to be missing, please ask the original author to add documentation!
• Documentation that needs to be modified
  • README
  • Any relevant documentation

Every Line#

• Please read every line of the code you are assigned to review.
• Content you can skim:
  • Data files
  • Generated code
  • Large data structures
• Content you cannot skim:
  • Manually written classes, methods, code blocks
• Some code deserves more attention:
  • This depends on the reviewer's judgement call.
  • For code you do not want to focus too much on, you must at least ensure you understand what the code is doing.
• If you find the code too long or too complex, hindering your review speed and understanding:
  • You should immediately inform the PR author.
  • Wait for the PR author to simplify or clarify the code's purpose before continuing the code review.
• If you understand the code but feel your ability is insufficient to review some parts of it:
  • Ensure to invite a reviewer you believe is capable enough to join the code review.

Context#

• Added code blocks are often only part of the logic and need to be read in conjunction with the surrounding code.
• If there is not much context, you can expand to view it in the code review tool.
• If the context is too complex, you need to download the code locally and use an IDE or other tools to view it.

Good Practices#

• Generally, comments left during reviews are for proposing critical improvement suggestions.
• However, when you find something great in the code, do not hesitate to use comments to express your praise, showing your appreciation for the PR author and encouraging everyone to adopt better practices.
  • For example: "Awesome!"
  • For example: "This code is brilliantly written; I learned a lot."
  • Besides praise, if there are some substantial compliments, it would be even better, as it can resonate with the author.

How to Review a PR#

Summary-TLDR#

• Does it comply with our PR standards?
• Is the change reasonable? If not, it needs to be rejected immediately.
• Check the design of the main parts of the PR; if there are design issues, feedback must be immediate.
• Review the remaining parts in the order of the code logic chain.

Step zero: Check for necessary compliance#

• Is there a related Jira ticket?
• Does the commit message comply with the specifications?
  • Jira-ticket(tag): description
  • tag:
    • feat for functionality
    • fix for bug fixes
    • ut for unit tests
    • mi for table changes
    • refactor for refactoring
    • inf for infrastructure
• Has it been rebased to the target branch?

Step one: Get a general understanding of the changes#

• Review the PR description to understand what it is generally doing.
• Does the change itself make sense?
• If it feels unreasonable, feedback must be immediate.
  • When providing feedback, first acknowledge the other person's work, then give reasons for rejection and suggest the next steps.
• If there are persistent unreasonable PRs, reflect on the development process (both within the team and extended teams).
  • Prevent these unreasonable PRs from being halted before development.

Step two: Check the main parts of the PR#

• Identify the main code logic files.
• If you cannot find them, ask the author or request the author to split the PR.
• First, check for major design issues; if problems arise, immediate feedback is required, and the current CR must be halted.
  • Generally, after a developer submits a PR, they will continue developing. If development is based on this PR, it needs to be stopped quickly to mitigate losses.
  • Design issues require a lot of time to fix, and most functionalities have a deadline. Raising modification requests early is beneficial for meeting deadlines.

Step three: Review the remaining PR in a reasonable order#

• Identify the logical chain of the PR.
• Review according to the logical chain.
• You can read UT first to understand the design concept.

Quick CR#

Summary-TLRD#

• Even when busy, provide timely feedback.
• The maximum feedback time is within one working day.
• If you are focused on work, do not interrupt your current task.
• Large PRs should be split into smaller PRs; if they cannot be split, step one review must still be completed.
• If the complexity of the code slows down the CR speed, immediately provide feedback to the author to simplify or explain.

Reasons#

• Slow CR can reduce team efficiency and block others' development.
• Slow CR may lead developers to resist the CR process.
• The quality of the code in the Code Base will be affected.
  • On one hand, the inability to effectively merge code improvements.
  • On the other hand, slow CR can lower the quality of developers' work.

How fast should it be?#

• Unless you are focused on a task, you should start reviewing immediately upon receiving a CR request.
• The maximum feedback time should not exceed one working day.

Speed VS Interruptions#

• Ensuring not to be interrupted is more important than the speed of CR.
• Because recovering from interruptions to a focused state takes a lot of time.
• Choose a point of focus to start the CR.

Quick Response#

• Even if you are busy, you should respond promptly.
• If you are very busy, you can first provide some general opinions, which corresponds to the previous step one.

Cross-Time Zone CR#

• Ensure to complete the CR before the other party's off-duty time; otherwise, the CR process will be significantly prolonged due to time differences.

LGTM with comments#

• LGTM means it can be merged.
• It is important to note that LGTM's essence is that this code meets "our" standards, not "my" standards.
• Sometimes LGTM may come with comments, but it does not affect the approval.
  • The reviewer has confidence in the developer, knowing that they will resolve the issue later.
  • Remaining comments are not critical and do not need to be resolved.

Large PRs#

• Request the author to split the PR into smaller PRs.
• If it cannot be split, then at least go through the overall design, which corresponds to step one.
• Ensure not to block the next development process while lowering code quality.

What the Author Should Do in CR#

Associate a Jira ticket#

• The content of the PR must align with the Jira ticket.
• If there are different tasks, a new ticket needs to be created and associated, and a separate PR submitted.

Submit via a feature branch#

• Do not submit a CR using a branch that has the same name as the main branch on your remote.
• Do not use unstable branches like alpha-only or free-only for testing.
• Develop and submit the CR based on a feature branch created from the target branch of the code base on your remote.

Review your PR from a Code Review perspective#

• Refer to Section Four for details.

Complete smoke test self-check#

• Before submitting a CR request, self-testing must be completed.
• The testing lead will provide a smoke test checklist that needs to be fully completed.

Comply with commit message specifications#

• One PR, one ticket, one commit.
• Associate with the Jira ticket.
• Remember to remove the WIP prefix.
• Format: See Step Zero in Section Five.

Ensure the pipeline passes#

• Code style
• Test
• Build

Designate necessary reviewers#

• For new functional logic, in addition to the author, a backup person should be assigned as the primary reviewer so that the same logic is known by more than one person.
• For changes to old logic, it needs to be assigned to the original author of the code for review, as they are more familiar with this part of the business logic and historical issues. The original author's information can be obtained through git blame.

Follow up on the CR progress, providing necessary help and feedback#

• Once the CR is submitted, the author has the obligation and responsibility to push for the PR to be quickly merged into the code base.
• If the code logic chain is relatively long, you can help the reviewer quickly understand the main logic chain through the PR description or by directly explaining to the reviewer. However, be careful to minimize interruptions to the reviewer, only providing verbal descriptions when the reviewer indicates they are starting the CR.
• For comments raised by the reviewer, they need to be replied to in comment form, indicating whether modifications will be made. If no modifications are planned, reasonable reasons must be provided. If the reviewer still disagrees, offline discussions or involving a second reviewer for arbitration may be necessary.
• If the PR has not been reviewed within one working day after submission, it should be raised during the next morning meeting.

After PR merging, promptly submit for testing#

• Merging the PR indicates that this code can enter the testing process.
• If it is a joint front-end and back-end development, generally, the front-end will submit for testing. Pure back-end development functionality will be submitted for testing by the back-end.
• However, following the principle of step-by-step testing, back-end PRs can also be submitted for testing separately after merging, which needs to be confirmed with testing in advance.

Starting the Review#

Principles:

• The reviewer's mental state must be complete.
• The reviewer should not be in a flow state of work.
• The reviewer and author can interact efficiently.

Conclusion:

• CR needs to have a fixed time.
• Recommended to be one hour after dinner.

Review Proposal and Process @Ehco Zhou#