卓越代码审查

通过建设性反馈、系统性分析和协作改进，将代码审查从把关转变为知识共享。

何时使用此技能

• 审查拉取请求和代码更改
• 为团队建立代码审查标准
• 通过审查指导初级开发人员
• 进行架构审查
• 创建审查清单和指南
• 改进团队协作
• 缩短代码审查周期
• 维护代码质量标准

核心原则

1. 审查心态

代码审查的目标：

• 发现错误和边界情况
• 确保代码可维护性
• 在团队中分享知识
• 执行编码标准
• 改进设计和架构
• 建立团队文化

非目标：

• 炫耀知识
• 挑剔格式（使用代码格式化工具）
• 不必要地阻碍进度
• 按个人偏好重写

2. 有效反馈

好的反馈是：

• 具体且可操作的
• 具有教育意义，而非评判性
• 关注代码本身，而非个人
• 平衡的（也要表扬好的工作）
• 有优先级的（关键问题 vs 锦上添花）

❌ 糟糕的反馈："这是错的。"
✅ 好的反馈："当多个用户同时访问时，这可能会导致竞态条件。
考虑在这里使用互斥锁。"

❌ 糟糕的反馈："你为什么不用 X 模式？"
✅ 好的反馈："你考虑过仓储模式吗？它会让测试变得更容易。
这里有一个例子：[链接]"

❌ 糟糕的反馈："重命名这个变量。"
✅ 好的反馈："[nit] 为了清晰起见，考虑使用 `userCount` 而不是 `uc`。
如果你倾向于保留，也不影响通过。"

3. 审查范围

要审查什么：

• 逻辑正确性和边界情况
• 安全漏洞
• 性能影响
• 测试覆盖率和质量
• 错误处理
• 文档和注释
• 应用程序编程接口设计和命名
• 架构契合度

不要手动审查什么：

• 代码格式（使用 Prettier、Black 等）
• 导入组织
• 代码检查工具违规
• 简单的拼写错误

审查流程

阶段 1：收集上下文（2-3 分钟）

在深入代码之前，先了解：

1. 阅读拉取请求描述和关联的 Issue
2. 检查拉取请求大小（超过 400 行？建议拆分）
3. 查看 CI/CD 状态（测试是否通过？）
4. 理解业务需求
5. 注意任何相关的架构决策

阶段 2：高层审查（5-10 分钟）

1. **架构与设计**
   - 解决方案是否符合问题？
   - 是否有更简单的方法？
   - 是否与现有模式一致？
   - 它能否扩展？

2. **文件组织**
   - 新文件是否放在正确的位置？
   - 代码是否按逻辑分组？
   - 是否有重复文件？

3. **测试策略**
   - 有测试吗？
   - 测试是否覆盖了边界情况？
   - 测试是否可读？

阶段 3：逐行审查（10-20 分钟）

对于每个文件：

1. **逻辑与正确性**
   - 是否处理了边界情况？
   - 是否存在差一错误？
   - 是否检查了空值/未定义？
   - 是否存在竞态条件？

2. **安全**
   - 是否验证了输入？
   - 是否存在 SQL 注入风险？
   - 是否存在跨站脚本攻击漏洞？
   - 是否暴露了敏感数据？

3. **性能**
   - 是否存在 N+1 查询？
   - 是否存在不必要的循环？
   - 是否存在内存泄漏？
   - 是否存在阻塞操作？

4. **可维护性**
   - 变量名是否清晰？
   - 函数是否只做一件事？
   - 复杂代码是否有注释？
   - 是否提取了魔法数字？

阶段 4：总结与决策（2-3 分钟）

1. 总结关键问题
2. 突出你喜欢的部分
3. 做出明确决定：
   - ✅ 批准
   - 💬 评论（次要建议）
   - 🔄 请求更改（必须处理）
4. 如果复杂，提供结对编程选项

审查技巧

技巧 1：清单法

## 安全检查清单

- [ ] 用户输入已验证和清理
- [ ] SQL 查询使用参数化
- [ ] 已检查身份验证/授权
- [ ] 密钥未硬编码
- [ ] 错误信息不泄露信息

## 性能检查清单

- [ ] 没有 N+1 查询
- [ ] 数据库查询已建索引
- [ ] 大列表已分页
- [ ] 昂贵操作已缓存
- [ ] 热路径中没有阻塞 I/O

## 测试检查清单

- [ ] 测试了快乐路径
- [ ] 覆盖了边界情况
- [ ] 测试了错误情况
- [ ] 测试名称具有描述性
- [ ] 测试是确定性的

技巧 2：提问式方法

与其指出问题，不如提问以启发思考：

❌ "如果列表为空，这会失败。"
✅ "如果 `items` 是一个空数组，会发生什么？"

❌ "这里需要错误处理。"
✅ "如果应用程序编程接口调用失败，这个行为应该如何处理？"

❌ "这效率低下。"
✅ "我看到这会遍历所有用户。我们考虑过当有 10 万用户时的性能影响吗？"

