开发篇 · 02 · 代码评审
本章目标
让代码评审成为团队质量防线，而不是形式主义的"点赞走流程"。
一、代码评审的三个价值
- 发现 Bug：在合入前找到问题，比测试阶段便宜 10 倍
- 传递知识：新人看老人的代码快速学习
- 统一风格：团队代码质量趋向一致
二、评审的基本流程
开发自检 ──► 提 MR/PR ──► CI 通过 ──► 指定 Reviewer ──►
├─► Approved ──► 合入
└─► Request Changes ──► 修改 ──► 重新评审
2.1 提 MR 前开发必做的 5 件事（Self Check）
- 本地构建通过
- 本地测试通过
- 删除调试代码（console.log / print / TODO）
- 自己过一遍 diff（checkout 后从头看一次）
- MR 描述完整（参照模板）
2.2 Reviewer 指派策略
| 策略 | 场景 |
|---|---|
| 按模块 owner | 明确的代码所有权 |
| 按语言/技术栈 | 前端变更找前端 reviewer |
| 轮值制 | 均衡负担 |
| 随机 + 必选 | 至少 1 位随机 + 1 位必选（模块 owner） |
原则：
- 至少 1 位 reviewer
- 大改动（> 500 行）至少 2 位
- 架构级变更必须有架构师参与
三、评审清单（通用）
3.1 正确性
- 逻辑是否正确（理解需求）
- 边界条件处理完整（空、null、0、最大值）
- 异常路径有处理
- 并发/竞态条件考虑
- 数据类型正确（int/float、有符号/无符号、时区）
3.2 可读性
- 命名清晰（变量、函数、类）
- 函数不超过 50 行（有合理例外）
- 嵌套不超过 3 层
- 复杂逻辑有注释（注释解释"为什么"，不是"做什么"）
- 魔法数字用常量替代
3.3 可维护性
- 无重复代码（DRY）
- 职责单一（SRP）
- 依赖方向合理（不循环依赖）
- 配置与代码分离
- 日志级别使用得当
3.4 性能
- 无 N+1 查询
- 无循环里的网络调用
- 批量处理合理（避免单条提交）
- 缓存策略合理
- 大对象及时释放
3.5 安全
- 输入校验（SQL 注入、XSS、CSRF）
- 敏感信息不进日志（密码、Token、个人信息）
- 密钥不硬编码
- 权限校验到位
- 错误信息不暴露内部细节
3.6 测试
- 新增代码有单元测试覆盖
- 测试用例覆盖正常流 + 异常流
- 测试命名清晰（
should_return_error_when_input_is_empty） - 无跳过的测试（除非有合理注释）
- 覆盖率达标（业务代码 ≥ 80%）
四、按语言的特定检查点
4.1 TypeScript / JavaScript
- 使用
const/let，禁用var - 没有滥用
any - Promise 都有
.catch或 try-catch - 异步函数不吞异常
- 没有未处理的
unhandledRejection - null/undefined 区分清楚
4.2 Python
- 类型注解完整
- 异常类型具体，不用裸
except: - 资源用
with管理 - 字符串格式化用 f-string
- 可变默认参数陷阱（
def f(l=[]):）
4.3 Java / Kotlin
- null 处理（Optional / @Nullable）
- 资源关闭（try-with-resources）
- 线程安全（共享变量的同步）
- equals/hashCode 同步实现
4.4 Go
- error 都有处理（不忽略）
- goroutine 有退出机制
- channel 避免泄漏
- context 正确传递
- 切片/map 的零值使用
4.5 SQL
- 索引命中（关键查询 EXPLAIN 过）
- 分页用
LIMIT + OFFSET的场景控制数据量 - 大表更新有 WHERE
- 避免
SELECT * - 事务范围合理
五、评审的礼仪
5.1 评审者的态度
✅ 应该：
- 先肯定做得好的地方
- 提问而不是命令（"这里用 X 会不会更好？"）
- 区分"必须改"和"建议"（用标签
must-fix/nit） - 提供具体建议和例子
- 尊重设计决策（作者可能比你更了解上下文）
❌ 不应该：
- 人身攻击（"这代码写得烂"）
- 风格之争（已有 lint 规则的不要争）
- 吹毛求疵（把 nit 当 must-fix）
- 拖延（> 24 小时不响应）
5.2 作者的态度
✅ 应该：
- 及时响应评审意见
- 对每条意见回复（改 / 不改 + 理由）
- 不同意时礼貌讨论，而非对抗
- 修改后清晰标记哪些已处理
❌ 不应该：
- 把评审当成"审判"
- 消极抵抗（改了但没真改）
- 一次合入太多变更，让 reviewer 无从下手
六、评审意见的分级标签
约定几个标签，方便 reviewer 表达意见：
| 标签 | 含义 | 作者是否必须处理 |
|---|---|---|
must-fix / block | 阻塞合入的问题 | ✅ 必须改 |
should-fix | 建议修改 | ⚠️ 通常改，除非有理由 |
nit / nitpick | 细节建议 | 🟢 可选 |
question | 提问，不是意见 | 💬 解释即可 |
suggestion | 改进建议 | 🟢 可选 |
praise | 写得好 | 🎉 享受 |
七、评审的常见反模式
| 反模式 | 问题 | 正确做法 |
|---|---|---|
| LGTM but never reads | 走流程 | 至少扫一遍关键改动 |
| 评审 3000 行的 MR | 看不过来 | 要求拆分 MR |
| 只评论风格 | 漏掉逻辑问题 | 先看逻辑，风格让 lint 管 |
| 同一问题反复提 | 效率低 | 记在团队 wiki，以后引用 |
| 意见太多一次性丢 | 作者崩溃 | 分轮次，先关键再细节 |
| 24h 后才评审 | 拖延节奏 | 约定 SLA（如 4 小时内） |
八、AI 辅助代码评审
8.1 适合 AI 做的检查
- 明显的安全漏洞（SQL 注入、XSS）
- 代码风格一致性
- 简单的逻辑错误（null 判断、边界）
- 注释/命名建议
- 单元测试覆盖率检查
8.2 不适合 AI 做的检查
- 业务逻辑正确性
- 架构层面的设计判断
- 团队特定约定
- 历史代码兼容性
8.3 Prompt 示例
你是一位资深代码评审员。请评审以下代码 diff。
## 检查维度
1. 逻辑正确性
2. 边界条件处理
3. 安全风险
4. 性能问题
5. 可读性
## 输出格式
对每个问题输出：
- 文件 + 行号
- 严重程度（must-fix / should-fix / nit）
- 问题描述
- 建议
## Diff
[粘贴 git diff]
详见 05 AI 辅助开发。
九、评审的度量
9.1 跟踪的指标
| 指标 | 目标 |
|---|---|
| MR 平均评审响应时长 | ≤ 4 小时 |
| MR 平均合入时长 | ≤ 2 天 |
| 合入后的 Bug 率 | 持续下降 |
| 每个 MR 的平均评审轮次 | 1-2 轮 |
| 每个 MR 的平均变更行数 | ≤ 300 行 |