1. 先说说我对代码评审这件事的重新定义1.1 代码评审不是找茬是一场效率博弈我进入这个行业已经有十多年代码评审这东西从最开始流行的结对编程变体到后来各家团队摸索出的各种流程规范我基本都经历过。如果让我用一句话总结这些年的感受那就是能把代码评审做好、做持久、做出价值的团队一定是把它当效率工具在经营而不是当QA流程在应付。为什么这么说我见过太多团队把Review当成上线前的最后一道关卡评审会开得像批斗会评论区长满了这里为什么要这么写建议改成xxx这类没有上下文的短句reviewer自己都没看明白调用链就凭感觉说话。这种评审不仅没有提高代码质量反而把写代码的人搞得畏手畏脚把审代码的人搞得意兴阑珊。最终结果就是Review从质量保障手段退化成了流程形式主义。反过来看我待过的一个比较成熟的团队他们对Review的理解非常直白你写的代码是给下一个维护它的人看的评审就是提前替那个人把疑问问出来。想通了这一点很多流程设计就都顺了。1.2 我见过最典型的三种错误心态第一种把Review当成审核关卡。评审人默认代码是有罪的作者需要自证清白。这种心态下reviewer会钻牛角尖作者会防御性解释双方最后都在争面子而不是代码质量。第二种把Review当成教学现场。资深工程师看到新人的代码忍不住逐行指点一次Review写下几十条评论。出发点是好的但效果往往很差——新人被淹没在意见海里反而抓不住重点。我一直认为一次评审能带出3到5条真正重要的意见就已经很成功了。剩下的细节问题完全可以通过静态检查、结对编程、或者另一个小迭代去消化。第三种把Review当成走过场。所有人都知道在看但所有人都没认真看看到绿勾就点通过。这种情况最危险因为它会让整个团队对评审机制失去信任一旦有了反正没人仔细看的共识垃圾代码就会流向主干回头再来补债成本高好几倍。我不打算再展开说这些心态有多糟糕因为我写这篇博文的主要原因是想分享这半年多来我在推进一个内部代码评审改进计划时沉淀下来的具体做法和踩过的坑。从定义评审目标、设计流程、量化指标到处理评审中的情绪问题和工具链替代每一块我都试过不少方案。这些东西不是论文里的框架是我和团队一步步调出来的写出来供同行参考。2. 一套能让评审不那么痛苦的提交与分配机制2.1 提交之前把改动描述当产品文档写我在Review实践里踩过最痛的一个坑有一次某工程师提交了一个改动改动说明只写了两个字update连标题都是系统自动生成的分支名。我点开代码一看改动涉及一个核心服务的配置结构、一个数据迁移脚本、还顺带重构了一个工具函数。我花了将近四十分钟才拼凑出他到底想干什么而且中途还判断错了一个方向给了完全错误意见。那次之后我定了一条规则改动说明写不清楚的PRreviewer有权利直接打回不用替作者猜。具体来说我觉得一个合格的PR描述至少要包含这几块内容背景为什么要做这个改动是响应了哪个需求、修复了哪个线上问题还是单纯的技术债清理改动概览涉及了哪些模块每个模块大致做了什么风险点有没有影响兼容性、有没有改到公共方法、有没有涉及数据迁移测试情况本地跑了哪些用例、有没有手动验证过的场景。这里我想多说一句测试情况。很多团队Review时最焦虑的就是我看了半天也没法确定这个改动到底跑没跑过。如果作者在描述里能明确写出我在xx场景下用xx数据验证过结果符合预期reviewer的状态会完全不一样——他从怀疑一切变成针对性地看某几个地方效率提升非常明显。我在内部推动这事的时候没有直接要求大家写长篇大论而是定了一个模板字段不多不少就四行默认值都是无逼着提交的人强制过一遍脑子。实行两周后Review评论里这是什么改动为什么改这里这类问题占比下降了大概六成。2.2 改动越小评审越快这几乎是所有评审效率问题的核心但恰恰最难执行。很多人嘴上认同小步提交实际操作中却总是攒一个大分支等所有功能都做完再开PR。原因也简单——拆分是反人性的人总是倾向于在脑子里把一个功能想完整了再动手代码也一样写到一个自认为可以见人的状态才愿意拿出来。解决这个问题我必须诚实地讲没有魔法只有硬性约束。我们团队后来设了一个红线——单个PR的改动量新增加删除的行数一般不超过400行涉及架构调整的大改动例外但例外必须提前在群里说明。超了的主管会找你聊原因不算惩罚就是帮你去拆。这个数字不是拍脑袋定的参考了行业里一些公开的经验数据——改动量越大reviewer每行代码投入的注意力就越少术语叫评审密度下降。一个小改动评审人能看到80%的细节一个几千行的大改动能看个大概就不错了。拆分还有一个好处出问题容易回滚和定位。有一次我们把一次重构拆成了四个连续的PR前三个合进去之后线上监控数据平稳最后一个出了问题直接回滚最后一个前三个保留。如果在同一个大PR里做回滚就是牵一发动全身。2.3 谁来Review、何时Review大部分团队都没定规则很多团队的做法是谁有空谁看或者在群里吼一嗓子帮忙review一下。这样做的直接后果是热门模块的代码反复被同两三个人看冷门模块的代码常年没人认真检查。我后来参考了一个开源社区推荐模型给团队定了一套简单的规则每个PR至少两个reviewer其中一个必须是该模块的熟悉者怎么定义过去一个月在这个目录下有最多提交记录的人另一个可以是任意工程师用来补充外部视角Review响应时间不超过四个工作时超过就算延误自动化工具会把PR顶到群置顶作者在非工作时间发的PR不强制当天响应避免大家下班了还盯着手机审代码。这里我想展开说一下外部视角。为什么需要一个不了解这块代码的人来review因为我发现模块熟悉者很容易陷入惯性理解——他看到某段代码会觉得哦这里本来就是这么写的默认一切合理。而一个没写过这块代码的人会真的傻乎乎地去追每一个函数调用结果反而最容易发现这个函数原来不是这个语义这里有个边界条件没处理这类问题。这和我们平时说的代码评审要看新人视角是一个道理。至于响应时长的约束说实话一开始阻力不小有人觉得我在专注写功能不想被打断。但后来我们把期待调整为四小时内给出初步回应哪怕结论是你现在很忙、最晚明早看大家就都能接受了。关键是不能让人发完PR就一直悬在那里得不到任何反馈。3. 评审过程里最容易被忽略的技术判断点3.1 从这段代码对不对到这段代码该不该存在评审技巧这东西很多技术文章都在讲你要看并发安全问题你要看边界条件这些当然都对。但我想说一个很少被系统化强调的点最高级的评审意见是能指出这段代码压根不该写。什么意思我举一个真实的例子。某团队有个模块每次调用外部服务都要先拼一个很长的签名串代码里重复了五六遍。A同学某次提交把这套签名逻辑抽成了一个公共工具类然后所有调用处都改成了调用新方法。从代码正确性角度讲这个改动没有毛病测试也过了压缩重复代码本身也是好事。但review时一个资深的工程师提了个问题为什么我们不把这个签名计算放到基础库里让所有服务统一使用而是只在这个服务内部抽个工具类这一问变成了一个架构评审议题——这个工具类最后真的下沉到了公共模块收益翻了不止一倍。所以我在评审时给自己定了两个原则每看一段新增代码先问一句这个功能本来是不是已经有现成的东西能实现了很多重复的轮子不是不知道去哪里找而是没人愿意花十分钟翻一下现有的依赖每看到一次删除代码会特别注意——如果删掉的是公共接口会不会有其他模块在调用这里需要多跑一步全局搜索别只看着当前PR的代码。这个维度写不进自动化检查工具恰恰是Review作为人肉活动的核心价值。3.2 并发、幂等、失败路径三类高发问题在看代码评审意见时我统计过一个大概印象——除了纯逻辑错误引发线上事故最多的问题集中在三类并发处理、幂等性、失败路径。并发问题的典型形态是新来的工程师写了个带状态的对象在多线程环境里直接用没有加锁也没有做线程封闭。这类问题看着代码很难发现因为本地测试不会触发。我的建议是review时凡是看到静态变量、共享的单例、或者写入全局容器的地方都下意识问一句这个会不会被多个线程同时碰幂等性问题在接口改造中尤其高发。比如支付回调这个场景回调可能会因为网络原因来两次、甚至三次。如果处理逻辑没有做幂等就可能在重复退款、重复扣费这种级别的事故里栽跟头。Review时我会特意看这个接口是不是天然可能被重复调用如果是它的去重逻辑在哪失败路径则是另一个容易忽视的点。很多代码写的是happy path主线一路顺畅能跑通但网络超时、缓存击穿、下游返回异常数据这些情况往往没有覆盖。我看过一个印象很深的PR——做了缓存管理的重构但完全没考虑Redis临时不可用怎么办代码直接抛异常。review意见只需要简单提一句降级策略是什么作者可能就会反应过来补上一个兜底逻辑。这三类问题有一个共同点都是无法通过看代码对不对来发现的必须靠reviewer主动拉高视角想象这段代码在真实运行环境里的样子。所以我一直觉得好的reviewer本质上是在脑子里做一次演算而不是在逐行做检查。3.3 如何提意见别人才愿意改这里我想说一些比较实际的东西。同样的意见不同的表述方式效果差很远。我观察到一个现象很多工程师提意见时喜欢用命令式短句比如这里改成用缓存去掉这个魔法数用Optional。这些话技术上是没毛病的但接收方容易产生一种被指挥的感觉尤其是在双方资历有差距的时候。我后来练习了一种表述结构简单说就是原因-建议-开放式结尾先说明我为什么这么提比如这个函数在循环里被调用了上千次每次创建新对象开销有点大再给出具体建议可以考虑提出来缓存一份最后补一句开放式的你觉得呢或者你了解过这里的数据量吗这样做的效果很神奇。接收方不会觉得被命令反而会带着要不要接受这个建议的心态去看讨论质量明显提升。另外还有一个很实用的小技巧——评论尽量绑定到具体的代码行上不要只写一个总结性的长篇大论放在PR最后。绑定到代码行的评论作者改动的时候方便对照讨论时上下文也清晰。放在最后的大段总结作者大概率会看但很难形成一对一的回应。如果有人问我Review意见收到后不知道怎么回复怎么办我会建议最简单的做法每条意见回一个改或者不改原因是xxx。不需要长篇大论的辩护也不需要卑微的道歉。Review讨论的本质是信息交换不是辩论赛更不是法庭审判。4. 那些让评审形同虚设的坑和真实排查4.1 已通过了但谁都没认真看的假评审有一段时间我发现一个比较隐蔽的问题某小组的代码质量没有变好但Review通过率一直是100%评论数低得异常。我一开始以为他们代码写得好后来偶然翻了一下评论记录发现大量PR的reviewer点开没几分钟就通过了有些评论内容甚至是空模板。这种情况就是典型的假评审。它比不评审更可怕因为它制造了安全假象。排查原因的时候我发现两个核心问题reviewer人选和作者是同一个小圈子里互相吹捧的伙伴不愿意给对方制造麻烦reviewer自己心里没底尤其是刚加入团队、对系统还不熟悉的人他觉得我看了半天没看出问题别是我水平不够那就算了通过吧。针对第一种我们调整了分配规则引入了跨组Review——不是本小组的人来当第二评审人。针对第二种我们在新人入职的前两周不强制安排独立Review跟着老人看、写意见草稿由老人过目后再发。同时把发表意见的心理门槛降低了明确告诉所有人不确定是不是问题的意见也欢迎提出来用疑问句就行作者自己会判断。4.2 Review疲劳意见说多了自己也累Review这事其实挺吃心力的。一次认真投入、有信息量的Review可能需要半小时甚至更久连续做几个之后大脑注意力就会明显下降。我们团队有过一个阶段几个人连续两周高强度评审结果后几天开始陆续出现只挑语法毛病的评论比如缩进不对、变量命名不好而真正的逻辑漏洞反而没人提。这是认知疲劳在作祟。人的精力有限前几个PR消耗掉大部分精力后面就进入省电模式。解决Review疲劳我从两个方向入手控制单日评审量——设置了每天评审的PR数量上限超过上限的自动转到第二天或者分流给其他有精力的工程师。这里需要团队有一定的人力冗余才能扛得住但哪怕只是明确这个意识也比硬撑要好把简单的流程动作机器化——格式化检查、静态规则扫描、测试覆盖率门禁这些琐碎但耗神的事情一股脑交给CI自动化跑掉评审者只需要关注真正需要人工判断的部分。第二条我记得一开始有个不小的阻力因为当时有些同事觉得静态检查工具报的问题太多了根本没人看然后就直接把工具给关了。我的态度是工具报对的问题你要想办法让工具闭嘴而不是把工具关了。比如某个规则误报率高那就配置白名单、调阈值让它只在真正违规的时候说话。调好的工具链能省下大量评审人力。4.3 用数据暴露评审质量问题很多团队Review做得怎么样全凭感觉——最近好像大家review都挺积极的。感觉这东西是会被集体情绪带偏的。为了搞清楚真实状态我给自己内部的实践项目加了一组简单的数据跟踪PR的平均首次响应时间从提交到第一个reviewer给出第一条评论的时间。评审覆盖率有至少一条实质评论的PR占所有PR的比例。评论分布评论数是否集中在某几个人身上其他人是不是从不发言。意见采纳率作者明确回复改或代码里落实了修改的比例。这些数据不用做复杂的大屏看板每周手工导一次Excel表格就能看个大概。有一次统计发现某个组的首次响应时间中位数已经超过了几个小时说明PR经常躺在挂着。拿到数据之后我们在站会上升级了这个问题的优先级很快就有了改善。数据不一定会让问题立刻消失但它至少能把问题从装看不见变成明摆着。这里我想澄清一点数据是为了服务持续改进不是为了给员工排名、扣绩效。一旦变成KPI考核工具每个人就会想办法刷数据——比如疯狂发LGTM来把响应时间刷下去。所以这些指标只能团队内部参考不适合做跨团队横向排名。5. 把Review往前推机器能做的别用人来做5.1 静态检查与格式化交给机器这个话题也许有人觉得老生常谈但我观察到很多团队的实际落地仍然很草率。他们可能有CI、有静态检查工具、有Lint规则但因为配置粗糙或者历史报错太多开发者已经把工具的输出当成背景噪音——反正错了也不会被拦真拦了我就在代码里加个忽略。合理的方式是逐步收敛。我不建议为了看起来很严格一次性打开所有规则那样只会逼疯所有人。我常用的步骤是先只开两类规则错误级别的error和风格强制的format其余全部warning级别给团队一个过渡期比如两周处理存量告警从某个时间点开始CI里对error做硬门禁不过不开PR合入每季度review一次告警趋势针对反复出现的规则单独开专项清理。一旦这一步走顺Review能省出一大块时间。那些少了个空指针判断这个函数命名不符合规范之类的低级意见机器全给你挡了评审人看到的是已经干净过的代码体验完全不同。5.2 基于提交历史和个人习惯的经验沉淀这是我自己比较得意的一个小实践。我们基于内部的提交历史做了一次简单的挖掘——把过去一年里发生过线上问题的改动拉出来做了回溯归类。结果发现有一类问题是反复犯的某个服务的超时时间被改短、某个接口的错误处理被吞掉、某个并发容量的初始值被写死。这些问题在Review时其实是可被识别的高频信号。于是我们做了一个轻量级的Review检查清单——不是挂在Wiki上吃灰的文档而是集成到了评论模板里这个改动是否涉及外部依赖的超时或重试异常是被抛出还是被吞掉了吞掉的有没有日志并发场景下共享状态是否被正确保护数据库字段变更是否考虑过存量数据兼容清单条目不多就十条左右但每一条都是从真实事故里提炼的。评审人看代码时可以对照这些信号去定向排查比自己漫无目的地看要高效得多。再过一段时间我们会把这个清单做成一个自动化扫描器——用一些启发式规则去匹配代码模式比如吞异常但没打日志这种模式正则都能探个大概。机器先扫出可疑位置评审人再人工确认。我认为这才是代码评审工具化的理想形态机器负责掏信号人负责做判断。这里补充一个容易踩的坑经验清单一定要保持轻量迭代每半年删掉一些已经不重要的条目、加入新发现的高频问题。如果清单越攒越长评审人记不住最后就会变成摆设。6. 实践一年之后我沉淀下来的几条操作原则写了这么多最后分享几个我在实践中反复验证过的操作原则都是即拿即用的那种。第一条把Review纳入工作排期而不是空余时间。很多工程师不把评审当正经工作往往是写完自己的代码才顺便去看别人的PR。这导致Review质量甚至被优先度无限压缩。我建议每天上午留出固定一小时看评审效果比下午或者晚上好很多——上午头脑清楚逻辑链完整评审质量明显更高。第二条宁可少开几个PR也要保证每个PR能被完整看完。这个观念和小步快跑不冲突。小PR的核心价值是能被完整看完而不是数量多。如果团队一天发出来二十个PR但没有一个能被认真看完还不如只发五个、保证每个都被认真看完。第三条写Review意见的时候多问自己一句这条意见值不值得别人花时间看如果答案模糊宁愿先不发等想清楚了再补。垃圾意见和假通过一样都会消耗团队对评审机制的信任。信任崩塌一次重建的成本极高。第四条评审既是技术活也是沟通活情绪管理放在第一位。一旦评论里出现了你怎么这么写这不是基本常识吗这类带情绪的词这个评审的质量就已经开始下滑了。技术讨论建立的前提是双方都愿意理性说话。哪怕你真觉得对方犯了低级错误也先确认一下是不是有隐性原因——很多时候背后有上下文只是你不知道。最后再分享一个小技巧回头看看自己一个月前写的代码然后用评审别人的心态去审一遍。这是提升评审能力最快的方式。你会惊讶地发现自己当初写的东西有多少漏洞和笨拙之处这种切身体会比读任何评审指南都有效。我也是这么练下来的现在每次看到旧代码都跟自己说还好当时有人在旁边帮我拦了一道。