技巧 3：建议而非命令

## 使用协作性语言

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

❌ "把这个提取成一个函数"
✅ "这个逻辑在三个地方出现了。把它提取成一个共享的工具函数是否有意义？"

技巧 4：区分严重性

使用标签指示优先级：

🔴 [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<User> {
    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;
    }
}

// ❌ 修改属性
function UserProfile({ user }: Props) {
    user.lastViewed = new Date();  // 修改属性！
    return <div>{user.name}</div>;
}

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

高级审查模式

模式 1：架构审查

当审查重大变更时：

1. **先审设计文档**
   - 对于大型功能，在代码之前请求设计文档
   - 在实现前与团队一起审查设计
   - 就方法达成一致，避免返工

2. **分阶段审查**
   - 第一个拉取请求：核心抽象和接口
   - 第二个拉取请求：实现
   - 第三个拉取请求：集成和测试
   - 更容易审查，迭代更快

3. **考虑替代方案**
   - "我们考虑过使用[模式/库]吗？"
   - "与更简单的方法相比，权衡是什么？"
   - "随着需求变化，这将如何演变？"

模式 2：测试质量审查

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

// ✅ 好的测试：测试行为
test('点击时显示增加的计数', () => {
    render(<Counter />);
    const button = screen.getByRole('button', { name: /增加/i });
    fireEvent.click(button);
    expect(screen.getByText('计数： 1')).toBeInTheDocument();
});

// 测试的审查问题：
// - 测试描述的是行为，而非实现？
// - 测试名称是否清晰且具有描述性？
// - 测试是否覆盖了边界情况？
// - 测试是否独立（无共享状态）？
// - 测试可以以任何顺序运行吗？

模式 3：安全审查

## 安全检查清单

### 身份验证与授权

- [ ] 在需要的地方是否进行了身份验证？
- [ ] 每次操作前是否进行了授权检查？
- [ ] JWT 验证是否正确（签名、过期）？
- [ ] 应用程序编程接口密钥/密钥是否妥善保管？

### 输入验证

- [ ] 所有用户输入都经过验证？
- [ ] 文件上传是否受限（大小、类型）？
- [ ] SQL 查询是否参数化？
- [ ] 是否防范跨站脚本攻击（转义输出）？

### 数据保护

- [ ] 密码是否已哈希（bcrypt/argon2）？
- [ ] 敏感数据是否在静态存储时加密？
- [ ] 敏感数据传输是否强制使用 HTTPS？
- [ ] 个人身份信息处理是否符合法规？

### 常见漏洞

- [ ] 没有使用 eval() 或类似的动态执行？
- [ ] 没有硬编码的密钥？
- [ ] 对状态更改的操作是否做了跨站请求伪造防护？
- [ ] 公共端点是否做了速率限制？

给予棘手的反馈

模式：三明治方法（改良版）

传统方式：表扬 + 批评 + 表扬（感觉虚假）

更好方式：背景 + 具体问题 + 有帮助的解决方案

示例：
"我注意到支付处理逻辑内联在控制器中。
这使得它更难测试和复用。

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

[有帮助的解决方案]
我们可以把它提取成一个 PaymentService 类吗？
这会让它变得可测试和可复用。如果需要，
我可以和你结对编程来解决这个问题。"

处理分歧

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

1. **寻求理解**
   "帮我理解你的方法。是什么让你选择这个模式？"

2. **承认有效观点**
   "关于 X 这一点说得很好。我之前没考虑到。"

3. **提供数据**
   "我担心性能。我们能否添加一个基准测试来验证这个方法？"

4. **必要时升级**
   "让我们请[架构师/高级开发人员]来权衡一下这个。"

5. **知道何时放手**
   如果它能工作且不是关键问题，就批准它。
   完美是进步的敌人。

最佳实践

1. 及时审查：24 小时内，最好当天完成
2. 限制拉取请求大小：有效审查最好在 200-400 行以内
3. 分时段审查：最多 60 分钟，中途休息
4. 使用审查工具：GitHub、GitLab 或专用工具
5. 自动化能自动化的：代码检查工具、格式化工具、安全扫描
6. 建立融洽关系：表情符号、表扬和同理心很重要
7. 保持可用性：主动提出结对解决复杂问题
8. 向他人学习：查看他人的审查评论

常见陷阱

• 完美主义：因次要的风格偏好而阻塞拉取请求
• 范围蔓延：”既然你在做，能不能顺便也……”
• 不一致性：对不同的人采用不同的标准
• 延迟审查：让拉取请求搁置数天
• 幽灵审查：请求更改后消失不见
• 橡皮图章：未真正审查就批准
• 琐事争论：在微不足道的细节上争论不休

模板

拉取请求审查评论模板

## 摘要

[简要概述审查内容]

## 优点

- [做得好的地方]
- [好的模式或方法]

## 必需更改

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

## 建议

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

## 问题

❓ [关于 X 需要澄清]
❓ [考虑替代方法]

## 裁决

✅ 处理必需更改后批准