Code Review 实战：告别 LGTM 敷衍，建立高效评审机制

我见过太多团队的 Code Review 已经沦为一种仪式感PR 一挂出来同事点个 LGTM然后 merge 按钮亮起绿灯所有人都松了一口气——仿佛只要有人看过一眼质量就有了保障。你如果追问一句到底看什么了对方大概率会愣一下然后说看了看逻辑没啥问题。这种快速点赞式的评审本质上不是在控制质量而是在给事故的前奏拍手。open-code-review这个主题我一直想认真聊一次。这里面的open不只是开源的意思更是一种心态代码是开放的、意见是开放的、评审过程本身也应该是开放的。不是找个人给你背书而是让代码在合入主干之前接受一轮真正有价值的设计复审、逻辑推敲和经验补充。这篇文章不打算推荐某个神级工具也不会扔给你一套模板让你抄完就完事而是想拆清楚一次不算浪费时间的 Code Review到底应该看什么、怎么推进、用什么机制让它稳定地运转起来。如果你正在为自己或团队搭建评审流程或者单纯想知道别人 review 代码时到底在想什么这篇文章应该能给你一些能直接用的东西。1. Code Review 沦为快速点赞的背后先看清病灶在哪很多团队不是不想把 Code Review 做好而是做着做着就走样了。先别急着定制度我们来看看最常见的几种变形。1.1 LGTM 文化只签收不审查LGTMLooks Good To Me本身没有错错的是它成了唯一的评论。我在一些团队里看到一个包含十几个文件、改动上千行的 PR评审人的评论只有一行 LGTM数据流、边界条件、异常处理全都没人提。为什么会这样因为看不懂或者懒得细看。看不懂往往是因为改动范围太大、上下文太复杂评审人很难在碎片时间里真正建立起完整的代码脉络。懒得细看则更隐蔽——大家默认写代码的人应该自己负责review 不过是流程卡口所以用最快的速度通过既不得罪人又不耽误自己的时间。但讽刺的是这种心态恰恰让 Code Review 失去了最重要的价值它本来是一个多双眼睛降低认知盲区的机会结果变成了一个人拍拍脑袋的签字仪式。1.2 评审对象错位把格式问题当核心问题另一种常见病是评审人在代码风格上花掉 80% 的精力。缩进对不对、变量名是不是够短、函数是不是该拆成三行……这些讨论不是没有意义但它们的优先级远低于架构合理性、业务正确性和潜在风险。我曾经见过一个团队在 PR 评论里为一个函数该不该拆成两个吵了整整两天但那个改动里明明藏着一个并发更新状态时的竞态条件没人注意到。后来线上出了问题大家才回过味来——吵的力气全都花在了不重要的地方。格式化、命名、甚至一部分重构建议这些其实都适合交给机器和工具去处理人应该把脑力集中在机器判断不了的语义问题上。后面我会单独讲工具链的配合方式。1.3 范围失衡要么看不完要么没得看还有一个几乎无法避免的困局PR 太大评审人望而却步PR 太小又感觉 review 是在浪费时间。一个几千行的巨型 PR本质上是把评审的难度推给了别人。哪怕评审人有心细看也很难在有限时间内把整体逻辑捋清楚最后只能草草看一遍就放行。而那种一次只改一个标点的 PR虽然风险极低但频繁的打断同样让人疲惫。所以你会发现Code Review 能不能做好很多时候在提交代码的那一刻就已经注定了——差异太大后面怎么努力都事倍功半。1.4 讨论没有闭环评论飘在空气里还有一种情况非常典型评审人提了个问题写代码的人回了一句这个我后面优化一下然后就没有然后了。评论还挂在页面上改动已经合进去了后面再优化的那部分永远躺在了 backlog 里。这个问题看上去是流程问题本质上是闭环机制的缺失——没有人定义什么才算一次评审结束也没有人把待办项变成可跟踪的后续任务。评审中提到的每一件事要么当场改掉要么明确记录为后续任务并指定负责人否则那些评论只能是白纸黑字的空气。2. 看代码不是看字面五个真正值得纠结的审查维度既然要 review就得有个抓手。我自己的习惯是拿到一个 PR 不会马上从第一行 diff 开始读而是先带着五个问题去看这样效率和深度都会高很多。2.1 架构与调用边界这个改动站对位置了吗这是最容易被忽略、但影响最深远的一层。要看的不只是这段代码对不对而是这段代码该不该出现在这里。比如一个订单服务为了图方便直接在 Service 层里拼 SQL 查了别的库的表跑起来没问题但这是把数据层的细节泄露到了业务层。短期看没什么等到后来数据源切换、表结构变更的时候你会发现到处都在直接操作底层的细节改一处崩三处。架构评审的核心是看依赖方向是否合理、模块边界是否清晰、有没有把本应封装在内部的实现细节暴露出去。我自己常用的判断方式是如果这个文件一年后要重写它的调用方会不会受影响受影响面太大说明边界划得不对。2.2 业务正确性与边界条件正常路径之外的代码才见功力很多 bug 都不是出现在正常流程里而是出现在边界条件被触发的时候。所以我会特别留意一段代码对这两类问题的处理数据异常了怎么办流程中断了怎么办。举几个我 review 时经常追问的例子如果某个依赖服务返回了空列表代码会 NPE 吗如果用户传进来的是一个超长字符串后面还有空间处理吗如果更新数据库时检测到版本号冲突是直接报错还是自动重试金额计算用浮点数还是 Decimal并发情况下两个请求同时操作同一条数据会不会覆盖更新这些问题不一定每个都要在评审里问一遍但至少要想一遍。哪怕最后结论是这个场景不会发生也得有个明确的原因而不是完全没有考虑过。2.3 可读性与可维护性代码是写给下一个维护者看的有一句我一直很认同的话代码写的次数是一次但读的次数是几十次。所以可读性不是锦上添花是实打实的维护成本。但这里说的可读性不是指命名够不够短、函数是不是长了那么几行。我更关注的是别人能不能理解这段代码的意图。比如一个魔法数字 86400如果注释不写这是一天的秒数下一个人根本不知道它为什么出现在这里。再比如一个函数名叫做 handleData你看完签名完全不知道它处理的是什么数据、产出什么结果、有什么副作用。另一个点注释写是什么通常是废话写为什么才是价值。循环里先查出所有子节点是废话这里用递归因为树深度不会超过三层且上层已经做过了权限过滤才是信息。2.4 安全与风险面上线之前就想清楚出事了怎么办这个维度在业务评审里经常被一带而过但它值得单独拎出来。我 review 时至少会过一遍这几个点这个输入是用户可控的吗有没有做有效的校验和过滤这个接口是不是需要鉴权当前的权限模型覆盖到它了吗日志里会不会打出敏感信息比如手机号、身份证号、token这个改动上线之后能不能快速回滚不要觉得我们是内部系统攻击者不会来。大多数安全事件都不是因为被高科技攻击而是因为内部接口默认信任了所有调用方。代码评审是发现这类问题成本最低的时机等上了线再发现就是另外一个故事了。2.5 性能与资源不是所有代码都要快但不能无脑慢性能本身不是越优化越好而是符合预期的成本。所以我会特别关注那些顺手写出来的资源浪费循环里做数据库查询、批量接口里同步调用第三方服务、高并发路径上用了过大的锁粒度、大文件一次性读入内存。举个例子我之前 review 过一个导出功能实现方式是把全量数据查出来丢在内存里循环组装 Excel数据量小的时候毫无感觉但到了线上真实数据量下内存直接飙到告警线。这种问题在代码评审阶段就能看出来的前提是你在看的时候脑子里有一根弦——这段代码上线之后会跑在什么量级的数据上。下面是一个我 review 时常用作自检的速查表贴在屏幕边上挺管用的审查维度我会反复问自己最想看到的答案架构与边界这段代码出现在这里合理吗依赖方向清晰没有越层调用业务正确性异常、边界、并发怎么处理每种非正常路径都有明确策略可读性下一个人能读懂意图吗命名表达意图注释解释原因安全与风险上线后会不会有权限或数据问题输入可信度有定义敏感信息不外泄性能与资源这个量级下还能扛得住吗复杂度和资源消耗匹配实际场景3. 从提交代码到完成合入我惯用的一条完整评审链路很多人把 Code Review 理解成提 PR 之后等评论实际上从你开始改动的那一刻起评审就已经开始了。我一般把整个过程切成五个环节。3.1 变更说明一份写得清楚的 PR 描述能省掉一半的沟通我在评审别人的代码时第一个看的不是 diff而是 PR 描述。一段合格的描述至少要说清楚三件事这次改动的背景是什么我的实现思路为什么是这样我做了哪些验证。背景解决的是为什么实现思路解决的是怎么做的验证解决的是凭什么信你。你把这些写清楚了评审人就能直奔核心去 review而不是对着代码猜你的意图。遇到那种只有一个fix bug描述的 PR我通常会在回复里先请他把描述补全——这不是刁难是因为连他自己都没有把问题定义清楚代码很可能也是随手写的。3.2 评审时间窗早一点、小一点、别让代码过夜我自己对团队的建议是白天提的 PR尽量当天回应晚上提的 PR第二天上午给出初步反馈。关键是快速响应哪怕只是先说一句我看到了预计两小时内给完整意见也比挂在页面上一整天没人理强得多。在这个环节里控制 PR 体量比什么都重要。我踩过的坑是有一个大功能分支开发了三周合回主干时一个 PR 里塞了六千行改动我硬着头皮看了两天看完一半就放弃了最后靠相信作者合了进去。后来我们就定了规矩单个 PR 尽量控制在一个工作日的产出量超过 500 行的改动要么拆分成多个可独立合入的阶段要么先做一次面向设计方案的预评审。3.3 先看测试再看实现测试暴露的是作者的思考方式很多人 review 的顺序是从头到尾把 diff 读一遍但我的习惯刚好相反——先看测试。测试是最能暴露作者思考方式的地方他有没有想过正常路径有没有覆盖边界条件和异常分支测试断言的粒度是行为正确还是凑够覆盖率有一次我 review 一个新的登录接口实现代码写得很漂亮但测试里只覆盖了密码正确能登录和密码错误返回失败两条路径。我追问了一句连续输错五次触发锁定了吗验证码失效重不重新发送账号被封禁时的返回信息是不是泄露了账号状态作者愣了一下——他确实没想。这些场景放到实现里看可能要在代码里找半天才能发现缺口但看测试的话一眼就能看出覆盖盲区在哪。看完测试再回头看实现你会带着这个功能到底应该怎么表现的预期去读代码而不是被代码带着走。3.4 在线评论与异步沟通意见要有优先级不搞批注轰炸在线评论是最基本的互动形式但我建议评论时给意见分层。我的分层方式是P0 是必须修改不改不能合入P1 是建议修改如果不同意要在评论区说明理由P2 是可选的优化提一下就好不用当作合入的阻碍。这样一来作者拿到评论时可以立刻分清轻重缓急评审人自己也避免了什么都想管的观感。很多评审效率低就是因为 P0 和 P2 的评论混在一起作者根本不知道哪条是生死线。如果评论里来回超过两轮还在同一个问题上打转我会果断停止在线辩论约个时间当面聊。文字的沟通效率在澄清事实阶段是够用的但在达成共识阶段会急剧下降。面对面或者语音通话十五分钟的讨论比在评论里打十轮字有用得多。3.5 合入与跟进评审通过不是终点待办不消失当所有 P0 评论都得到解决测试通过门禁绿了才是我个人理解里可以合入的时刻。但合入之后还有一件事要做把评审中提到的 P1、P2 事项记录成可跟踪的任务明确责任人和截止时间。这一步看起来多余但它是让评审意见真正产生长期价值的关键。很多团队 review 了很多轮也积累了很多好建议就是没人跟进最后全烂在 PR 评论区里。我的做法是每次评审结束后花五分钟把未解决的建议搬到 issue 或者任务看板里标注来自 PR #xxx 评审这样它们就不会被遗忘。4. 工具放在台面上CI 门禁、静态检查与 AI 辅助怎么配合人工很多人以为 Code Review 只是人看人但成熟的做法是把机器能干的事尽量提前交给机器让人把精力放在真正需要判断力的地方。4.1 把机器能判的全部自动化格式、风格、常见坏味道、甚至一部分安全漏洞这些都不应该占用评审人的脑力。我自己团队里跑的最基础的一组检查包括工具类型典型代表帮我挡掉什么格式化工具Prettier、Black、gofmt缩进、引号、换行等纯格式差异静态检查ESLint、RuboCop、SonarQube未使用变量、明显的坏味道、复杂度预警测试与覆盖率JUnit、pytest Coverage功能回归、单测覆盖盲区CI 流水线GitHub Actions、GitLab CI编译、单测、构建产物的一体化验证这部分的收益非常直接我 review 时遇到这段代码风格和项目不一致的评论次数几乎降到了零因为 CI 在第一时间就把这些问题拦截掉了剩下的只有语义层面的讨论。下面是我们在 GitHub Actions 里常写的一个简版流水线配置核心逻辑就是每次 PR 都跑一轮完整的检查和测试跑不通过的不允许合入。name: code-review-check on: pull_request: branches: [ main ] jobs: check: runs-on: ubuntu-latest steps: - uses: actions/checkoutv3 - name: Setup Node uses: actions/setup-nodev3 with: node-version: 18 - name: Install dependencies run: npm ci - name: Lint run: npm run lint - name: Unit tests with coverage run: npm run test:coverage - name: Check coverage threshold run: npx jest --coverage --coverageThreshold{global:{lines:80}}别小看这个简单的门禁它最大的价值不是发现了多少 bug而是让团队成员对合入标准有了统一的预期。CI 跑挂了不用等评审人说什么作者自己就知道回去修了。4.2 Code Owners让懂的人审到位的部分一个经常被忽略但极其好用的机制是 Code Owners。简单说就是给不同目录或模块设置指定的负责人当 PR 改到这些文件时系统自动把相应负责人加入评审人列表。比如你的项目里有支付模块、有用户模块、有推荐模块每个模块的 owner 一定是最了解这块历史包袱的人。让完全不了解支付业务的同事去评审支付逻辑他只能看看缩进和命名但让支付 owner 来看他一眼就能看出这个状态流转漏了退款分支。Code Owners 的价值就是把正确的人和正确的代码匹配起来而不是让所有人对所有代码都有一票发言权。4.3 AI 辅助评审能补漏但别指望它做决策这两年 AI 辅助评审工具越来越多了我也在部分项目里试用过。它确实有不可替代的价值能在几秒内扫完一个上千行的 PR找出显而易见的死代码、未处理错误、测试盲区甚至提醒我这个函数是不是可以被分解成更小的函数。但我的经验是AI 目前仍然缺乏对业务上下文的理解。它不知道你们的订单状态机为什么是这样设计的不知道这个用户字段的历史迁移过程更不知道产品经理昨天刚改过一段交互逻辑。所以我的定位很明确AI 是第一道人工以外的过滤器帮我挡掉明显的问题留出更多时间去关注那些真正需要商业判断和技术权衡的讨论。换句话说工具是放大镜不是裁判。最终对这个改动是否应该合入负责的依然是坐在屏幕前的人。5. 代码之争不靠嗓门评审意见的表达与冲突化解Code Review 不仅是技术活动还是一场社会活动。你永远要意识到你评论的是代码但对面坐的是一个有情绪、有自尊的人。同样的问题表达方式不同结果可能是愉快的改进和一场争论的差别。5.1 意见表达用情景-影响-建议替代你怎么写的我的习惯是每一条评论都尽量包含三个要素我说的是哪个场景它会带来什么后果我建议怎么改。比如简单一句这里写错了别人接收到的只有否定没有任何有效信息但如果你说当列表为空时这里会抛异常导致接口 500建议在循环前加一个空置判断并且返回空列表对方一下子就理解了你关心的点。不是所有人都能迅速接受批评但几乎所有工程师都会认真对待一个具体的 bug 和一个可行的修复方案。5.2 遇到分歧先同步目标再讨论方案评审中最重要的分歧通常都不是这个变量叫不叫 xxx而是这个架构方案到底行不行。这种问题很容易变成持久战因为两边都对自己的方案有很强的信心。我的处理方式是先回到目标我们这次改动的核心诉求是什么是满足短期的业务时间窗口还是追求长期的架构合理性和可维护性如果这次需求就是明天要上线那最快的可用方案就是答案如果是准备长期演进的系统那讨论的焦点就该放在扩展性和可维护性上。目标对齐之后很多分歧会自然消失——不是谁说服了谁而是大家发现争论的根本不是同一件事。5.3 当共识无法达成数据说话或者升级决策如果确实在技术选型或架构路径上无法达成共识还有两条路让数据和事实说话或者把决策升级给最终负责人。数据的方式比如分别列出两个方案在响应时间、可维护性、迁移成本等维度的对比或者写一个小的原型验证直接在实测数据里看结果。很多争论在原型面前都会变得非常高效因为正面刚过、跑起来测过之后抽象的争论就变成了具体的数字。如果仍无法达成一致就把它明确升级给技术负责人或者架构评审组并且尊重最终决策。项目里不能出现谁嗓门大听谁的这样的隐性规则那只会让下一次评审变成辩论赛。5.4 不要过度坚持我的风格还有一个心态很重要Code Review 不是把你的个人编码风格强加给别人的机会。每个人都有自己习惯的写法只要不违反团队规范、不损害可读性和可维护性很多细微差异都不值得提出来。我见过最让人疲惫的评审是那种每行代码都要按评审人的风格来写的。这种评审消耗的是大家的耐心和积极性最后的结果就是别人开始绕着你提 PR甚至连 review 都懒得认真做了。真正的团队规范应该是一套能被大多数人认同、能写进文档的约定而不是某个人的品味。6. 落地实操如何从零搭一套不会变成负担的评审机制理论聊了很多最后直接给一条能落地的路径。如果我现在接手一个新的团队要从零搭建 Code Review 文化我不会一上来就推全量强制 review——那样大概率会在三周内被所有人抵制。我会分几步走。6.1 先污染后治理从重点 PR 开始试点第一步先定一个范围很小的规则核心模块的改动、涉及支付/权限/数据迁移的改动、以及超过 300 行的 PR必须经过至少一个人 review。其他改动可以放宽先不做强制。这样做的目的是让团队先感受到 review 带来的实际好处——有人帮你在上线前抓住了一个隐蔽的 bug有人帮你指出了另一种更优雅的实现——而不是一开始就感受到又要排队等审批的麻烦。6.2 明确定义完成DoD第二步和团队一起把可合入的标准写清楚。我习惯用一份 Definition of Done 来做CI 全部通过单测覆盖率达到约定的阈值PR 描述写清楚了背景和验证方式如果有 P0 评论必须全部解决涉及核心模块的改动必须至少有对应 owner 的 approve。这不是用来卡人的是让所有人有共同的预期。标准越清晰review 执行起来就越省力因为不再需要每次讨论这算不算可以合。6.3 用数据控制成本Review 机制如果想要长期存在必须控制在可接受的成本范围里。我给团队的建议是两条单次 PR 的评审时间尽量控制在 20 分钟以内如果超过了说明 PR 本身拆分得不够每个 PR 的评论数尽量集中在 5 条以内如果超过了说明评审人和作者的沟通成本已经高到需要切换方式了。这不是硬性指标而是一个自我诊断的信号。每次你觉得review 好累的时候往回看一眼多半是 PR 太大、描述不清、或者讨论没有及时闭环这三件事中的一个出了问题。6.4 留出固定的复盘时间最后每隔一段时间跟团队一起看看 review 的产出和价值。我的做法是每月花 30 分钟做一次简单的回顾这个月通过 review 拦截到了哪些明显的问题大家在评审中有没有反复被同一个问题困扰有没有哪个模块的 owner 长期缺位这些问题都适合放进复盘的清单里而不是等到大家的耐心耗尽才来补。我个人在推动这套机制时最大的感触是Code Review 这件事技术含量其实没想象中那么高真正难的是让团队每个人都觉得我的意见被认真对待了以及这件事真的帮到我写更稳的代码了。一旦这两种感受建立起来评审就不再是需要推动的任务而是一种大家主动想要参与的习惯。