代码评审:一台双向学习机器
这一章讲三件事: 评审到底在买什么(答案不只是「抓 bug」); 作为被评审者,怎么让你的代码容易被评、被评后怎么接; 作为评审者,怎么评得又快又准又不伤人。 读完你会拿到一台「双向学习机器」的两侧操作手册。
1. 这一章讲什么
你的代码不会直接进主干(主分支)——中间隔着代码评审:团队里的其他人在你的改动合并之前阅读它、提意见、最后批准。多数团队都有这道流程,但流程的质量天差地别:原书开篇的对仗是,高质量的评审文化「有助于所有具有不同经验水平的工程师的成长」,糟糕的评审文化「抑制创新,减慢开发速度,并且导致滋生怨恨情绪」1。
本章在全书链条里的位置:第 05 章的测试是你自己给自己立的栅栏;评审是团队替你立的第二道闸。它同时是第 01 章那两个心理循环(冒充者/邓宁-克鲁格)的高发现场——原书明确说,评审会同时触发焦虑与过度自信2。
2. 顶层全景
一条改动的生命周期
准备 ──→ 提交(创建评审请求) ──→ 评审(讨论/修改) ──→ 批准 ──→ 合并
│ │ │
│ 被评审者侧 评审者侧
│ ├ 小改动+描述+测试 ├ 分流(紧急?复杂?)
│ ├ 草案可提前 ├ 先懂意图再评论
│ ├ 别用评审触发 CI ├ 区分问题/建议/挑剔(Nit)
│ └ 别太在意 ├ 不当橡皮图章,也不当路障
└──共同前提:高度信任的环境 ←┘
图说:左右两侧各有一套手艺,共享同一个前提。主走查在 3.3(UI-1343)。
3. 核心原理
3.1 评审买的是什么:五重价值,一个前提
抓 bug 是肉眼可见的好处,但原书把它排在更长的清单里3:评审是教学工具(评审者会指出你不知道的类库和实践,你也能从资深队友的评审请求里学写生产级代码);是共同理解的保障(确保不止一个人熟悉每一行生产代码——你休假时没人抓瞎,值班工程师能查到「这块代码什么时候、为什么改的」);是实现决策的档案(为什么这么写的理由,白纸黑字留在评审意见里);有时还是安全与合规的硬要求(防止任何一个人恶意修改代码库)4。
但这五重价值有一个共同的开关:只有当所有参与者处在「高度信任」的环境里——评审者有意提供有用的反馈,被评审者也愿意接受——好处才成立5。反过来,执行不力的评审是一种腐蚀剂:轻率的反馈拖慢所 有人,缓慢的周转让改动停滞,没完没了的拉锯扯皮可能毁掉整个团队。原书给评审文化划的两条底线值得贴在墙上:评审不是证明你有多聪明的机会,也不是橡皮图章式的官僚主义障碍6。
3.2 被评审者侧:把你的改动变成好评的对象
准备请求。 一条改动的生命周期是准备、提交、评审、批准、合并7。好的评审请求(请求别人评审你改动的载体,在 GitHub 上就是拉取请求)有几个构造要素,原书给了样例:标题带上工作单号;描述写清改了什么、怎么验证;勾检查列表(是否加了测试、是否动了公开接口);把该来的评审者(个人加团队)都列上8。
草案降险。 很多人靠写代码思考。写得太早提交正式评审是浪费,闷头写完才发现方向错了更浪费——中间态是评审草案:标题挂上 DRAFT 或 WIP 字样的非正式请求,专门用来在错误的道路上还不太远时获得低成本反馈9。
别拿评审当测试跑。 大项目的测试套件又多又难跑,新人的诱惑是「提交评审让持续集成(CI,合并前自动构建和测试的流水线)替我跑一遍」。原书点名这是糟糕做法:你填满了测试队列,堵住真正要合并的评审;队友误以为你的请求需要人看;而 CI 只会跑全套,你明明只需要跑相关的那几个10。正确动作是花时间搭好本地环境——本地调试失败测试,比隔着 CI 环境容易得多11。
大改动先预排。 几千行的改动在网页评审界面里根本看不动。原书的工具是预排会议:当面分享屏幕,从页面加载或程序启动一路走到执行结束,把新抽象的主要概念讲给团队听——注意,预排不是评审,意见留到正式评审环节;预排给的是「为什么要这么改」的心理模型12。
接反馈的姿态。 两条:一是别太在意——保持一点情感距离,评审意见针对代码,不针对你,而且代码很快就不全是「你的」了,整个团队都会拥有它13;二是保持同理心但不容忍粗鲁——有些人的「简短且命中要害」在别人那里就是「粗暴无礼」,遇到越界的评论要明确指出来;聊不清楚就转成面 对面14。真遇到分歧,先问自己「我护着这段代码,是因为我写的,还是因为这样确实更好?」,解释你的观点,仍谈不拢就交给团队的裁决惯例——有的服从提交者,有的服从技术负责人,有的服从小组法定人数15。
保持主动。 评审者会被各种提醒淹没,没动静就去问(但别催);收到意见尽快回——你回得越快,对方回得也越快。批准之后尽快合并:悬而不决的评审会需要变基(把你的分支挪到最新的主干上重放一遍),极端情况会破坏代码逻辑,逼你再来一轮评审16。
3.3 主走查:一条评审意见的两个版本,和一次完整的请求
原书在两处给了「同一件事的差版和好版」,把它们拼成一条走查最能看出评审手艺的分寸。
走查第一段:一条意见。 场景:被评审的代码在处理网络端口。作者收到的意见原文分两截——第一截:「校验端口是否大于或等于 0,如果不是,需要触发 InvalidArgumentException 异常。」紧跟 着一行「端口不可能是负值」17。这条意见好在哪?它同时给了「什么」和「为什么」:动作(校验并抛异常)+ 理由(端口不可能是负数)。原书给评论的总原则就是按「你们坐在一起评审代码时的说话方式」来写——太简短的评论既难懂又容易显得像命令18。对照第 01 章潘卡的坏邮件,结构是一样的:给足背景,说清理由,让人能行动。
走查第二段:一次请求。 把原书的 UI-1343 样例完整走一遍字段19:
评审者: agupta, csmith, jshu, UI/UX 团队 ← 个人+整个相关团队
标题: [UI-1343] 修复了在目录 Header 上缺失链接的问题
← 带工作单号,可自动关联需求
描述:
# 概述
主页目录 Header 上缺失「关于我们」的链接;现状是单击目录按钮
没有反应,通过追加一个正确的链接修正。
追加了一项 Selenium 测试来验证本次修改。 ← 主动交代怎么验证
# 检查列表
- [x] 添加新的测试代码
- [ ] 修改面向公众的 API ← 让评审者一眼看到风险面
- [ ] 把设计文档涵盖在内
评审者拿到这份请求,从分流(小改动、中等风险,可以安排进今天的评审时段)到理解意图(补一个漏掉的链接,修按钮无响应)都不需要追问;检查列表里「未修改公开 API」替他省掉了兼容性盘查。原书补了一个细节:很多代码库有评审模板,照着填就行——模板存在的意义就是把这些字段变成不用想就会填的习惯20。
3.4 评审者侧:分流、分寸与决断
先分流,再评审。 收到请求先定紧急度和复杂度:紧急的立即评;大多数改动不紧急,别每次有请求就中止手头的一切——评审和运维工作一样,规模和频率无法预知,不控制就会摧毁你的产出21。具体动作:在日历上划出固定的评审时段;大评审(预计一两小时以上)建一张任务票来跟踪,和你的管理者商量把它排进冲刺22。
先懂意图,再开评论。 不要上来就写评论。先读、先问:为什么要做这个改动?代码过去什么样?改完之后什么样?原书的提醒很实用:理解动机之后,你可能会发现某些修改根本不需要23。
给全面的反馈,但分清重量级。 该看的清单:正确性(读测试、找 bug)、可实施性、可维护性、可读性、安全性(按 OWASP 十大扫一遍)、公共 API 的兼容影响、未来的人会怎么误用这段代码24。但不是所有意见一个分量——原书给了一套标签纪律:同样的问题反复出现时,别喋喋不休,指出第一例、声明「这是要全面展开的问题」;风格类的小挑剔,评论前加 Nit 前缀(意即「吹毛求疵,可无视」),这是业界惯例;更好的建议但不必阻塞合并的,加上「可选」或「非必须」 字样25。如果你发现自己写的评审大多在挑风格,先问问项目是不是缺代码检查工具——该工具干的活,别用人干26。
承认优点。 评审自然地盯着问题,但好的部分也要说:学到新东西就明说;重构清掉了问题区域、新测试降低了未来风险,就给鼓励性的评论。原书甚至说:即使是一项令你讨厌的修改,也可以对它的意图和努力说句好话27。
拒绝橡皮图章,也不当路障。 两个对称的失败:迫于压力看都不看就批准(橡皮图章)——团队成员会以为你看懂了,出了事你要担责;反过来,坚持完美、把「相邻代码还能改进」塞进本次评审——那是让「完美」杀死「优秀」。原书引了谷歌的工程实践文档:「一般来说,评审者应该倾向于批准 CL,只要它处于肯定能改善正在运行的系统的整体代码运行的状况,即使 CL 并不完美」28。发现顺手可改的相邻问题?另开一张任务票,把工作留到以后29。如果请求大到没法认真评——数千行的改动对单人评审不合理——就要求作者拆小,或者改为预排会议30。
别忘了两处暗角: 测试代码也要评(先读测试再读实现,往往更快理解意图;专门找那几种糟糕模式——依赖执行顺序、缺乏隔离、调用远程系统,第 05 章的检修单在这里直接复用)31;以及别被网页界面困住——把改动迁出到本地,用你的 IDE 看、跑起来、附上调试器、甚至亲手触发那个失败场景32。
4. 作者的判断与证据
- 「评审是教学工具/共同理解的保障」:作者判断,论据是机制推演(改动被更多人读过)加行业惯例,不是对照实验。
- 谷歌引文:「肯定能改善整体状况即批准,即使不完美」出自谷歌工程实践文档,是本章唯一引用外部机构明文规则的地方;原书同时提醒:那是为谷歌写的,你的公司对风险容忍度、自动化投入的偏好可能不同33。
- 「不要容忍粗鲁」:作者立场,且给了升级路径(当面聊、找管理者)——说明这不是「忍一忍」的建议。
- 预排会议的定位(传播心理模型,不收意见):作者的操作性判断,依据是「同时做两件事两件都做不好」的经验。
5. 边界与局限
- 规模假设:本章默认团队评审全部改动;小团队、结对编程密集的团队可能以更轻的流程覆盖同等价值,书里没有讨论「多小的改动可以免评」。
- 工具视角偏 GitHub 式拉取请求:内部评审系统(如谷歌的 CL)的差异只在引文里带过一句。
- 文化依赖:「Nit」「非必须」这套标签依赖团队共识;没有这套共识的团队直接照搬,标签本身也会变成新的扯皮点。
- 书里没有给「评审响应时长」的行业基准(比如「24 小时内响应」这类常见团队公约)。
6. 可带走的
- 评审买五样:抓 bug、教学、共同理解、决策档案、合规——只把评审当质检,亏了四样;
- 一切好处的共同前提是高度信任;做不到时,流程还在、价值已经没了;
- 评审请求要自带上下文:单号、概述、验证方式、检查列表;大改动先发草案或开预排;
- 别用提交评审的方式触发 CI——那是把测试队列当免费劳动力,代价是队友的时间;
- 评审者先分流再评审,给评审留固定时段;大评审建任务票、进排期;
- 意见要有「什么」和「为什么」(「 端口不可能是负值」比「校验端口」有力量得多);
- 用标签分重量:Nit 是挑剔、可选/非必须是不阻塞;重复的风格问题去找工具而不是找作者;
- 谷歌口径可抄:「肯定能改善整体状况即批准,即使不完美」——相邻的改进另开任务票;
- 测试代码也在评审范围内;看不动网页 diff 就迁到本地跑起来;
- 批准之后尽快合并——拖延的评审会变基,变基可能毁掉逻辑,又是一轮评审。
7. 原文地图
| 主题 | 原书章 | 原文位置 |
|---|---|---|
| 五重价值 | 第7章 代码评审 | text/16-ch07.txt:22(搜「教学工具」) · text/16-ch07.txt:31(搜「共同理解」) · text/16-ch07.txt:38(搜「实现决策的档案」) |
| 合规要求 | 第7章 代码评审 | text/16-ch07.txt:40(搜「安全性与合规性」) |
| 高度信任 | 第7章 代码评审 | text/16-ch07.txt:43(搜「高度信任」) |
| 两条底线 | 第7章 代码评审 | text/16-ch07.txt:52(搜「橡皮图章式的官僚主义障碍」) · text/16-ch07.txt:51(搜「拉锯扯皮」) |
| 生命周期 | 第7章 代码评审 | text/16-ch07.txt:56(搜「最后批准和合并」) |
| 主走查:请求样例 | 第7章 代码评审 | text/16-ch07.txt:80(搜「UI-1343」) · text/16-ch07.txt:90(搜「检 查列表」) |
| 评审草案 | 第7章 代码评审 | text/16-ch07.txt:112(搜「评审草案」) |
| 别触发 CI | 第7章 代码评审 | text/16-ch07.txt:130(搜「触发持续集成」) · text/16-ch07.txt:137(搜「如何在本地运行」) |
| 预排会议 | 第7章 代码评审 | text/16-ch07.txt:147(搜「预排会议」) · text/16-ch07.txt:166(搜「心理模型」) |
| 别太在意 | 第7章 代码评审 | text/16-ch07.txt:171(搜「情感上的距离」) |
| 不容忍粗鲁 | 第7章 代码评审 | text/16-ch07.txt:186(搜「粗暴无礼」) |
| 分歧的裁决 | 第7章 代码评审 | text/16-ch07.txt:197(搜「法定人数」) |
| 尽快合并与变基 | 第7章 代码评审 | text/16-ch07.txt:211(搜「变基」) |
| 分流 | 第7章 代码评审 | text/16-ch07.txt:216(搜「分流」) |
| 预留评审时间 | 第7章 代码评审 | text/16-ch07.txt:239(搜「划出代码评审时间」) · text/16-ch07.txt:246(搜「任务票」) |
| 先懂意图 | 第7章 代码评审 | text/16-ch07.txt:253(搜「理解拟议的代码修改」) · text/16-ch07.txt:257(搜「修改的动机」) |
| 全面反馈 | 第7章 代码评审 | text/16-ch07.txt:263(搜「可实施性」) |
| 主走查:意见两截 | 第7章 代码评审 | text/16-ch07.txt:279(搜「InvalidArgumentException」) · text/16-ch07.txt:280(搜「端口不可能是负值」) |
| 承认优点 | 第7章 代码评审 | text/16-ch07.txt:285(搜「也要进行赞扬」) |
| Nit 与可选 | 第7章 代码评审 | text/16-ch07.txt:302(搜「Nit」) · text/16-ch07.txt:321(搜「接受或不接受」) |
| 别喋喋不休 | 第7章 代码评审 | text/16-ch07.txt:312(搜「全面展开的问题」) |
| 找工具而不是找作者 | 第7章 代码评审 | text/16-ch07.txt:315(搜「代码检查工具」) |
| 拒绝橡皮图章 | 第7章 代码评审 | text/16-ch07.txt:333(搜「橡皮图章」) · text/16-ch07.txt:341(搜「数千行」) |
| 谷歌引文 | 第7章 代码评审 | text/16-ch07.txt:381(搜「倾向于批准」) |
| 另开任务票 | 第7章 代码评审 | text/16-ch07.txt:387(搜「另开一张任务票」) |
| 评审测试代码 | 第7章 代码评审 | text/16-ch07.txt:360(搜「忽略测试代码」) · text/16-ch07.txt:365(搜「缺乏隔离」) |
| 本地迁出 | 第7章 代码评审 | text/16-ch07.txt:349(搜「迁出或下载」) |