Overview
Domestic teams doing Code Review often encounter two extremes: either being overly polite so that critical issues are let through, or copying the Western direct style and making colleagues uncomfortable. This skill helps you find the balance—neither avoiding problems nor making people unwilling to accept feedback。
Core principles: Use "suggestions" instead of "commands", use "questions" instead of "negation", but never let a bug through for the sake of saving face.
How to phrase review feedback
Use suggestions instead of commands
| Avoid (imperative) | Recommended (suggestive) |
|---|---|
| You must change it to X | Consider using X, because Y |
| This is written wrong | There may be an issue here; have you considered the case of Z? |
| Don't use this method | This method may have performance issues in scenario A; you could look at option B |
| This code is no good | Am I understanding this logic correctly? What happens if the input is empty? |
Use questions instead of negation
When you are unsure of the other person's intent, ask before evaluating:
# 好的方式
这里用 sync 方式读文件是出于什么考虑？如果并发量上来，可能会阻塞事件循环。
# 不好的方式
这里不应该用 sync 方式读文件。
Severity labels
Use priority markers consistently so the author can quickly judge what is urgent and what is not:
- [Must fix] — security vulnerabilities, data loss risks, logic errors (cannot merge without fixing)
- [Suggested change] — performance issues, maintainability, missing validation (fix this iteration or next)
- [For reference only] — naming improvements, style suggestions, alternative approaches (fine not to change)
- [Question] — uncertain points; the author needs to explain their intent
Review comment templates
[必须修复] SQL 注入风险
第 42 行：用户输入直接拼接到 SQL 语句中。
原因：攻击者可以通过 name 参数注入 `'; DROP TABLE users; --`。
建议：使用参数化查询：
db.query('SELECT * FROM users WHERE name = $1', [name])
参考：https://cheatsheetseries.owasp.org/cheatsheets/SQL_Injection_Prevention_Cheat_Sheet.html
Guidelines for mixed Chinese-English code comments
When to use Chinese
- Business logic explanations — use Chinese to explain business context and where requirements come from
- Complex algorithm comments — write the approach in Chinese to ensure all team members can understand it
- TODO / FIXME — describe to-do items in Chinese for easier searching and tracking
- Documentation 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 name in English and follow the team's naming conventions
- Git commit message — see the commit guidelines below
- Open source project comments — for projects facing the international community, use English consistently for comments
- Error messages and logs — use English for production error messages (to avoid encoding issues)
- API documentation — use English for externally exposed APIs
Mixed-language formatting requirements
// 好：中英文之间加空格
// 使用 Redis 缓存来减少 MySQL 的查询压力
// 坏：中英文之间没有空格
// 使用Redis缓存来减少MySQL的查询压力
// 好：技术术语保留英文
// 这里用 debounce 防抖处理，避免频繁触发 API 请求
// 坏：强行翻译技术术语
// 这里用防抖动处理，避免频繁触发应用程序接口请求
Bilingual Chinese-English Commit Message format
Recommended format
Internal team projects use Chinese commit messages, adopting the Chinese version of Conventional Commits:
<类型>(<范围>): <简要描述>
<详细说明（可选）>
<关联信息（可选）>
Type reference table
| Type | Meaning | Example |
|---|---|---|
| feat | New feature | feat(user): add phone number login feature |
| fix | Bug fix | fix(payment): fix the issue of duplicate processing of WeChat Pay callbacks |
| docs | Documentation change | docs: update API documentation |
| style | Code formatting | style: unify indentation to 2 spaces |
| refactor | Refactor | refactor(order): split the order service and extract common logic |
| perf | Performance optimization | perf(list): optimize long list rendering performance with virtual scrolling |
| test | Testing | test(auth): add unit tests for the login module |
| chore | Build/tooling | chore: upgrade Node.js to v20 |
Example
fix(支付): 修复支付宝异步回调签名校验失败的问题
原因：升级 SDK 后签名算法从 RSA 变为 RSA2，但回调校验仍使用旧算法。
方案：回调处理中同时兼容 RSA 和 RSA2 签名校验。
Closes #1234
Projects aimed at the international community
If the project targets the international community or has foreign members, write commit messages in English, and you may add a Chinese explanation in the PR description:
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: Being overly polite
Manifestation: All comments are "I think maybe perhaps it seems there's a small issue here."
Consequence: Critical bugs are hidden in a pile of euphemisms, and the author has no idea what must be changed.
Countermeasure: Use severity labels. [Must fix] means must fix; the tone can be gentle, but the severity must be accurate.
# 坏：过度客气
不知道我理解得对不对，这里好像可能有一点点并发问题，不过也许我看错了...
# 好：温和但清晰
[必须修复] 并发安全问题
这里的 map 在多个 goroutine 中同时读写，会触发 panic。
建议加 sync.RWMutex，或者换成 sync.Map。
复现方式：加 -race flag 跑测试就能看到。
Anti-pattern 2: Not daring to give feedback to senior developers
Manifestation: Directly approving the code of senior developers or leaders without looking carefully.
Consequence: Double standards in code quality, and the team loses trust in Code Review.
Countermeasure: Code Review should focus on the matter, not the person. Try a different way of phrasing:
# 提问式（适合给资深同事的反馈）
想请教一下，这里选择用递归而不是迭代，是出于什么考虑？
我在想如果递归深度超过 1000 层会不会有栈溢出的风险？
# 学习式
学到了一个新写法！不过有个小疑问——这里的类型断言在运行时不会做检查，
如果上游数据结构变了，这里会静默通过。是否考虑加个 runtime validation？
Anti-pattern 3: Review turning into a style debate
Manifestation: A large number of comments obsess over indentation, spaces, and brace placement.
Consequence: Wastes time and ignores real problems.
Countermeasure: Let style issues be handled automatically by tools such as ESLint / Prettier / gofmt. Code Review should focus on logic, security, and performance.
Anti-pattern 4: Writing only "LGTM"
Manifestation: Casually approving with an LGTM, without any substantive review.
Consequence: Code Review becomes a mere formality, and no one takes responsibility when problems arise.
Countermeasure: Even if the code quality is very good, write down which aspects you paid attention to:
LGTM
审查了以下方面：
- 并发安全：锁的粒度合理
- 错误处理：所有外部调用都有 error handling
- 向下兼容：新增字段都有默认值，不影响老版本
一个小建议 [仅供参考]：第 78 行的变量名 `d` 可以改成 `duration`，更易读。
Review process suggestions
Before starting the review
- First read the PR description, understand the background and purpose of the changes
- Look at the related Issue or requirements document
- First browse the whole thing, then examine it file by file in detail
Review order
- Architecture level — Is the approach reasonable? Is there a better way?
- Correctness — Is the logic correct? Are boundary conditions handled?
- Security — Are there injection, privilege escalation, or information disclosure issues?
- Performance — Are there N+1 queries, memory leaks, or unnecessary loops?
- Maintainability — Will it be understandable six months from now? Is it covered by tests?
- Style — Focus only on the parts that tools cannot handle automatically
Give a summary
After the review is complete, give a summary that includes:
- Overall assessment (one sentence)
- Things worth learning from (praise first, then critique)
- List of main issues (by priority)
- Suggested directions for changes
总结：整体实现思路清晰，支付回调的幂等处理很到位。
主要问题：
1. [必须修复] 并发写 map 的问题（2 处）
2. [建议修改] 缺少对空值的校验（3 处）
3. [仅供参考] 几个变量命名可以更语义化
建议先修复并发问题，校验的部分可以本次一起改或者拆到下个迭代。
Checklist
Before submitting review comments, confirm:
- Every comment is labeled with a priority
- [Must fix] issues all include specific fix suggestions
- No key issues were skipped to save face
- No getting stuck on style issues that tools can handle automatically
- Good code was acknowledged
- An overall summary was given