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 unify 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 are for annotations
  • Comments left during the CR process 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
  • Minor suggestions that can be ignored
• SG - Style Guide, coding 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 supplemented.

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 whenever 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 find better solutions.
• Unless it is an urgent PR, the PR 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 parallel programming must ensure safety.
• Code should be as simple as possible.
• Developers should not write features that are not currently needed.
• 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 coding style 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? Try to avoid using concurrent models that can cause deadlocks or race conditions.

Complexity#

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

Testing#

• Generally, tests must be included in the same PR as the added functionality, 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 construct only partial data.
    • Are all successful paths covered?
    • Are all possible 400 responses tested? Is the returned error message validated against expectations?
    • For successful requests, besides returning consistent data, 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 make UT pass superficially while there is actually a bug.
    • 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 names given by developers 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:
    • The reasons behind decisions.
    • Why these codes exist.
    • Todo: 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 a textual description 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 it should perform.
      • That is the job of documentation.

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 comply with SG, and SG should not adapt to PRs.
• If you want to propose a code style improvement not present in the SG, you need to add "nit" to indicate 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 requirement, then whether to modify depends on Judgement Call.
  • But still prioritize compliance with SG unless the change 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.
• You can skim through:
  • Data files
  • Generated code
  • Large data structures
• You cannot skim through:
  • Manually written classes, methods, code blocks.
• Some code deserves more attention:
  • This depends on the reviewer's judgement call.
  • For code that 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 part of it:
  • Ensure to invite a reviewer you believe is capable enough to join the code review.

Context#

• Added code blocks often represent only part of the logic and need to be read in conjunction with the surrounding context.
• 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 suggesting improvements.
• However, when you find something great in the code, do not hesitate to use comments to express your praise, show appreciation for the PR author, and encourage everyone to adopt better practices.
  • For example: "Awesome!"
  • For example: "This code is brilliantly written; I learned from it."
  • Besides praise, if there are some substantial compliments, that 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 given immediately.
• Review the remaining parts in the order of the code logic chain.

Step Zero: Compliance with Necessary Standards#

• Is there a related Jira ticket?
• Does the commit message comply with the standards?
  • Jira-ticket(tag): description
  • tag:
    • feat: feature
    • fix: fix
    • ut: unit test
    • mi: table modification
    • refactor: refactor
    • inf: infrastructure
• Has it been rebased onto 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 seems unreasonable, feedback must be given immediately.
  • When providing feedback, first acknowledge the other person's work, then give reasons for rejection and suggest next steps.
• If there are continuous unreasonable PRs, reflect on the development process (both within the team and extended teams).
  • Stop these unreasonable PRs before development begins.

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, feedback must be given immediately, 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 minimize losses.
  • Design issues require a lot of time to fix, and most functionalities have a deadline; raising modification requests early is beneficial to meet the deadline.

Step Three: Review the Remaining PR in a Reasonable Order#

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

Quick CR#

Summary-TLDR#

• Even when busy, provide timely feedback.
• The longest feedback time should be 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.
• When the complexity of the code slows down the CR speed, immediate feedback should be given to the author to simplify or provide explanations.

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 improves quality.
  • On the other hand, slow CR reduces 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 longest 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 it takes a lot of time to regain focus after an interruption.
• Choose a point of focus to start the CR.

Quick Response#

• Even if you are busy, you should provide a timely response.
• If you are extremely busy, you can first provide some general opinions, which corresponds to step one above.

Cross-Timezone 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 the essence of LGTM is that this code meets "our" standards, not "my" standards.
• Sometimes LGTM may also have comments, but it does not affect the approval.
  • The reviewer has confidence in the developer, knowing 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, try to review the overall design, i.e., step one.
• Ensure not to block the next steps of development while lowering code quality.

What the Author Should Do in CR#

Associate a Jira Ticket#

• The content of the PR must be consistent 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 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 for the code base on your remote.

Review Your PR from a Code Review Perspective#

• Refer to Section Four for details.

Complete Smoke Test Self-Testing#

• Self-testing must be completed before submitting a CR request.
• Testing experts will provide a smoke test checklist that needs to be fully completed.

Comply with Commit Message Standards#

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

Ensure Pipeline Passes#

• Code style
• Test
• Build

Specify Must-Review Assigner#

• For new functional logic, in addition to the author, a backup person should be designated as the main 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 CR Progress, Provide 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 it, but be careful to minimize interruptions to the reviewer, only providing verbal descriptions when the reviewer indicates they are starting the CR.
• All comments raised by the reviewer need to be responded to in comment form, indicating whether modifications will be made. If not planning to modify, provide reasonable reasons. 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 needs to be raised in the next morning meeting.

After PR Merging, Submit for Testing as Soon as Possible#

• Merging the PR indicates that this code can enter the testing process.
• If it is a joint development between front-end and back-end, 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 stepwise testing, back-end PRs can also be submitted for testing separately after merging, which needs to be communicated and confirmed with the testing team in advance.

Beginning of 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.
• It is recommended to do it an hour after dinner.

Review Proposal and Process @Ehco Zhou#