代码审查的艺术

代码审查不是找 bug，是分享知识、建立团队共识的过程。

---

为什么代码审查重要

研究数据

指标	数据
发现的 bug	50%+ 在审查中被发现
知识传播	代码审查是主要学习方式
团队凝聚力	共同的编码标准

错误类型

# 早期发现
def calculate_total(prices):
    return sum(prices)  # ❌ 忘记处理 None

# PR 审查时发现
def calculate_total(prices):
    if not prices:
        return 0
    return sum(prices)  # ✅ 正确

---

审查前准备

自查清单

• 代码自测通过
• 格式化
• 自我注释清楚
• 遵循项目约定

工具辅助

# 格式检查
npm run format-check
cargo fmt --check

# 静态分析
npm run lint
cargo clippy

# 类型检查
npm run type-check
cargo check

---

审查时关注点

1. 正确性

// ❌ 未处理边界
function getFirstItem(arr) {
    return arr[0];
}

// ✅ 处理空数组
function getFirstItem(arr) {
    if (!arr || arr.length === 0) return null;
    return arr[0];
}

2. 可读性

# ❌ 难读
def f(x,y,z):
    return x if x>y else y if y>z else z

# ✅ 清晰
def median_of_three(a, b, c):
    if a > b:
        return a if a > c else c
    return b if b > c else c

3. 可维护性

// ❌ 魔法数字
if (status === 5) {
    // something
}

// ✅ 有意义的常量
const ORDER_STATUS_PROCESSING = 5;
if (status === ORDER_STATUS_PROCESSING) {
    // something
}

4. 性能

# ❌ O(n²)
def find_duplicates(arr):
    duplicates = []
    for i, item1 in enumerate(arr):
        for j, item2 in enumerate(arr):
            if i != j and item1 == item2:
                duplicates.append(item1)
    return duplicates

# ✅ O(n)
def find_duplicates(arr):
    seen = set()
    return [x for x in arr if x in seen or seen.add(x)]

5. 安全性

# ❌ SQL 注入风险
query = f"SELECT * FROM users WHERE name = '{name}'"

# ✅ 参数化查询
query = "SELECT * FROM users WHERE name = %s"
cursor.execute(query, (name,))

6. 测试

// ❌ 没有测试
export function calculateDiscount(price) {
    return price * 0.9;
}

// ✅ 有测试
export function calculateDiscount(price) {
    if (price <= 0) return 0;
    return price * 0.9;
}

// 测试
test('zero price returns zero', () => {
    expect(calculateDiscount(0)).toBe(0);
});

---

审查风格

Google 的 CL（Change Log）

## CL

第一行：一句话总结

详细描述：
- 背景
- 变更内容
- 测试方法

测试：
- 如何验证
- 边界情况

GitHub 风格

## Changes
- Add feature X
- Fix bug Y

## Related
- Closes #123
- Fixes issue/456

---

审查评论模板

正面反馈

LGTM! 👍

小的建议：
- 变量名 `foo` 可能改成 `userCount` 更清晰

建设改进

整体不错，有几个建议：

1. **性能**：`find_duplicates` 是 O(n²)，可以优化成 O(n)
2. **可读性**：`if (x > y ? x : y > z ? y : z)` 可以用 `Math.max`

需要修改

⚠️ 安全问题：

`query = f"SELECT * FROM users WHERE name = '{name}'"` 有 SQL 注入风险。

