Chinese Code Review Guidelines

Overview

Expression of Review Feedback

Use Suggestions Instead of Commands

Use Questions Instead of Negations

# Good practice
What's the consideration for using sync to read files here? If the concurrency increases, it may block the event loop.

# Bad practice
You shouldn't use sync to read files here.

Priority Labeling

• [Must Fix] — Security vulnerabilities, data loss risks, logical errors (cannot merge without fixing)
• [Suggested Modification] — Performance issues, maintainability, missing validations (can be fixed in this iteration or the next)
• [For Reference Only] — Naming optimization, style suggestions, alternative solutions (optional to fix)
• [Question] — Uncertain areas that require the author to explain intent

Review Comment Template

[Must Fix] SQL Injection Risk

Line 42: User input is directly concatenated into the SQL statement.

Reason: Attackers can inject `'; DROP TABLE users; --` via the name parameter.

Suggestion: Use parameterized queries:
  db.query('SELECT * FROM users WHERE name = $1', [name])

Reference: https://cheatsheetseries.owasp.org/cheatsheets/SQL_Injection_Prevention_Cheat_Sheet.html

Chinese-English Mixed Code Comment Standards

When to Use Chinese

• Business Logic Explanations — Use Chinese to explain business background and demand sources
• Complex Algorithm Comments — Use Chinese to write the logic to ensure all team members can understand
• TODO / FIXME — Use Chinese to describe pending tasks for easy search and tracking
• Document Comments (Internal Projects) — Use Chinese for descriptive text in JSDoc / Javadoc

/**
 * 计算用户的会员等级折扣
 *
 * 业务规则：
 * - 普通会员 9.5 折
 * - 银卡会员 9 折
 * - 金卡会员 8.5 折
 * - 钻石会员 8 折
 *
 * @param level - 会员等级（MemberLevel enum）
 * @param amount - 原始金额（单位：分）
 * @returns 折后金额（单位：分）
 */
function calculateDiscount(level: MemberLevel, amount: number): number {
  // ...
}

When to Use English

• Variable Names, Function Names, Class Names — Always use English naming, following team naming conventions
• Git commit message — Refer to the commit guidelines below
• Open Source Project Comments — For projects targeting the international community, use unified English comments
• Error Messages and Logs — Use English for production environment error messages (to avoid encoding issues)
• API Interface Documents — Use English for externally exposed APIs

Mixed Format Requirements

// Good: Add space between Chinese and English
// 使用 Redis 缓存来减少 MySQL 的查询压力

// Bad: No space between Chinese and English
// 使用Redis缓存来减少MySQL的查询压力

// Good: Keep technical terms in English
// 这里用 debounce 防抖处理，避免频繁触发 API 请求

// Bad: Forcibly translate technical terms
// 这里用防抖动处理，避免频繁触发应用程序接口请求

Bilingual Chinese-English Commit Message Format

Recommended Format

<类型>(<范围>): <简要描述>

<详细说明（可选）>

<关联信息（可选）>

Type Comparison Table

Example

fix(支付): 修复支付宝异步回调签名校验失败的问题

原因：升级 SDK 后签名算法从 RSA 变为 RSA2，但回调校验仍使用旧算法。
方案：回调处理中同时兼容 RSA 和 RSA2 签名校验。

Closes #1234

Projects Targeting the International Community

fix(payment): fix Alipay async callback signature verification failure

The SDK upgrade changed the signature algorithm from RSA to RSA2,
but the callback handler still used the old algorithm.

Closes #1234

Common Anti-Patterns and Countermeasures

Anti-Pattern 1: Overly Polite

# Bad: Overly polite
I don't know if I understand correctly, but it seems like there might be a tiny concurrency issue here, though maybe I'm wrong...

# Good: Gentle but clear
[必须修复] 并发安全问题

这里的 map 在多个 goroutine 中同时读写，会触发 panic。
建议加 sync.RWMutex，或者换成 sync.Map。

复现方式：加 -race flag 跑测试就能看到。

Anti-Pattern 2: Afraid to Give Feedback to Senior Developers

# Question-based (suitable for feedback to senior colleagues)
I'd like to ask, what's the consideration for choosing recursion instead of iteration here?
I'm wondering if stack overflow will occur if the recursion depth exceeds 1000 layers?

# Learning-based
I learned a new writing style! But I have a small question—this type assertion isn't checked at runtime.
If the upstream data structure changes, this will pass silently. Have you considered adding runtime validation?

Anti-Pattern 3: Review Becomes a Style Debate

Anti-Pattern 4: Only Write "LGTM"

LGTM

Reviewed aspects:
- Concurrency safety: Lock granularity is reasonable
- Error handling: All external calls have error handling
- Backward compatibility: New fields have default values and do not affect old versions

A small suggestion [仅供参考]: The variable name `d` on line 78 can be changed to `duration` for better readability.

Review Process Recommendations

Before Starting the Review

1. First read the PR description to understand the background and purpose of the changes
2. Read the associated Issue or requirement document
3. First browse the overall content, then read each file in detail

Review Order

1. Architecture Level — Is the solution reasonable? Is there a better way?
2. Correctness — Is the logic correct? Are boundary conditions handled?
3. Security — Are there injection, privilege escalation, or information leakage issues?
4. Performance — Are there N+1 queries, memory leaks, or unnecessary loops?
5. Maintainability — Can it be understood in half a year? Is test coverage sufficient?
6. Style — Only focus on parts that cannot be handled automatically by tools

Provide a Summary

• Overall evaluation (one sentence)
• Aspects worth learning (praise first, then critique)
• List of main issues (by priority)
• Suggested modification directions

Summary: The overall implementation logic is clear, and the idempotent processing of payment callbacks is well done.

Main Issues:
1. [Must Fix] Concurrent map write issues (2 places)
2. [Suggested Modification] Missing null value validations (3 places)
3. [For Reference Only] Several variable names can be more semantic

Suggestion: Fix the concurrency issues first; the validation part can be fixed in this iteration or split into the next iteration.

Checklist

• Each comment is labeled with priority
• Specific repair suggestions are provided for [Must Fix] issues
• No critical issues were skipped for the sake of saving face
• No style issues that can be handled automatically by tools were debated
• Good code was praised
• An overall summary was provided ",