283 lines
9.1 KiB
Markdown
283 lines
9.1 KiB
Markdown
---
|
||
name: chinese-code-review
|
||
description: 中文 review 沟通参考——话术模板、分级标注（必须修复/建议修改/仅供参考）、国内团队常见反模式应对。仅在用户显式 /chinese-code-review 时调用，不要根据上下文自动触发。
|
||
version: "1.0.0"
|
||
license: MIT
|
||
metadata:
|
||
hermes:
|
||
tags: [code-review, chinese]
|
||
---
|
||
|
||
# 中文代码审查规范
|
||
|
||
## 概述
|
||
|
||
国内团队做 Code Review 常遇到两个极端：要么过度客气导致关键问题被放过，要么照搬西方直白风格让同事下不来台。本技能帮你找到平衡点——**既不回避问题，又让人愿意接受反馈**。
|
||
|
||
**核心原则：** 用"建议"代替"命令"，用"提问"代替"否定"，但绝不因为面子而放过 bug。
|
||
|
||
## 审查反馈的表达方式
|
||
|
||
### 用建议代替命令
|
||
|
||
| 避免（命令式） | 推荐（建议式） |
|
||
|---------------|---------------|
|
||
| 你必须改成 X | 建议考虑用 X，因为 Y |
|
||
| 这里写错了 | 这里可能存在一个问题，是否考虑过 Z 的情况？ |
|
||
| 不要用这个方法 | 这个方法在 A 场景下可能有性能问题，可以看看 B 方案 |
|
||
| 这段代码不行 | 这段逻辑我理解得对吗？如果输入为空的话会怎样？ |
|
||
|
||
### 用提问代替否定
|
||
|
||
当你不确定对方意图时，先问再评：
|
||
|
||
```
|
||
# 好的方式
|
||
这里用 sync 方式读文件是出于什么考虑？如果并发量上来，可能会阻塞事件循环。
|
||
|
||
# 不好的方式
|
||
这里不应该用 sync 方式读文件。
|
||
```
|
||
|
||
### 分级标注
|
||
|
||
统一使用优先级标记，让作者快速判断轻重缓急：
|
||
|
||
- **[必须修复]** — 安全漏洞、数据丢失风险、逻辑错误（不修不能合）
|
||
- **[建议修改]** — 性能问题、可维护性、缺少校验（本次或下次迭代修复）
|
||
- **[仅供参考]** — 命名优化、风格建议、替代方案（不改也行）
|
||
- **[问题]** — 不确定的地方，需要作者解释意图
|
||
|
||
### 审查评论模板
|
||
|
||
```
|
||
[必须修复] 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
|
||
```
|
||
|
||
## 中英混排代码注释规范
|
||
|
||
### 何时用中文
|
||
|
||
- **业务逻辑说明** — 用中文解释业务背景和需求来源
|
||
- **复杂算法注释** — 用中文写思路，确保团队成员都能理解
|
||
- **TODO / FIXME** — 用中文描述待办事项，方便搜索和追踪
|
||
- **文档注释（内部项目）** — JSDoc / Javadoc 中的描述文字用中文
|
||
|
||
```typescript
|
||
/**
|
||
* 计算用户的会员等级折扣
|
||
*
|
||
* 业务规则：
|
||
* - 普通会员 9.5 折
|
||
* - 银卡会员 9 折
|
||
* - 金卡会员 8.5 折
|
||
* - 钻石会员 8 折
|
||
*
|
||
* @param level - 会员等级（MemberLevel enum）
|
||
* @param amount - 原始金额（单位：分）
|
||
* @returns 折后金额（单位：分）
|
||
*/
|
||
function calculateDiscount(level: MemberLevel, amount: number): number {
|
||
// ...
|
||
}
|
||
```
|
||
|
||
### 何时用英文
|
||
|
||
- **变量名、函数名、类名** — 始终用英文命名，遵循团队命名规范
|
||
- **Git commit message** — 参考下方 commit 规范
|
||
- **开源项目注释** — 面向国际社区的项目，注释统一用英文
|
||
- **错误信息和日志** — 生产环境的 error message 用英文（避免编码问题）
|
||
- **API 接口文档** — 对外暴露的 API 用英文
|
||
|
||
### 混排格式要求
|
||
|
||
```typescript
|
||
// 好：中英文之间加空格
|
||
// 使用 Redis 缓存来减少 MySQL 的查询压力
|
||
|
||
// 坏：中英文之间没有空格
|
||
// 使用Redis缓存来减少MySQL的查询压力
|
||
|
||
// 好：技术术语保留英文
|
||
// 这里用 debounce 防抖处理，避免频繁触发 API 请求
|
||
|
||
// 坏：强行翻译技术术语
|
||
// 这里用防抖动处理，避免频繁触发应用程序接口请求
|
||
```
|
||
|
||
## Commit Message 中英双语格式
|
||
|
||
### 推荐格式
|
||
|
||
团队内部项目使用中文 commit message，采用约定式提交（Conventional Commits）的中文版：
|
||
|
||
```
|
||
<类型>(<范围>): <简要描述>
|
||
|
||
<详细说明（可选）>
|
||
|
||
<关联信息（可选）>
|
||
```
|
||
|
||
### 类型对照表
|
||
|
||
| 类型 | 含义 | 示例 |
|
||
|------|------|------|
|
||
| feat | 新功能 | feat(用户): 新增手机号登录功能 |
|
||
| fix | 修复 Bug | fix(支付): 修复微信支付回调重复处理的问题 |
|
||
| docs | 文档变更 | docs: 更新 API 接口文档 |
|
||
| style | 代码格式 | style: 统一缩进为 2 个空格 |
|
||
| refactor | 重构 | refactor(订单): 拆分订单服务，提取公共逻辑 |
|
||
| perf | 性能优化 | perf(列表): 虚拟滚动优化长列表渲染性能 |
|
||
| test | 测试 | test(auth): 补充登录模块单元测试 |
|
||
| chore | 构建/工具 | chore: 升级 Node.js 至 v20 |
|
||
|
||
### 示例
|
||
|
||
```
|
||
fix(支付): 修复支付宝异步回调签名校验失败的问题
|
||
|
||
原因：升级 SDK 后签名算法从 RSA 变为 RSA2，但回调校验仍使用旧算法。
|
||
方案：回调处理中同时兼容 RSA 和 RSA2 签名校验。
|
||
|
||
Closes #1234
|
||
```
|
||
|
||
### 面向国际社区的项目
|
||
|
||
如果项目面向国际社区或有外籍成员，commit message 用英文，PR 描述中可附加中文说明：
|
||
|
||
```
|
||
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
|
||
```
|
||
|
||
## 常见反模式与对策
|
||
|
||
### 反模式一：过度客气
|
||
|
||
**表现：** 所有评论都是"我觉得可能也许大概好像这里有个小问题"。
|
||
|
||
**后果：** 关键 bug 被隐藏在一堆委婉语里，作者根本不知道哪些必须改。
|
||
|
||
**对策：** 使用分级标注。[必须修复] 就是必须修复，语气可以温和，但级别必须准确。
|
||
|
||
```
|
||
# 坏：过度客气
|
||
不知道我理解得对不对，这里好像可能有一点点并发问题，不过也许我看错了...
|
||
|
||
# 好：温和但清晰
|
||
[必须修复] 并发安全问题
|
||
|
||
这里的 map 在多个 goroutine 中同时读写，会触发 panic。
|
||
建议加 sync.RWMutex，或者换成 sync.Map。
|
||
|
||
复现方式：加 -race flag 跑测试就能看到。
|
||
```
|
||
|
||
### 反模式二：不敢给高级开发者提意见
|
||
|
||
**表现：** 高级开发者或 Leader 的代码直接 Approve，不仔细看。
|
||
|
||
**后果：** 代码质量双标，团队对 Code Review 失去信任。
|
||
|
||
**对策：** Code Review 对事不对人。可以换个表达方式：
|
||
|
||
```
|
||
# 提问式（适合给资深同事的反馈）
|
||
想请教一下，这里选择用递归而不是迭代，是出于什么考虑？
|
||
我在想如果递归深度超过 1000 层会不会有栈溢出的风险？
|
||
|
||
# 学习式
|
||
学到了一个新写法！不过有个小疑问——这里的类型断言在运行时不会做检查，
|
||
如果上游数据结构变了，这里会静默通过。是否考虑加个 runtime validation？
|
||
```
|
||
|
||
### 反模式三：审查变成风格之争
|
||
|
||
**表现：** 大量评论纠结于缩进、空格、花括号位置。
|
||
|
||
**后果：** 浪费时间，忽略真正的问题。
|
||
|
||
**对策：** 风格问题交给 ESLint / Prettier / gofmt 等工具自动处理。Code Review 聚焦逻辑、安全、性能。
|
||
|
||
### 反模式四：只写"LGTM"
|
||
|
||
**表现：** 随手一个 LGTM 就 Approve，没有实质性审查。
|
||
|
||
**后果：** Code Review 形同虚设，出了问题没人兜底。
|
||
|
||
**对策：** 即使代码质量很好，也要写出你关注了哪些方面：
|
||
|
||
```
|
||
LGTM
|
||
|
||
审查了以下方面：
|
||
- 并发安全：锁的粒度合理
|
||
- 错误处理：所有外部调用都有 error handling
|
||
- 向下兼容：新增字段都有默认值，不影响老版本
|
||
|
||
一个小建议 [仅供参考]：第 78 行的变量名 `d` 可以改成 `duration`，更易读。
|
||
```
|
||
|
||
## 审查流程建议
|
||
|
||
### 开始审查前
|
||
|
||
1. **先看 PR 描述**，理解改动的背景和目的
|
||
2. **看关联的 Issue 或需求文档**
|
||
3. **先整体浏览**，再逐文件细看
|
||
|
||
### 审查顺序
|
||
|
||
1. **架构层面** — 方案是否合理？有没有更好的方式？
|
||
2. **正确性** — 逻辑对不对？边界条件处理了吗？
|
||
3. **安全性** — 有没有注入、越权、信息泄露？
|
||
4. **性能** — 有没有 N+1 查询、内存泄漏、不必要的循环？
|
||
5. **可维护性** — 半年后能看懂吗？测试覆盖了吗？
|
||
6. **风格** — 只关注工具无法自动处理的部分
|
||
|
||
### 给出总结
|
||
|
||
审查结束后，给一段总结，包括：
|
||
- 整体评价（一句话）
|
||
- 值得学习的地方（先扬后抑）
|
||
- 主要问题列表（按优先级）
|
||
- 建议的修改方向
|
||
|
||
```
|
||
总结：整体实现思路清晰，支付回调的幂等处理很到位。
|
||
|
||
主要问题：
|
||
1. [必须修复] 并发写 map 的问题（2 处）
|
||
2. [建议修改] 缺少对空值的校验（3 处）
|
||
3. [仅供参考] 几个变量命名可以更语义化
|
||
|
||
建议先修复并发问题，校验的部分可以本次一起改或者拆到下个迭代。
|
||
```
|
||
|
||
## 检查清单
|
||
|
||
在提交审查意见前，确认：
|
||
|
||
- [ ] 每条评论都标注了优先级
|
||
- [ ] [必须修复] 的问题都给出了具体的修复建议
|
||
- [ ] 没有因为面子而跳过关键问题
|
||
- [ ] 没有纠结于工具能自动处理的风格问题
|
||
- [ ] 对好的代码给予了肯定
|
||
- [ ] 给出了整体总结
|