93 lines
6.5 KiB
Markdown
93 lines
6.5 KiB
Markdown
# **4.2 代码审查 (Code Review) 规范**
|
||
|
||
代码审查（Code Review, 简称 CR）不仅仅是发现 Bug 的防线，更是**团队知识共享、架构共识对齐**的最重要途径。本规范旨在建立一套高效、客观、充满建设性的 CR 流程。
|
||
|
||
### **4.2.1 审查的触发时机与前置条件 (Prerequisites)**
|
||
|
||
为了不浪费审查者（Reviewer）的时间，提交者（Author）在发起合并请求（PR/MR）前，**必须满足以下前置条件（即质量门禁）**，否则 Reviewer 有权直接打回：
|
||
|
||
1. **自动化门禁通过 (绿灯状态)**：
|
||
* 代码必须通过所有的 Linter 格式化检查（如 Ruff, ESLint）。
|
||
* 必须通过所有的单元测试与核心自动化测试。**不要让人类去 Review 机器能检查出来的缩进和语法错误。**
|
||
2. **提交规模限制 (Small PR)**：
|
||
* 一个 PR 的逻辑代码变更行数建议**控制在 400 行以内**。超过 1000 行的“巨型 PR”极难审查，往往会流于形式（"LGTM \- Looks Good To Me"）。
|
||
* 如果是大型需求，必须拆分为多个独立的小 PR 提交。
|
||
3. **完成自我审查 (Self-Review)**：
|
||
* 作者在指派 Reviewer 之前，必须先自己完整看一遍 Diff，清理掉调试代码（如 print(), console.log）、注释掉的废弃代码。
|
||
4. **规范的 PR 描述**：
|
||
* 必须清晰描述“修改了什么”、“为什么修改”、“如何测试”。
|
||
|
||
### **4.2.2 审查核心 Checklist (Review Dimensions)**
|
||
|
||
Reviewer 在审查时，应当自顶向下，先看架构，再看逻辑，最后看细节。严禁在没有看懂业务逻辑的情况下，只揪着变量命名不放。
|
||
|
||
#### **1\. 架构与设计 (Design & Architecture) \-\> *最高优先级***
|
||
|
||
* **分层边界**：是否出现了越界操作？（例如：在 Controller/View 层直接拼接了复杂的 SQL，或者在 Serializer 中写了极重的业务流转）。
|
||
* **设计原则**：是否遵循了 DRY（不重复造轮子）原则？是否有过度设计？
|
||
* **向后兼容性**：修改现有的 API 或数据库字段，是否会引发线上老版本 App 的崩溃？
|
||
|
||
#### **2\. 业务功能正确性 (Functional Correctness)**
|
||
|
||
* **需求契合**：代码是否真正解决了 PR 描述中的问题？
|
||
* **边界条件**：空值、负数、极大值、并发场景下的数据状态是否被妥善处理？
|
||
* **事务一致性**：涉及多个表的写入时，是否正确使用了数据库事务（如 @Transactional 或 transaction.atomic）？
|
||
|
||
#### **3\. 安全与性能隐患 (Security & Performance)**
|
||
|
||
* **安全漏洞**：是否存在 SQL 注入风险（直接拼接 SQL 字符串）、XSS/CSRF 风险？越权漏洞（没有校验 owner \== current\_user）？
|
||
* **性能瓶颈**：是否存在潜在的 N+1 查询？是否在大循环中操作了数据库或发起外部网络请求？是否进行了会导致内存溢出的大列表全量加载？
|
||
|
||
#### **4\. 可测试性与健壮性 (Testability & Robustness)**
|
||
|
||
* **异常处理**：有没有吞噬异常（空的 except/catch）？是否抛出了合理的业务错误码，还是直接把系统底层报错扔给了前端？
|
||
* **测试覆盖**：新增的核心逻辑是否包含了对应的单元测试？
|
||
|
||
### **4.2.3 基于变更类型的动态审查重点**
|
||
|
||
根据提交的代码类型，Reviewer 的关注点应当有所侧重：
|
||
|
||
* 🆕 **新功能 (Feature)**: 重点关注 **\[功能正确性\]** → **\[设计合理性/扩展性\]** → **\[代码质量\]**。
|
||
* 🐛 **缺陷修复 (Bugfix)**: 重点关注 **\[问题根因是否找准\]** → **\[修复是否引入回归风险\]** → **\[是否补充了防回归测试\]**。
|
||
* ♻️ **重构 (Refactor)**: 重点关注 **\[系统向后兼容性\]** → **\[设计是否得到改进\]** → **\[性能与测试保持\]**。（重构 PR 原则上不应包含新业务逻辑）。
|
||
|
||
### **4.2.4 Review 礼仪与沟通规范 (Etiquette)**
|
||
|
||
代码审查极其容易引发工程师之间的情绪对立。良好的沟通方式是 CR 顺利推行的润滑剂。
|
||
|
||
* **对事不对人**：
|
||
* ❌ 错误表达：“*你这里写的完全不对，你为什么不用 Map？*”
|
||
* ✅ 正确表达：“*这段逻辑如果改用 Map 结构，查找复杂度是不是可以从 O(n) 降到 O(1)？我们试一下如何？*”
|
||
* **区分严重程度 (使用标签)**：
|
||
* 建议在评论前加上标签，明确反馈的性质，避免作者过度恐慌或误解：
|
||
* \[Blocker\] / \[Must-Fix\]：致命错误、安全漏洞、严重架构违规。**必须修改才能合并**。
|
||
* \[Suggestion\] / \[Should-Fix\]：强烈的优化建议（如代码结构、性能）。建议修改。
|
||
* \[Nit\] / \[Optional\]：细枝末节的挑刺（Nitpick，如命名规范、稍微优雅一点的写法）。作者可凭主观意愿决定是否修改，**不阻塞合并**。
|
||
* \[Question\]：纯粹的疑问，希望能解释一下为什么这么写。
|
||
* **提出问题，更要给出建议**：如果指出了一段代码很糟糕，请尽量提供一段伪代码或者文档链接作为优化参考。
|
||
* **不要吝啬赞美**：如果看到一段非常优雅的实现、极其周全的测试，请直接在评论里留下 👍 或夸奖。正向反馈能极大提升团队技术氛围。
|
||
|
||
### **4.2.5 标准的 PR 描述与审查总结模板**
|
||
|
||
提交者发起 PR 或 Reviewer 完成一次重大审查时，应尽量使用以下结构化的 Markdown 模板：
|
||
|
||
\#\# 📋 变更摘要 (Summary)
|
||
\* \*\*变更类型\*\*: \[🆕新功能 | 🐛缺陷修复 | ♻️重构 | ⚡性能优化\]
|
||
\* \*\*关联 Ticket\*\*: \#Issue-123
|
||
\* \*\*核心目的\*\*: 解决了订单并发退款时的状态不一致问题。
|
||
|
||
\#\# 🔍 核心变更点 (Key Changes)
|
||
1\. 引入了 \`Redisson\` 分布式锁控制并发请求。
|
||
2\. 重构了 \`RefundService\`，将第三方退款网关的调用抽离为了防腐层。
|
||
3\. 数据库 \`order\` 表新增了 \`refund\_status\` 字段（已包含 Migration 脚本）。
|
||
|
||
\#\# ⚠️ 影响面与风险评估 (Impact & Risks)
|
||
\* \*\*功能影响\*\*: 仅影响退款链路。
|
||
\* \*\*兼容性\*\*: \*\*\[注意\]\*\* 接口 \`POST /api/v1/refund\` 新增了必填参数 \`reason\_code\`，需前端同步发版，暂不兼容旧版 App。
|
||
\* \*\*性能风险\*\*: 引入了分布式锁，预期单个退款请求耗时增加 10ms，在接受范围内。
|
||
|
||
\#\# 🎯 自我检查 (Self Check)
|
||
\- \[x\] 本地单测全部通过。
|
||
\- \[x\] 没有遗留的 \`print\` 和注释掉的死代码。
|
||
\- \[x\] 已补充对应的 OpenAPI 文档注解。
|