SuperAPI AI Directory
Partner with us中文AI newsFavorites
Skillsreview-code

review-code

Testing & quality

View repository

Updated 10-16 01:38

About this skill

Code Review

Overview

A code review guide compiled based on Google engineering practices.Primary purpose: to ensure that the overall code health of the codebase improves over time. There is no "perfect" code, onlybettercode—pursue continuous improvement, not perfection.

Reviewers should approve a CL when it clearly improves the code health of the system, even if the CL is not perfect.

Three major principles:

  1. Technical facts and data override opinions and personal preferences.
  2. On style issues, the style guide is the absolute authority. Any purely stylistic issue not in the style guide is a personal preference and should remain consistent with the existing code.
  3. Software design is almost never a purely stylistic issue or personal preference. Evaluate based on underlying principles, not merely personal opinion.

When to Use

Use cases:

  • Review code changes, PRs, diffs, commits, patches
  • The user says "review", "review", "CR", "take a look at this code", "check this PR", "review it"
  • Audit the code quality of an existing codebase or module
  • As a requesting-code-review reference standard for the review phase

Not applicable:

  • Pure documentation changes, configuration adjustments (unless security configuration is involved)
  • Generated code, data files, automatically formatted code

And requesting-code-review difference: This skill defines reviewstandardsandmethods(what to review, how to review it).requesting-code-review Defines the pre-commitautomated pipeline(security scanning, baseline comparison, auto-fix loop).

Review overview

The review should cover the following dimensions:

Dimension Core question
Design Is the code design reasonable and well suited to the system?
Functionality Does the behavior match the author's intent? Is it beneficial to users?
Complexity Can it be simpler? Can other developers in the future easily understand and modify it?
Testing Are there correct, well-designed automated tests?
Naming Are the names of variables, classes, methods, etc. clear?
Comments Are the comments clear and useful? Comments should explainwhyrather thanwhat。
Style Does it follow the style guide?
Documentation Have the relevant documents been updated accordingly?
Every line Every line of manually written code must be looked at. Data files and generated code may be skipped.
Context Examine it in a broader context—the entire file, the entire system. Do not accept a CL that degrades code health.

Review process

Three-step method for browsing a CL

Step one: overall perspective

  • Read the CL description to understand what the change does and why it is being made.
  • Is the change itself reasonable? If it should not be done at all, reply immediately explaining why and suggesting an alternative.

Step 2: Review the core parts

  • Find the "main" files with the largest amount of change, and review these core parts first.
  • If you find a major design problem, send a comment immediately—do not wait until the other parts have been reviewed. If the design problem is serious, a large amount of subsequent code may be discarded.

Step 3: Review the remaining parts in a reasonable order

  • After confirming there are no major design problems, browse the remaining files in logical order.
  • Sometimes reading the test code before the main code helps in understanding the intent of the change.

Detailed explanation of review standards

Design review (most important)

  • Is the interaction between the code modules in the CL reasonable?
  • Should the change be placed in the current codebase or extracted into a library?
  • Does it integrate well with the rest of the system?
  • Is now the appropriate time to add this feature?

Functionality review

  • Does the CL implement the developer's intent? Is the intent beneficial to users?
  • Think about edge cases, concurrency issues, and think from the user's perspective.
  • Pay special attention to:UI changes(it is difficult to judge the impact by reading code alone),parallel programming(which may cause deadlocks or race conditions).

Complexity review

Check level by level: Is a single line → function → class too complex?

"Too complex" = readers of the code cannot quickly understand it、it is easy to introduce bugs when calling or modifying it。

Be especially wary ofover-engineering: code is made more general than needed, or features are added that the current system does not need. Solvenowknown problems, rather than guessing at problems that may need to be solved in the future.

Test review

  • Require unit tests, integration tests, or end-to-end tests that match the change. Tests should be in the same CL as the production code.
  • Ensure the tests are correct, reasonable, and useful:
    • Will the tests actually fail when the code is broken?
    • Will the tests produce false positives when the underlying code changes?
    • Are the assertions simple and useful?
    • Is there a reasonable separation between different test methods?
  • Tests are also code that needs to be maintained; do not accept complexity in tests.

Naming review

A good name is long enough to fully convey what the item is/does, but not so long that it is difficult to read.

Comment review

  • Comments should usually explainwhyrather thanwhat. If the code itself is not clear enough, the code should be simplified rather than adding comments.
  • Exception: regular expressions and complex algorithms usually need comments explaining what they are doing.
  • Look at comments before the CL: are there TODOs that can be removed, and are there old comments opposing this change?
  • Comments ≠ documentation (documentation for classes/modules/functions should express purpose, usage, and behavior).

Style review

  • Ensure the CL follows the relevant style guide.
  • If you want to improve something beyond the style guide, mark it with the "Nit:" prefix to indicate that it is non-mandatory.
  • Do not block a submission based solely on personal style preferences.
  • A CL should not mix large-scale style changes with functional changes—formatting and functionality should be submitted separately.

Consistency

  • If existing code is inconsistent with the style guide: the style guide is the absolute authority, and the CL should follow the guide.
  • If the style guide is a suggestion rather than a requirement: lean toward following the style guide, unless local inconsistency would cause greater confusion.

Documentation Review

  • If the CL changes how users build, test, interact with, or release the code, check whether the README, documentation pages, and generated reference documentation have been updated accordingly.
  • If the CL deletes or deprecates code, consider whether the documentation should also be deleted.

How to Write Comments

