代码审查卓越实践

通过建设性反馈、系统性分析与协作式改进，将代码审查从“守门”转变为知识共享。

何时使用本技能

• 评审 Pull Request 与代码变更
• 为团队建立代码审查标准
• 通过审查指导初级开发者
• 开展架构评审
• 制定审查检查清单与指南
• 提升团队协作效率
• 缩短代码审查周期
• 维持代码质量标准

核心原则

1. 审查思维模式

代码审查的目标：

• 发现缺陷与边界情况
• 保障代码可维护性
• 促进团队间知识共享
• 落实编码规范
• 优化设计与架构
• 塑造团队文化

非审查目标：

• 炫耀个人知识
• 过度挑剔格式（应交由 linter 处理）
• 不必要地阻碍进度
• 按个人偏好重写代码

2. 高效反馈

优质反馈的特征：

• 具体且可操作
• 具有教育意义，而非评判性
• 聚焦于代码本身，而非开发者个人
• 保持平衡（同时肯定优秀实践）
• 区分优先级（关键问题 vs. 可选建议）

❌ 不佳："这不对。"
✅ 优良："当多个用户同时访问时，此处可能引发竞态条件。建议在此处使用 mutex。"

❌ 不佳："你为何未采用 X 模式？"
✅ 优良："你是否考虑过 Repository 模式？它将使该逻辑更易于测试。示例参考：[link]"

❌ 不佳："请重命名此变量。"
✅ 优良："[nit] 建议将 `uc` 改为 `userCount` 以提升可读性。若你倾向保留原名，亦可接受，此非阻塞性建议。"

3. 审查范围

需人工审查的内容：

• 逻辑正确性与边界情况
• 安全漏洞
• 性能影响
• 测试覆盖率与质量
• 错误处理机制
• 文档与注释
• API 设计与命名规范
• 架构适配性

无需人工审查的内容：

• 代码格式（应使用 Prettier、Black 等工具）
• 导入语句组织
• lint 错误
• 简单拼写错误

审查流程

阶段一：上下文收集（2–3 分钟）

在深入代码前，请先理解：

1. 阅读 PR 描述及关联 issue
2. 检查 PR 规模（超过 400 行？建议拆分）
3. 查看 CI/CD 状态（测试是否通过？）
4. 明确业务需求
5. 记录相关架构决策

阶段二：高层级审查（5–10 分钟）

1. **架构与设计**
   - 解决方案是否匹配问题本质？
   - 是否存在更简洁的实现方式？
   - 是否与现有模式保持一致？
   - 是否具备可扩展性？

2. **文件组织**
   - 新增文件是否置于合理路径？
   - 代码是否按逻辑合理分组？
   - 是否存在重复文件？

3. **测试策略**
   - 是否包含测试？
   - 测试是否覆盖边界场景？
   - 测试是否易读？

阶段三：逐行审查（10–20 分钟）

针对每个文件：

1. **逻辑与正确性**
   - 边界情况是否已处理？
   - 是否存在索引越界错误？
   - 是否校验了 null/undefined？
   - 是否存在竞态条件？

2. **安全性**
   - 输入是否经验证与净化？
   - 是否存在 SQL 注入风险？
   - 是否存在 XSS 漏洞？
   - 敏感数据是否被意外暴露？

3. **性能**
   - 是否存在 N+1 查询？
   - 是否存在冗余循环？
   - 是否存在内存泄漏？
   - 是否存在阻塞型操作？

4. **可维护性**
   - 变量命名是否清晰？
   - 函数是否只做一件事？
   - 复杂逻辑是否附有注释？
   - “魔法数字”是否已提取为常量？

阶段四：总结与决策（2–3 分钟）

1. 归纳关键关注点
2. 指出值得肯定之处
3. 明确做出决定：
   - ✅ 批准
   - 💬 评论（提出次要建议）
   - 🔄 请求修改（必须解决）
4. 如涉及复杂问题，主动提出结对协作

审查技巧

技巧一：检查清单法

## 安全性检查清单

- [ ] 用户输入已验证并净化
- [ ] SQL 查询使用参数化
- [ ] 已执行身份认证/授权校验
- [ ] 密钥/敏感信息未硬编码
- [ ] 错误消息未泄露敏感信息

## 性能检查清单

- [ ] 无 N+1 查询
- [ ] 数据库查询已建立索引
- [ ] 大型列表已分页
- [ ] 高开销操作已缓存
- [ ] 热路径中无阻塞型 I/O

## 测试检查清单

- [ ] 已覆盖主流程（happy path）
- [ ] 已覆盖边界情况
- [ ] 已覆盖错误场景
- [ ] 测试名称具有描述性
- [ ] 测试具备确定性（deterministic）

