CODE REVIEW · 评审实践 / 团队质量
Code Review速查
评审节奏与响应时限、八大 Review 维度、blocker/suggest/question/nit 话术分级、作者提审自查清单、机器人预检与门禁、评审文化与成长——代码评审最常追问的 43 个核心要点一张表收齐，随查随用。
43条速查
8大主题
∞持续更新
📖 速查表
点击展开各小节
🔁 CR 流程与节奏
| 名称 | 说明 | 要点 / 示例 |
|---|---|---|
| 何时发起 | 小步尽早提审：把功能拆成可独立评审的小块，别攒一周的变更一次丢出来 | 经验口径：单 PR 有效变更 ≤ 400 行；越早提审，返工成本越低 必背 |
| PR 合适大小 | 评审质量随变更行数上升而骤降，超大 PR 只能「看起来过了一遍」 | 大型重构先发设计稿 / RFC 评审，再按模块分批提 PR，每批可独立合并回滚 |
| Reviewer 响应时限 | 团队约定硬性 SLA：首个响应 ≤ 1 个工作日，小 PR 当天完成，阻塞型 MR 优先插队 | 响应 ≠ 审完：先给「初步意见 / 预计完成时间」，让作者可安排 团队约定 |
| 阻塞 vs 非阻塞 | 明确标注哪些意见必须修改才能合并（blocker），哪些仅供参考（non-blocking / suggestion） | 作者有权合并非阻塞意见；Reviewer 不应拿个人偏好无限挂起 MR 易起冲突 |
| Escalation 路径 | 意见分歧先在评论/会议讨论；无法达成一致升级 Tech Lead 或架构评审裁决 | 别在评论区冷战或无限循环；裁决后记录结论供后续复用 |
| Review 分工 | 指派 + 认领结合：CODEOWNERS 定必审人，团队轮值认领兜底 | 至少 1~2 个 reviewer；核心模块追加资深成员评审，避免知识单点 |
🔍 Review 维度清单
八个维度按优先级过：先正确性与安全，再看测试与性能，风格问题交给格式化工具。
| 维度 | 检查要点 | 示例 / 提示 |
|---|---|---|
| 正确性 | 逻辑是否符合需求与描述？算法边界（空输入/重复/溢出）是否处理？与既有行为是否兼容？ | 第一优先级：宁多花时间核对业务语义，别只看代码风格 最高优先级 |
| 可读性 | 函数是否过长（一屏放得下）、嵌套是否过深、注释是否解释「为什么」而非复述代码 | 早返回替代 if 嵌套；复杂条件提取成具名布尔方法 |
| 边界与异常 | 空值/越界/超时/重试是否处理？异常是否被吞掉（catch 后仅打印）？资源是否释放（连接/流/锁） | finally 或 try-with-resources 释放资源；吞异常是最常见的隐性 bug 源 |
| 并发安全 | 共享可变状态有无保护？锁粒度与加锁顺序（防死锁）？线程池参数、ThreadLocal 泄漏、异步上下文丢失 | 能无共享就不共享：优先不可变对象与局部状态 面试高频 |
| 测试覆盖 | 新逻辑有无单测？边界与异常分支是否覆盖？受影响的老用例是否需要补？测试是否真的断言 | 「跑通即过、无断言」的测试等于没写 |
| 性能 | 循环内 DB/RPC 调用（N+1）、大集合全量加载进内存、重复计算 | 多问一句：数据量放大 100 倍后这段逻辑还成立吗 |
| 安全 | 入参校验与注入（SQL/命令/XSS）、越权访问（水平/垂直）、敏感信息打日志、依赖漏洞 | 密钥、手机号、身份证号出现在日志/响应体里要当场拦下 安全红线 |
| 命名 | 名字是否表达意图（userList vs data）、布尔用 is/has、方法用动词；同一概念全库统一 | 别让「订单」一会叫 order 一会叫 purchase；命名分歧就是通用语言问题 |
💬 评论话术分级
每条评论带上分级前缀，作者才能一眼分清「必须改」和「仅供参考」。
| 级别 | 含义与用法 | 话术示例 |
|---|---|---|
| [blocker] 必须改 | 正确性缺陷、安全漏洞、数据破坏风险，必须解决才能合并 | 「这里没有校验越权，任意 user_id 可查他人订单，需修复后合并」 |
| [suggest] 建议 | 更好的写法但不阻塞合并，作者自行权衡 | 「可以用 X 替代，可读性更好，不阻塞，你判断」 作者可取舍 |
| [question] 提问 | 不懂先问：先弄清意图再下结论，避免基于错误假设的意见 | 「这里为什么手动重试三次？是不是为了兼容 A 场景？」 |
| [nit] 吹毛求疵 | 风格偏好、可选优化，明确标注「可忽略」 | 「nit：变量名 misc 太泛，可改 list；可忽略」 别写成阻塞 |
| 对事不对人句式 | 评论指向代码与行为，不评价作者；用第一人称陈述观察 | 「这段代码很烂」→「这个方法 200 行，拆成 X/Y 更好评审」；「你写的看不懂」→「我没看懂这里的意图」 必背 |
| 疑虑先问原因 | 看似错误的代码可能有隐含约束（历史 bug 兜底、框架限制、线上兼容） | 先问「这里是为了处理什么情况？」再提修改；避免消耗信任的无效意见 |
✅ 作者自查清单
| 自查项 | 说明 | 要点 / 示例 |
|---|---|---|
| 自测通过 | 本地编译、单测、冒烟通过，关键路径手工验证过 | CI 绿了再提审，别让 Reviewer 当「编译器」 提审前提 |
| 自 Review 一遍 | 提审前通读自己的 diff：删调试代码与注释掉的死代码、拆过大文件、清理多余 TODO | 大量低级问题在这一步被自己抓出来，尊重评审者时间 |
| 描述背景与影响面 | MR 描述写清：解决什么问题（背景）、怎么解决（方案一句话）、影响面（接口/表结构/配置变更）、如何验证 | 评审者需要上下文才能评审正确性；「修复 bug」三个字的描述直接打回 |
| 关联单号与文档 | 关联 Jira/Issue 单号、设计文档链接；含数据变更时附迁移脚本与回滚说明 | 回滚方案提前想好：出问题时按单号能立刻定位当时变更 |
| 提交原子性 | 一个 PR 只做一件事：不混格式化与逻辑变更、不夹带无关重构 | 历史脏文件与依赖升级单独提 chore PR，方便回溯与 revert |
🛠️ 工具与实践
| 名称 | 说明 | 要点 / 示例 |
|---|---|---|
| GitHub PR | 逐行评论锚定、Review 总结（Approve / Request Changes / Comment）、suggestion 块一键采纳 | 配合 Actions 跑 CI 门禁；suggestion 采纳后自动生成 commit，改起来零摩擦 |
| Gerrit | Change-Id 驱动的逐提交评审、+1/+2 分级投票、按 _topic 归组管理 | 适合强管控流程的大型团队；与 GitHub PR 是两种主流评审模型 一句话了解 |
| PR 模板要素 | 背景 / 方案 / 自测情况 / 影响面 / 回滚方案 checklist | 机器人检测未填模板自动打回，让「描述质量」标准化 |
| 机器人预检 | Lint（格式/静态检查）、CI 门禁（编译 + 单测 + 覆盖率阈值）、安全扫描（依赖漏洞/密钥泄漏）先过机器 | 人类只审机器审不了的：业务语义、边界、设计取舍 效率关键 |
| CODEOWNERS 与门禁 | 目录级自动指派必审人；分支保护规则禁止直推主干、强制 status check 与最少 approve 数 | 核心模块要求资深评审 +2 才可合，用流程兜底而非自觉 |
🌱 CR 文化与成长
| 名称 | 说明 | 要点 / 示例 |
|---|---|---|
| 新人学什么 | Review 他人代码是最快的隐性知识吸收渠道：看资深 MR 的拆分粒度、测试写法、命名与注释习惯 | 主动申请加入核心模块 reviewer 名单，比读文档学得快 成长捷径 |
| CR 时间预算 | 团队约定每日固定评审时段（如上午集中 30~60 分钟），避免评审无限侵占整块开发时间 | 超出预算的复杂 MR 另约时间一起评审，别让大 PR 陪跑一周 |
| 跨团队 CR 价值 | 邀请相邻团队评审接口契约，提前暴露跨团队耦合与口径不一致 | 跨团队 Review 也是技术方案对齐的低成本渠道，比开会更轻 |
| 知识沉淀 | 高频意见沉淀为团队 CheckList 与静态检查规则，同类问题下次由机器人拦截 | 人肉重复纠正三次的问题就该进 Lint 规则 长期主义 |
| 反馈与认可 | Reviewer 指出好问题要公开点赞，作者感谢有效意见；把评审质量纳入技术贡献 | 让「发现别人问题」被看见，评审才不是义务劳动 |
| 大型重构评审 | 超大重构不走常规 MR 流程：先 RFC/设计评审 → 原型 PoC 验证 → 分阶段小 PR 落地 | 每阶段独立可回滚，设计问题在白板上解决而不是在评论区解决 |
🧾 PR 描述模板
描述质量决定评审质量：背景、改动、影响面、自测、回滚五要素齐全，Reviewer 才有上下文判断正确性
## 背景
修复线上退款金额偶发翻倍问题，单号：BUG-2024-0187
根因：重试逻辑与幂等校验之间存在并发窗口
## 改动点
- refund 表新增退款单号唯一约束，重复请求直接返回原结果
- RefundService#refund 增加 Redis 分布式锁兜底（10s 自动过期）
## 影响面
- 接口：POST /api/refund 行为变更（重复请求返回 200 + 原退款单号）
- 数据：新增唯一索引，需先清理历史重复数据（脚本见附链）
- 配置：新增开关 refund.idempotent.enabled，默认关闭、灰度开启
## 自测清单
- [x] 单测 14 个通过，覆盖并发重复请求分支
- [x] 预发冒烟：同单号连发 5 次仅退款 1 次
- [x] 历史重复数据清理脚本在测试库演练通过
## 回滚方式
配置开关 refund.idempotent.enabled 置 false 即回旧逻辑；
极端情况 revert 本 PR，唯一索引需同步 DROP（附回滚脚本）
🚦 常见争议裁决
评审桌上最常见的六场拉锯，与其靠嗓门与职级，不如靠预先约定的裁决规则
| 争议 | 裁决 | 依据 |
|---|---|---|
| 风格之争 | 空格、换行、大括号位置一律不评论，交给格式化工具 | EditorConfig + Lint 统一兜底，人只审机器审不了的 零争议 |
| 性能洁癖 | 「我觉得慢」不构成修改意见，先出 Profile / 压测数据 | 凭直觉猜的瓶颈大多不是真瓶颈；有数据再谈优化 先测量 |
| DRY vs 可读 | 三次法则：第一二次容忍重复，第三次出现再抽公共 | 过早抽象的公共函数往往抽错了方向，比重复更难改 |
| 测试要不要 100% | 不追求全库 100%，按风险分级：支付 / 权限 / 资金路径必须全覆盖 | 给 getter 和展示层凑覆盖率是无效劳动 按风险分级 |
| 大 PR 怎么拆 | 先 RFC 对齐方案，再按「模块 / 分层」拆成可独立合并的批次 | 每批 ≤ 400 行且独立可回滚；接口先行、实现跟进 |
| 命名各执一词 | 先查团队词库 / 领域术语表；仍无共识就去问业务方怎么叫 | 命名分歧多是通用语言问题，业务术语说了算，不比谁嗓门大 |