Three Key Points

  1. Explain the reason—let the developer understand the intent of the comment, the best practice to follow, or how the suggestion improves code health.
  2. Balance direct guidance with pointing out the problem and letting the developer decide—point out the problem and let the developer decide, but sometimes a direct instruction, suggestion, or even code is more helpful.
  3. Encourage developers to simplify the code or add code comments, rather than relying on explanations in the review tool. Explanations in the review tool are not helpful to future readers of the code.

Comment Severity Labels

Label Meaning
Nit Minor, technically should be done but has little impact
Optional / Consider Might be a good idea, but not strictly required
FYI Not expected to be changed in this CL, but could be considered in the future

Use labels to make the review intent clear and help the author prioritize.

About Explanations

If you ask a developer to explain a piece of code, the usual result should be that theyrewrite the code to make it clearer. Occasionally adding a code comment is also an appropriate response (provided it is not merely explaining overly complex code). Explanations written in the review tool are not helpful to future readers of the code.

Handling "clean up later"

Developers often say they will clean up in a follow-up CL—experience shows that unless a cleanup CL is writtenimmediately, this rarely actually happens.

  • It is usually best to insist that the developer clean upnowbefore the code enters the codebase. "Clean up later" is a common way for a codebase to degrade.
  • If the CL introduces new complexity, it must be cleaned up before submission (except in emergencies).
  • If the CL exposes surrounding problems that cannot be resolved now, the developer should file a bug and assign it to themselves, and optionally write a TODO in the code referencing that bug.

CL Guidelines

Write a Good CL Description

A CL description should convey:What changes were made? + Why were these changes made?

First-line rule: A short summary (imperative mood) that stands on its own so that future people searching the code history can quickly understand it. Leave a blank line after the first line.

Bad description: "Fix bug." / "Fix build." / "Add patch." / "Phase 1."

Good description:

RPC: 移除 RPC 服务器消息空闲列表的大小限制。

像 FizzBuzz 这样的服务器有非常大的消息，能从复用中受益。
增大空闲列表，并添加一个 goroutine 随时间缓慢释放空闲列表条目，
使空闲服务器最终释放所有空闲列表条目。

Write Small CLs

One CL = one self-contained change, solving only one thing. Usually 100 lines is reasonable, and 1000 lines is too large. The degree to which files are spread out also affects "size"—200 lines in one file may be OK, while being spread across 50 files is usually too large.

Splitting strategies:

  • Stack-based: After finishing a small CL and sending it for review, immediately write the next one based on this CL.
  • Split by file: Group files that need different reviewers, each self-contained.
  • Horizontal splitting: Create shared code or stubs between layers of the tech stack to isolate changes.
  • Vertical splitting: Split by feature, where each feature is an independent full-stack implementation.
  • Refactoring separation: Refactor in an independent CL, not mixed with feature changes or bug fixes.
  • Tests in the same CL: Logic changes should include new or updated tests.

AI Review Execution Guide

Review preparation

git diff --cached
# 如果为空
git diff HEAD~1 HEAD

Review output format

## 审查总结
[1-2 句整体评估]

## 设计层面
- [设计相关评论]

## 关键问题（必须修复）
- 问题 1：[描述 + 建议修复方案]
- 问题 2：...

## 建议（可选）
- 建议 1：[描述]

## 好的实践（肯定）
- [开发者做得好的地方]

## 问题
- [需要开发者澄清的问题]

Common issues quick reference

Python

# 差: SQL 注入风险
cursor.execute(f"SELECT * FROM users WHERE id = {user_id}")
# 好: 参数化查询
cursor.execute("SELECT * FROM users WHERE id = ?", (user_id,))

# 差: Shell 注入
os.system(f"ls {user_input}")
# 好: 安全子进程
subprocess.run(["ls", user_input], check=True)

# 差: 危险 eval/exec
eval(user_input)
# 好: 使用安全的解析方式

JavaScript

// 差: XSS 风险
element.innerHTML = userInput;
// 好: 安全文本内容
element.textContent = userInput;

Signals requiring special attention

  • Hardcoded secrets, API keys, credentials
  • Missing input validation
  • SQL queries using string concatenation
  • File operations without path validation (path traversal risk)
  • External calls lacking error handling
  • Leftover debug print/console.log
  • Commented-out code
  • New code without corresponding tests
  • Over-engineering (abstractions/features not currently needed)
  • Concurrent code not correctly handling race conditions
  • Refactoring and feature changes mixed in the same CL
  • Large-scale formatting changes mixed with logic changes

Common Pitfalls

  1. Focusing only on surface differences—Should review the intent and design of the change, not just the syntactic correctness of the diff.
  2. Skipping test review—The quality and coverage of tests are equally important; tests are code too.
  3. Accepting "clean up later"—It almost never happens. Insist on handling it now.
  4. Style preferences masquerading as design issues—Distinguish style (follow the style guide) from design (trade-offs based on principles). When unsure, use the Nit label.
  5. Ignoring over-engineering—"Might need it later" is not a reason to add abstraction. Solve the problem now.
  6. Reviewing a large CL all at once—Anything over 1000 lines should be required to be split; otherwise review quality cannot be guaranteed.
  7. Mixing features and refactoring—Formatting/refactoring changes should be independent of feature changes and submitted separately.

Verification Checklist

  • The CL description clearly conveys what is done and why
  • Reviewed all 10 dimensions (design, functionality, complexity, tests, naming, comments, style, documentation, every line, context)
  • No changes that degrade code health were accepted
  • Complexity and over-engineering have been evaluated
  • Test coverage is sufficient and test design is reasonable
  • Security signals have been checked (secrets, injection, path traversal, etc.)
  • Comments carry severity labels (Nit/Optional/FYI)
  • "Clean up later" has been rejected (unless there is an immediate follow-up CL)
  • Output format follows the standard template (review summary → key issues → suggestions → good practices → questions)