技巧二：提问式方法

避免直接指出问题，转而通过提问引导思考：

❌ "若列表为空，此处将失败。"
✅ "若 `items` 是空数组，会发生什么？"

❌ "此处需添加错误处理。"
✅ "若 API 调用失败，预期行为应如何？"

❌ "此实现效率低下。"
✅ "我注意到此处遍历了全部用户。当用户量达 10 万时，我们是否评估过其性能影响？"

技巧三：建议而非指令

## 使用协作性语言

❌ "你必须改用 async/await。"
✅ "建议：async/await 可能提升可读性：
`typescript
    async function fetchUser(id: string) {
        const user = await db.query('SELECT * FROM users WHERE id = ?', id);
        return user;
    }
    `
你怎么看？"

❌ "请将此逻辑提取为函数。"
✅ "该逻辑在 3 处出现。是否考虑将其提取为共用工具函数？"

技巧四：区分问题严重等级

使用标签标明优先级：

🔴 [blocking] — 合并前必须修复  
🟡 [important] — 应修复；若存异议，可讨论  
🟢 [nit] — 改进项，非阻塞性  
💡 [suggestion] — 值得考虑的替代方案  
📚 [learning] — 教育性说明，无需行动  
🎉 [praise] — 表扬，继续保持！

示例：
"🔴 [blocking] 此 SQL 查询存在注入风险，请改用参数化查询。"

"🟢 [nit] 建议将 `data` 重命名为 `userData` 以增强可读性。"

"🎉 [praise] 测试覆盖率极佳！能有效捕获边界情况。"

语言特异性审查模式

Python 代码审查

# 检查 Python 特有陷阱

# ❌ 可变默认参数
def add_item(item, items=[]):  # 错误！跨多次调用共享同一对象
    items.append(item)
    return items

# ✅ 使用 None 作为默认值
def add_item(item, items=None):
    if items is None:
        items = []
    items.append(item)
    return items

# ❌ 过度宽泛的异常捕获
try:
    result = risky_operation()
except:  # 捕获所有异常，甚至包括 KeyboardInterrupt！
    pass

# ✅ 捕获特定异常
try:
    result = risky_operation()
except ValueError as e:
    logger.error(f"无效值：{e}")
    raise

# ❌ 使用可变类属性
class User:
    permissions = []  # 所有实例共享！

# ✅ 在 __init__ 中初始化
class User:
    def __init__(self):
        self.permissions = []

TypeScript / JavaScript 代码审查

// 检查 TypeScript 特有陷阱

// ❌ 使用 any 将破坏类型安全
function processData(data: any) {  // 避免使用 any
    return data.value;
}

// ✅ 使用精确类型定义
interface DataPayload {
    value: string;
}
function processData(data: DataPayload) {
    return data.value;
}

// ❌ 未处理异步错误
async function fetchUser(id: string) {
    const response = await fetch(`/api/users/${id}`);
    return response.json();  // 若网络失败，如何处理？
}

// ✅ 正确处理错误
async function fetchUser(id: string): Promise {
    try {
        const response = await fetch(`/api/users/${id}`);
        if (!response.ok) {
            throw new Error(`HTTP ${response.status}`);
        }
        return await response.json();
    } catch (error) {
        console.error('获取用户失败：', error);
        throw error;
    }
}

// ❌ 修改 props
function UserProfile({ user }: Props) {
    user.lastViewed = new Date();  // 修改传入的 props！
    return 
{user.name}
;
}

// ✅ 不修改 props
function UserProfile({ user, onView }: Props) {
    useEffect(() => {
        onView(user.id);  // 通知父组件更新状态
    }, [user.id]);
    return 
{user.name}
;
}

高级审查模式

模式一：架构评审

评审重大变更时：

1. **先审设计文档**
   - 对大型功能，要求先提交设计文档再编写代码
   - 实现前组织团队评审设计方案
   - 共同确认技术路线，避免返工

2. **分阶段评审**
   - 首个 PR：核心抽象与接口定义
   - 第二个 PR：具体实现
   - 第三个 PR：集成与测试
   - 更易评审，迭代更快

3. **评估替代方案**
   - "我们是否考虑过使用 [模式/库]？"
   - "相比更简单的方案，权衡点是什么？"
   - "当需求变化时，此设计如何演进？"

模式二：测试质量评审

// ❌ 劣质测试：测试实现细节
test('递增计数器变量', () => {
    const component = render();
    const button = component.getByRole('button');
    fireEvent.click(button);
    expect(component.state.counter).toBe(1);  // 测试内部状态
});