建议：
```python
query = "SELECT * FROM users WHERE name = %s"
cursor.execute(query, (name,))

请修复后我再审查。

---

## 大公司实践

### Google

- **CL 必须**：每次提交都要写 CL
- **审查者指定**：每个 PR 指定审查者
- **自动化工具**：Presubmit 检查
- **知识分享**：审查是学习过程

### Microsoft

- **代码审查清单**：结构化检查项
- **安全审查**：敏感代码需要额外审查
- **性能审查**：关键路径需要 perf 审查
- **自动化**：大量静态分析工具

### GitHub

- **保护分支**：限制谁能推送
- **必需审查**：设置审查人数
- **代码所有者**：文件/目录级别的审查要求
- **CI 集成**：自动检查在审查前运行

---

## 审查者角色

### 责任

1. **验证正确性**：代码是否工作
2. **检查风格**：是否符合团队约定
3. **学习**：了解别人的代码
4. **指导**：帮助作者改进

### 心态

> "审查者不是裁判，是合作伙伴。"

### 时间投入

| 复杂度 | 预期时间 |
|------|----------|
| 小 PR | 5-15 分钟 |
| 中等 PR | 15-60 分钟 |
| 大 PR | 1-3 小时 |

---

## 作者责任

### PR 大小

```bash
# 小 PR 更快审查
git log --oneline origin/main..HEAD | wc -l

# 理想：< 200 行
# 可接受：< 500 行
# 太大：> 1000 行，需要拆分

自审查

## Self-Review

- [ ] 代码自测
- [ ] 格式化
- [ ] 文档更新
- [ ] 测试覆盖
- [ ] 性能影响
- [ ] 安全检查

响应反馈

## Addressing Review

### Reviewer: @alice

> 建议：变量名改成 `userCount`

**Done**: ✅ 已修改

> 问题：SQL 注入风险

**Done**: ✅ 已改为参数化查询

> 建议：添加边界测试

**Done**: ✅ 已添加 `test_empty_array`

---

工具

静态分析

# Python
black .              # 格式化
flake8              # Lint
mypy .              # 类型检查
pytest               # 测试

# JavaScript
npm run format       # Prettier
npm run lint         # ESLint
npm run type-check   # TypeScript
npm test            # Jest

# Rust
cargo fmt           # 格式化
cargo clippy        # Lint
cargo check         # 类型检查
cargo test          # 测试

CI/CD 集成

# .github/workflows/review.yml
name: Review Checklist

on: [pull_request]

jobs:
  review:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
      - name: Format Check
        run: npm run format-check
      - name: Lint
        run: npm run lint
      - name: Type Check
        run: npm run type-check
      - name: Test
        run: npm test

---

常见陷阱

1. 太关注细节

# ❌ 扣细节
"""变量名 foo 可以改成 userCount 更清晰"""

# ✅ 关注本质
"""`find_duplicates` 是 O(n²)，考虑用 set 优化"""

2. 打击作者

# ❌
这代码写得不好，重写。

# ✅
有个更好的实现方式，用 set 会是 O(n) 而不是 O(n²)。

3. 忽略积极反馈

# 作者的努力也要认可
"""
性能优化做得好！👍

小建议：`find_duplicates` 可以进一步优化成用 `collections.Counter`。
"""

4. 阻塞等待

# ❌ 阻塞其他工作
"""@alice @bob 请审查，等你们批准我再继续"""

# ✅ 并行进行
"""开始做 X 部分，等 Y 部分审查完合并"""

---

指标与改进

衡量质量

指标	目标
审查覆盖率	> 80% 代码
审查时间	< 2 小时/PR
轮数	< 3 轮合并
审查后 bug 率	< 5%

团队改进

# 定期回顾审查流程
def review_team_practices():
    metrics = collect_review_metrics()

    # 识别问题
    if metrics.avg_review_time > 2 * 60 * 60:
        suggest("减少 PR 大小")

    if metrics.reviews_needed > 3:
        suggest("提高自审查质量")

    if metrics.blocked_prs > 0:
        suggest("改善流程，减少等待")

---

总结

代码审查的艺术在于：

1. 准备充分 - 自查、测试、格式化
2. 关注本质 - 正确性、可读性、性能
3. 建设性反馈 - 具体建议，不人身攻击
4. 协作心态 - 审查者不是裁判，是合作伙伴
5. 工具辅助 - 自动化检查，减少人工负担

“代码审查是团队成长最重要的工具之一。“

---

资源

• Google Review Guide
• Microsoft Code Review Guide
• GitHub Best Practices
• Effective Code Review