// ✅ 优质测试：测试行为表现
test('点击后显示递增后的计数值', () => {
    render();
    const button = screen.getByRole('button', { name: /increment/i });
    fireEvent.click(button);
    expect(screen.getByText('Count: 1')).toBeInTheDocument();
});

// 测试评审要点：
// - 测试是否描述行为，而非实现细节？
// - 测试名称是否清晰、具描述性？
// - 是否覆盖边界情况？
// - 测试是否相互独立（无共享状态）？
// - 测试是否可任意顺序运行？

模式三：安全审查

## 安全审查检查清单

### 身份认证与授权

- [ ] 关键位置是否强制身份认证？
- [ ] 每次操作前是否执行授权校验？
- [ ] JWT 验证是否完整（签名、过期时间等）？
- [ ] API 密钥/密钥是否妥善保护？

### 输入验证

- [ ] 所有用户输入是否均已验证？
- [ ] 文件上传是否限制大小与类型？
- [ ] SQL 查询是否参数化？
- [ ] 输出是否已转义以防范 XSS？

### 数据保护

- [ ] 密码是否经哈希（如 bcrypt/argon2）？
- [ ] 敏感数据是否静态加密？
- [ ] 敏感数据传输是否强制 HTTPS？
- [ ] 个人身份信息（PII）是否符合法规要求？

### 常见漏洞

- [ ] 是否禁用 eval() 或类似动态执行？
- [ ] 是否杜绝硬编码密钥/敏感信息？
- [ ] 状态变更操作是否具备 CSRF 防护？
- [ ] 公开端点是否配置请求频率限制？

提供困难反馈

模式：改良版“三明治法”

传统方式：表扬 + 批评 + 表扬（易显生硬）

更优方式：背景说明 + 具体问题 + 建设性方案

示例：
"我注意到支付处理逻辑内联在 controller 中，这会降低其可测试性与复用性。

[具体问题]
calculateTotal() 函数混合了税费计算、折扣逻辑与数据库查询，导致难以单元测试和理解。

[建设性方案]
能否将这部分逻辑提取为 PaymentService 类？这样既便于测试，也利于复用。如有需要，我很乐意与你结对完成。"

处理意见分歧

当作者不同意你的反馈时：

1. **先寻求理解**
   "请帮我理解你的思路——是什么促使你选择这一模式？"

2. **认可合理观点**
   "关于 X 的观点很有道理，此前我并未考虑到这一点。"

3. **提供依据**
   "我主要担心性能问题。能否补充基准测试来验证当前方案？"

4. **必要时升级讨论**
   "让我们邀请 [架构师/资深开发者] 一起评估这个决策。"

5. **适时放手**
   若方案可行且非关键问题，可直接批准。完美主义是进步的敌人。

最佳实践

1. 及时评审：理想情况下当日完成，最迟不超过 24 小时
2. 控制 PR 规模：高效评审建议单个 PR 不超过 200–400 行
3. 分时段评审：单次专注时长不超过 60 分钟，并适时休息
4. 善用评审工具：GitHub、GitLab 或专用评审平台
5. 尽可能自动化：linter、格式化工具、安全扫描等
6. 建立信任关系：适当使用 emoji、真诚表扬与共情表达
7. 保持可支持性：对复杂问题主动提出结对协助
8. 向他人学习：研读其他人的评审评论

常见误区

• 完美主义：因细微风格偏好阻塞 PR
• 范围蔓延：“顺手也……”类额外需求
• 标准不一：对不同人执行不同审查标准
• 延迟评审：让 PR 长期滞留无人响应
• 失联式评审：提出修改要求后即消失
• 形式主义审批：未经实质审查即批准
• 自行车棚效应：就无关紧要的细节展开冗长争论

模板

PR 评审评论模板

## 总结

[简要概述本次评审内容]

## 优点

- [做得好的方面]
- [值得借鉴的设计或实践]

## 必须修改项

🔴 [阻塞性问题 1]  
🔴 [阻塞性问题 2]

## 建议项

💡 [改进建议 1]  
💡 [改进建议 2]

## 问题与澄清

❓ [关于 X 的疑问]  
❓ [是否考虑过其他方案？]

## 结论

✅ 在解决上述必须修改项后批准

参考资料

• references/code-review-best-practices.md：全面的代码审查指南
• references/common-bugs-checklist.md：各语言常见缺陷检查清单
• references/security-review-guide.md：面向安全的审查检查清单
• assets/pr-review-template.md：标准化评审评论模板
• assets/review-checklist.md：快速查阅检查清单
• scripts/pr-analyzer.py：分析 PR 复杂度并推荐评审人