写代码这么多年我审过的MR/PR没有一千也有八百。但说句实话真正让我觉得有价值的审查可能不超过三成。剩下的多数时间我们都在做一件事给代码盖个我看了的章。代码审查这个词每个程序员都听过团队也都有流程但能把审查从形式过场变成质量杠杆的少之又少。这篇文章我不打算讲什么高深理论就结合我自己的评审经验和踩过的坑聊聊怎么把代码审查做出实效顺带把它嵌进团队的代码质量体系里。适合刚接手团队的技术负责人、想提升代码质量的资深工程师以及那些被LGTM文化折磨得够呛的一线开发者。我在不少团队见过同一个现象代码审查被写进了流程但从未真正融入开发。评论集中在这行太长了吧变量名再改改,而真正要命的并发隐患、边界漏判、错误处理缺失反而被漏过去。这背后的原因不单是态度问题更是一套方法论问题。把审查当成找茬游戏自然只会找到表面的茬把它当成对系统负责的联合设计你才能看见那些深层的坑。1. 代码审查为什么会沦为走过场1.1 形式化审查的典型病症我先描述几个场景你看看是不是似曾相识。第一个场景下午四点你收到一个PR提醒明天要上线。你打开diff650行改动横跨12个文件。你心想这次改动之前开会聊过架构上没什么大问题于是草草扫了两眼评论了一句整体没啥问题有几个命名我再想想然后点了Approve。第二个场景团队定了规矩所有PR必须至少一个人review通过才能合并。于是大家学聪明了谁写的代码就在群里一下平时比较好说话的同事对方十有八九回一个LGTM或者OK。审查时间不超过三分钟。代码质量如何没人说得清。第三个场景审查中确实有评论但永远集中在格式、命名、注释上。只要你把变量名改得足够规范提交就通过了。至于这里有没有数组越界、那个异常吞掉后会发生什么、为什么这个接口返回这么大的数据量没人关心。这三种情况我统称为假审查。假审查比不审查更危险——它给团队一种代码被看过了的心理安慰实际上质量漏洞一个没少只是从没被人发现变成了走流程漏掉。1.2 背后的机制问题激励错位与语境缺失为什么代码审查总会滑向形式化我觉得有三个关键原因。第一个原因是激励错位。大家默认上线更快是最高目标而审查是卡进度的环。审查者多评论、严格把关反而容易被视为难搞拖慢节奏。久而久之严格的人要么妥协要么被边缘化。代码审查没有成为质量防线反而成了社交压力测试。第二个原因是上下文缺失。审查者打开一个500行的diff但完全不了解这次改动背后的业务背景、技术选型讨论、之前踩过的坑。他只能针对眼前可见的代码做点状审查自然只看得见命名和格式。我见过最典型的例子是某次改动的核心风险在缓存一致性和事务边界上但因为PR描述写得含糊审查者把注意力全放在了新写的DTO有没有遵循项目的命名规范上。第三个原因是缺乏可操作的审查清单。多数团队没有定义代码审查到底看什么每个人全凭感觉。感觉型审查的后果是——团队里经验最丰富的人可能关注的是数据一致性初级工程师关注的是分号有没有漏结果谁也没覆盖到完整的风险面。1.3 代码审查真正应该解决什么问题我们把代码审查拉回原点它到底为了什么我的答案是四个层面。第一层面是发现缺陷——找到逻辑错误、边界遗漏、并发问题、安全隐患这是最基础的诉求。第二层面是传递知识——通过审查作者能学到更优的写法审查者也能理解业务上下文团队整体水位被拉高。第三层面是统一标准——一个团队如果没有评审很快会写出风格迥异、结构混乱的代码审查是维持代码规范最有效的手段。第四层面是架构守门——好的审查不只盯着每一行代码还会判断这个改动是否违背了模块边界、依赖方向、整体架构演进方向。你会发现后面三个层面的价值远大于第一个层面。只追求发现缺陷的团队会把审查变成找茬比赛而追求知识传递和架构守门的团队审查会成为一个隐形的导师和设计评审机制。搞清楚这个定位差异下面的所有实操方法才有意义。2. 审查维度的拆解从格式到架构的四个层次2.1 第一层规范与风格层我习惯把代码审查拆成四个层次审查时从低到高逐层扫描。规范层是最简单也最容易自动化的一层包括命名是否清晰、格式是否符合团队规范、是否有明显的死代码、注释是否与实现一致、是否残留调试代码或临时代码。这一层的问题不一定严重但它们一旦积累起来代码库的可读性会断崖式下跌。关于这一层我的建议是能用工具解决的不要用人工。ESLint、Prettier、golangci-lint、Checkstyle这些工具应该成为CI的第一道关卡。如果团队还在人工 review 缩进对不对引号单双,说明工具链建设落后了。审查者应该把精力留给机器做不了的事情。但规范层有个角落值得人工留意命名的语义准确性。工具能检查出命名是否符合驼峰,但检查不出这个变量名叫temp根本不知道它存的是什么。审查者看到temp、data、res、list这种模糊命名时值得停下来问一句——用3分钟把名字改清楚可能帮后人省下3小时的排查时间。2.2 第二层逻辑与正确性层第二层是我审查时最花力气的地方也是区分真审查和假审查的分水岭。逻辑层要重点看的东西包括边界条件有没有处理空数组、极端值、null/undefined、异常路径有没有覆盖网络超时、依赖服务不可用、并发写冲突、状态变更是否完整数据库事务、缓存更新顺序、消息队列的幂等性、条件判断是否反了、循环有没有可能死循环或越界、数值计算有没有溢出或精度问题。一个我经常提起的实战例子某次业务需要删除一批过期数据开发者的实现是先查出来再逐条删除。从单条逻辑看没什么问题但数据量在十万级时会发生两个问题——内存占用过高以及删除过程中如果有新插入的数据满足过期条件会被漏掉。这种问题只有审查者跳出单条语句正确的框架去推演这个操作在数据规模和环境变化下是否仍然正确才能被发现。审查时我会拿着调用链推演这个函数被谁调用入参取值范围是什么当前改动是否破坏了上游的假设尤其是公共函数、接口层、异步回调这三处最容易出现局部正确、全局错误的情况。2.3 第三层设计与可维护性层第三层关注的是代码在未来的生命力。我不只看这段代码改完能不能跑还会看它半年后、一年后当需求变化时改起来是容易还是痛苦。设计层的问题通常表现为一个函数或类承担了太多职责、模块间出现了本可避免的耦合、扩展点是否被硬编码抹平、重复代码是否应该被抽取、接口设计是否对调用方足够友好。举个例子我review过一段代码业务方需要拿到用户信息列表开发者在Service层写了一个方法返回ListUser但调用方还需要拿到总数做分页于是又加了一个方法。两个方法内部逻辑几乎一样只是返回类型不同。这种代码从今天的视角看没错但明天多一个字段、多一个排序维度两个方法要同步改改漏一个就是bug。审查者的价值在于对这种将来必然变化的地方敏锐一些。我不是建议所有代码都过度抽象。在多数业务代码里比起设计优雅我更重视结构直白。可维护性最高的代码是那种新人也能顺着读下去的代码而不是用了五个设计模式、看半天看不懂的代码。审查设计层时我会反复问自己这个改动让系统变量更复杂了还是更简单了如果答案是变复杂了但未带来对应收益,那我会要求作者简化。2.4 第四层性能与安全层第四层不是每次都需要从头到尾扫但凡是改动涉及热点链路、大数据量处理、用户敏感信息、对外暴露接口就必须专门过一遍。性能方面重点看是否在循环里做了数据库查询或远程调用N1问题、是否加载了比实际需求多得多的数据、是否存在无意义的重复计算、缓存策略是否合理缓存粒度、过期时间、一致性处理。安全方面重点看输入校验是否充分、有没有拼接式SQL、有没有存储型/反射型XSS风险、权限校验是在前端还是后端、敏感信息是否被序列化到日志或响应体里、第三方依赖是否有已知漏洞。举一个我在审查中逮到过的典型问题一个管理后台的导出接口拼接了用户传入的排序字段直接拼到SQL的 ORDER BY 后面。单看这个接口功能正常但等我知道这个管理后台是在内网部署、权限控制还宽松时这就成了可能被SQL注入的入口。审查者的任务之一就是在代码里嗅出这类看似正常但放大了攻击面的写法。3. 一份可复用的代码审查清单3.1 从审查维度到检查条目四个层次讲完你可能觉得这么多每次审查都要过一遍吗当然不是。一个人脑容量有限每次硬记八个维度容易漏。我建议团队把四个层次拆成一份结构化清单贴在PR模板里审查者照着走一遍。以下是我在团队里推行过的审查清单表格你可以直接抄过去按需调整层次检查点典型问题举例规范层命名语义清晰变量名是不是temp/data/res规范层死代码与调试残留console.log、注释掉的代码逻辑层边界与空值处理空集合、最大长度、数值0值逻辑层异常路径catch后是否静默吞错逻辑层并发与状态一致性竞态条件、缓存与DB不一致逻辑层幂等性重复调用是否产生副作用设计层单一职责函数是否在做三件事设计层模块边界是否跨层直接访问设计层扩展性硬编码是否阻碍了合理扩展设计层重复代码逻辑复制后渐渐分叉性能层N1查询循环里查库/调远程性能层大对象传输返回全字段而调用方只要两个字段安全层输入校验用户输入未清洗直接使用安全层敏感信息日志里打印了手机号/身份证落地时有个技巧把清单作为PR描述模板的一部分要求作者提交PR时逐项自检审查者在此基础上复查。这能有效降低审查者的记忆负担也让作者提交前自己过一遍问题。3.2 按变更规模调整审查策略清单是通用的但不同规模的diff审查深度和方式应该有明显差异。小变更比如100行以内的改动我的建议是逐行审查重点关注逻辑正确性和API兼容性。这类变更往往是修bug或小功能风险集中一次看仔细能避免后续扯皮。中等变更100到400行适合先看整体结构再深入逻辑。我会先看PR描述、文件列表、关键类图然后按调用链从入口开始往下走。中间层代码可以扫读但核心业务逻辑要逐步推演。大变更超过400行或者波及多个模块靠人肉逐行审查既不现实也不可靠。我的做法是先要求作者拆分PR。一个能拆的PR说明原来的改动粒度就太大了。如果因为业务原因确实拆不开那就按风险优先级审查——先看公共基础类、核心数据结构的改动再看高风险模块支付、权限、数据一致性最稳妥的办法是拉上架构师一起过设计。注意我见过太多团队把几百上千行的大PR直接合并理由是每个人都在场review过了。实际上人脑对长diff的注意力衰减极快超过400行的逐行审查基本只剩形式意义。拆分PR不是流程教条是对审查质量的基本保障。4. 评审意见的表达让建议能被接受4.1 用分级表达替代模糊反馈技术能力到位了表达跟不上审查依然会翻车。我在早期吃过亏评语写得犀利一句话把人家代码贬得一文不值结果对方直接摆烂该改的问题也没改。后来我琢磨出一套分级表达法效果很不错。我习惯把评审意见分成四档Nit吹毛求疵例如多了一个空格、命名可以更好。这类意见不阻塞合并作者有空再改。Suggestion建议另一种写法或者当前实现没问题但有优化的空间。给出具体替代方案不强制。Should应当当前实现有隐患但不修复不会立即出事比如将来扩展困难、潜在边界问题。这类意见应当修复后再合并但如果有时间压力可以明确讨论后再决定。Blocking必须修明确的功能错误、安全漏洞、数据一致性破坏或者明确违背了架构原则。不修不能合并。我会在每条评论前面带上这个分级标签。没有标签的评论作者容易产生每条都要改的心理压力结果重要的被稀释次要的被放大。4.2 陈述事实而非攻击作者同样一个问题两种说法带来的效果完全不同。错误的说法你这写的什么垃圾循环里查库优化一下。我的推荐说法这个循环里每次都在查用户表数据量上来后查询次数会很多。建议先批量查出来生成Map再在循环里取这样只需要一次数据库查询。这里的 UserMapper 可以直接提供一个批量查询方法。看出区别了吗第二种说法包含了三部分具体的问题场景、问题可能导致的后果、可落地的改进方向。评论针对的是代码不是人。作者看完之后知道这里要改、为什么改、怎么改而不是我水平不行。4.3 分歧处理优先结论次优方案先落地审查中一定会遇到分歧尤其是设计取舍上。A认为用组合优于继承B认为当前继承层级已经够清晰了没有必要改动。我的处理原则是如果双方都拿得出站得住脚的理由且短期内无法验证哪个更好那就先让作者按自己的方案落地然后开一个技术债issue记录下来注明此处的方案取舍待验证后续若出现XX问题需要重构。这既保证了PR不被无限期卡住也保留了后续优化的线索。另一个常见分歧是代码风格。碰到这种情况别跟你旁边的人争论打开团队规范文档没写明的就补充进去。一旦标准形成以后就按标准讨论不再按个人审美讨论。注意代码审查中最容易被低估的工具是引用事实。当你说这个接口本来就是幂等的你这样改破坏了约定比我觉得这里应该幂等有说服力得多。平时养成读源码注释和接口文档的习惯评审时等于带了一本法律书。5. 从个人审查到团队代码质量体系的落地5.1 把审查嵌进开发流程的节奏个人再会审查如果团队流程不配合也白搭。我分享几个经过验证的团队级落地方法。首先是控制PR规模。团队约定单个PR不超过400行超过的必须拆。这条规则可以配合CI检查diff行数超限就挂起合并。前两周大家会觉得麻烦但坚持一个月后效果立竿见影——不只是审查质量提高了连代码出bug的概率都明显下降。原因很简单变更越小影响面越可控逻辑越容易被看清。其次是固定评审响应时间。团队约定在工作时间内评审者的首轮响应不超过4小时。这里强调首轮响应而不是全部解决——先让作者知道有人在看了具体的完整review可以再花时间。千万不要让作者提交PR后石沉大海那种等待最耗士气。第三是建立轮值评审制度。没有固定的御用审批人每个工程师轮流承担评审责任。新人在前几次评审时由资深工程师带边评边讲思路。一个团队里大家都会review知识才能流动起来代码风格也能互相渗透。5.2 工具链与自动化的配合人工审查应该站在自动化的肩膀上看代码而不是孤军奋战。我建议在CI流水线里至少铺三层自动化关卡。第一层是静态检查包括代码风格检查、重复代码检测、简单规则引擎比如禁止在循环里调用远程接口这样的自定义规则。第二层是自动化测试包括单测、集成测试和关键链路的契约测试。任何PR必须通过全部测试才能进入人工评审环节。第三层是增量覆盖率统计不要求整体覆盖率100%但新代码的覆盖率至少应该达到80%以上。覆盖率低于阈值的PR人工评审时要特别关注未被测试覆盖的路径。有了这三层兜底人工审查才真正可以聚焦到机器干不了的事情上业务正确性推演、架构一致性判断、安全隐患嗅觉、代码可维护性感受。5.3 技术债台账与审查反馈闭环审查中发现但暂时未修复的问题如果只是一句先记一下后续大概率就丢了。我在团队里会维护一份技术债台账每个未修复的评审意见登记为一张issue包含位置、问题描述、影响评估、提出日期和认领人。每周技术评审会上过一遍台账挑出优先级最高的一两个在当前迭代中安排修复。这会带来一个很好的副产品作者知道这里的问题不会被遗忘审查者也知道提意见不是白提。审查的真正闭环不是合并PR而是问题被跟踪、被解决、最终沉淀成经验。从人治到制度代码审查才真正嵌入了团队的质量体系。5.4 常见问题速查表我把这几年review中反复出现的典型问题整理成一张速查表每次拿不准时翻一下很有帮助现象可能原因建议处理方向循环内查数据库未意识到数据量增长改批量查询或缓存结果异常被catch后吞掉不想让接口报错至少记录日志或者上抛到合适层两个方法长得一模一样快来粘贴抽取公共方法注意差异参数缓存更新写在删除之后对一致性顺序没概念先更新缓存再发消息或先删缓存再延迟重填一个Controller写了两百行业务贫血模型Service层形同虚设把业务逻辑下沉到Service/Domain所有字段都返回给前端图省事增加VO按需返回防止泄露内部字段幂等控制缺失没意识到重复请求关键写操作上幂等号用字符串拼接SQL图方便改为参数化查询加了一堆参数但没人用预留可能性删除死参数等真正需要时再加5.5 我个人的一些体会代码审查做到最后我最大的体会是它考验的不是技术深度而是协作修养。一个真正优秀的reviewer不是能挑出最多问题的人而是能让作者在修改中感受到成长的人。我评审时经常把话说得很直白你这个命名我看了三遍没明白假如三个月后回来改这段代码的是你你想搜什么关键词能找到它这种问法常常比命名要清晰有用得多——它让作者站到未来的自己面前想问题。给刚起步的团队一个建议不要一上来就推大而全的审查流程。先从固定PR描述模板和四层清单入手让大家在review时有个抓手。等习惯养成之后再补工具链、再提响应时间、再设计轮值制度。我见过太多团队流程设计的比执行力还超前最后制度变成文档没人照做。好的代码审查体系是长出来的不是设计出来的。我已经把能踩的坑都写在前面了。如果你所在团队还在假审查的状态挑一两个方法先试起来。代码质量是一场持久战而审查是我们手里最便宜的武器。
如何把代码审查从形式过场变成质量杠杆？四层清单与团队落地实践
写代码这么多年我审过的MR/PR没有一千也有八百。但说句实话真正让我觉得有价值的审查可能不超过三成。剩下的多数时间我们都在做一件事给代码盖个我看了的章。代码审查这个词每个程序员都听过团队也都有流程但能把审查从形式过场变成质量杠杆的少之又少。这篇文章我不打算讲什么高深理论就结合我自己的评审经验和踩过的坑聊聊怎么把代码审查做出实效顺带把它嵌进团队的代码质量体系里。适合刚接手团队的技术负责人、想提升代码质量的资深工程师以及那些被LGTM文化折磨得够呛的一线开发者。我在不少团队见过同一个现象代码审查被写进了流程但从未真正融入开发。评论集中在这行太长了吧变量名再改改,而真正要命的并发隐患、边界漏判、错误处理缺失反而被漏过去。这背后的原因不单是态度问题更是一套方法论问题。把审查当成找茬游戏自然只会找到表面的茬把它当成对系统负责的联合设计你才能看见那些深层的坑。1. 代码审查为什么会沦为走过场1.1 形式化审查的典型病症我先描述几个场景你看看是不是似曾相识。第一个场景下午四点你收到一个PR提醒明天要上线。你打开diff650行改动横跨12个文件。你心想这次改动之前开会聊过架构上没什么大问题于是草草扫了两眼评论了一句整体没啥问题有几个命名我再想想然后点了Approve。第二个场景团队定了规矩所有PR必须至少一个人review通过才能合并。于是大家学聪明了谁写的代码就在群里一下平时比较好说话的同事对方十有八九回一个LGTM或者OK。审查时间不超过三分钟。代码质量如何没人说得清。第三个场景审查中确实有评论但永远集中在格式、命名、注释上。只要你把变量名改得足够规范提交就通过了。至于这里有没有数组越界、那个异常吞掉后会发生什么、为什么这个接口返回这么大的数据量没人关心。这三种情况我统称为假审查。假审查比不审查更危险——它给团队一种代码被看过了的心理安慰实际上质量漏洞一个没少只是从没被人发现变成了走流程漏掉。1.2 背后的机制问题激励错位与语境缺失为什么代码审查总会滑向形式化我觉得有三个关键原因。第一个原因是激励错位。大家默认上线更快是最高目标而审查是卡进度的环。审查者多评论、严格把关反而容易被视为难搞拖慢节奏。久而久之严格的人要么妥协要么被边缘化。代码审查没有成为质量防线反而成了社交压力测试。第二个原因是上下文缺失。审查者打开一个500行的diff但完全不了解这次改动背后的业务背景、技术选型讨论、之前踩过的坑。他只能针对眼前可见的代码做点状审查自然只看得见命名和格式。我见过最典型的例子是某次改动的核心风险在缓存一致性和事务边界上但因为PR描述写得含糊审查者把注意力全放在了新写的DTO有没有遵循项目的命名规范上。第三个原因是缺乏可操作的审查清单。多数团队没有定义代码审查到底看什么每个人全凭感觉。感觉型审查的后果是——团队里经验最丰富的人可能关注的是数据一致性初级工程师关注的是分号有没有漏结果谁也没覆盖到完整的风险面。1.3 代码审查真正应该解决什么问题我们把代码审查拉回原点它到底为了什么我的答案是四个层面。第一层面是发现缺陷——找到逻辑错误、边界遗漏、并发问题、安全隐患这是最基础的诉求。第二层面是传递知识——通过审查作者能学到更优的写法审查者也能理解业务上下文团队整体水位被拉高。第三层面是统一标准——一个团队如果没有评审很快会写出风格迥异、结构混乱的代码审查是维持代码规范最有效的手段。第四层面是架构守门——好的审查不只盯着每一行代码还会判断这个改动是否违背了模块边界、依赖方向、整体架构演进方向。你会发现后面三个层面的价值远大于第一个层面。只追求发现缺陷的团队会把审查变成找茬比赛而追求知识传递和架构守门的团队审查会成为一个隐形的导师和设计评审机制。搞清楚这个定位差异下面的所有实操方法才有意义。2. 审查维度的拆解从格式到架构的四个层次2.1 第一层规范与风格层我习惯把代码审查拆成四个层次审查时从低到高逐层扫描。规范层是最简单也最容易自动化的一层包括命名是否清晰、格式是否符合团队规范、是否有明显的死代码、注释是否与实现一致、是否残留调试代码或临时代码。这一层的问题不一定严重但它们一旦积累起来代码库的可读性会断崖式下跌。关于这一层我的建议是能用工具解决的不要用人工。ESLint、Prettier、golangci-lint、Checkstyle这些工具应该成为CI的第一道关卡。如果团队还在人工 review 缩进对不对引号单双,说明工具链建设落后了。审查者应该把精力留给机器做不了的事情。但规范层有个角落值得人工留意命名的语义准确性。工具能检查出命名是否符合驼峰,但检查不出这个变量名叫temp根本不知道它存的是什么。审查者看到temp、data、res、list这种模糊命名时值得停下来问一句——用3分钟把名字改清楚可能帮后人省下3小时的排查时间。2.2 第二层逻辑与正确性层第二层是我审查时最花力气的地方也是区分真审查和假审查的分水岭。逻辑层要重点看的东西包括边界条件有没有处理空数组、极端值、null/undefined、异常路径有没有覆盖网络超时、依赖服务不可用、并发写冲突、状态变更是否完整数据库事务、缓存更新顺序、消息队列的幂等性、条件判断是否反了、循环有没有可能死循环或越界、数值计算有没有溢出或精度问题。一个我经常提起的实战例子某次业务需要删除一批过期数据开发者的实现是先查出来再逐条删除。从单条逻辑看没什么问题但数据量在十万级时会发生两个问题——内存占用过高以及删除过程中如果有新插入的数据满足过期条件会被漏掉。这种问题只有审查者跳出单条语句正确的框架去推演这个操作在数据规模和环境变化下是否仍然正确才能被发现。审查时我会拿着调用链推演这个函数被谁调用入参取值范围是什么当前改动是否破坏了上游的假设尤其是公共函数、接口层、异步回调这三处最容易出现局部正确、全局错误的情况。2.3 第三层设计与可维护性层第三层关注的是代码在未来的生命力。我不只看这段代码改完能不能跑还会看它半年后、一年后当需求变化时改起来是容易还是痛苦。设计层的问题通常表现为一个函数或类承担了太多职责、模块间出现了本可避免的耦合、扩展点是否被硬编码抹平、重复代码是否应该被抽取、接口设计是否对调用方足够友好。举个例子我review过一段代码业务方需要拿到用户信息列表开发者在Service层写了一个方法返回ListUser但调用方还需要拿到总数做分页于是又加了一个方法。两个方法内部逻辑几乎一样只是返回类型不同。这种代码从今天的视角看没错但明天多一个字段、多一个排序维度两个方法要同步改改漏一个就是bug。审查者的价值在于对这种将来必然变化的地方敏锐一些。我不是建议所有代码都过度抽象。在多数业务代码里比起设计优雅我更重视结构直白。可维护性最高的代码是那种新人也能顺着读下去的代码而不是用了五个设计模式、看半天看不懂的代码。审查设计层时我会反复问自己这个改动让系统变量更复杂了还是更简单了如果答案是变复杂了但未带来对应收益,那我会要求作者简化。2.4 第四层性能与安全层第四层不是每次都需要从头到尾扫但凡是改动涉及热点链路、大数据量处理、用户敏感信息、对外暴露接口就必须专门过一遍。性能方面重点看是否在循环里做了数据库查询或远程调用N1问题、是否加载了比实际需求多得多的数据、是否存在无意义的重复计算、缓存策略是否合理缓存粒度、过期时间、一致性处理。安全方面重点看输入校验是否充分、有没有拼接式SQL、有没有存储型/反射型XSS风险、权限校验是在前端还是后端、敏感信息是否被序列化到日志或响应体里、第三方依赖是否有已知漏洞。举一个我在审查中逮到过的典型问题一个管理后台的导出接口拼接了用户传入的排序字段直接拼到SQL的 ORDER BY 后面。单看这个接口功能正常但等我知道这个管理后台是在内网部署、权限控制还宽松时这就成了可能被SQL注入的入口。审查者的任务之一就是在代码里嗅出这类看似正常但放大了攻击面的写法。3. 一份可复用的代码审查清单3.1 从审查维度到检查条目四个层次讲完你可能觉得这么多每次审查都要过一遍吗当然不是。一个人脑容量有限每次硬记八个维度容易漏。我建议团队把四个层次拆成一份结构化清单贴在PR模板里审查者照着走一遍。以下是我在团队里推行过的审查清单表格你可以直接抄过去按需调整层次检查点典型问题举例规范层命名语义清晰变量名是不是temp/data/res规范层死代码与调试残留console.log、注释掉的代码逻辑层边界与空值处理空集合、最大长度、数值0值逻辑层异常路径catch后是否静默吞错逻辑层并发与状态一致性竞态条件、缓存与DB不一致逻辑层幂等性重复调用是否产生副作用设计层单一职责函数是否在做三件事设计层模块边界是否跨层直接访问设计层扩展性硬编码是否阻碍了合理扩展设计层重复代码逻辑复制后渐渐分叉性能层N1查询循环里查库/调远程性能层大对象传输返回全字段而调用方只要两个字段安全层输入校验用户输入未清洗直接使用安全层敏感信息日志里打印了手机号/身份证落地时有个技巧把清单作为PR描述模板的一部分要求作者提交PR时逐项自检审查者在此基础上复查。这能有效降低审查者的记忆负担也让作者提交前自己过一遍问题。3.2 按变更规模调整审查策略清单是通用的但不同规模的diff审查深度和方式应该有明显差异。小变更比如100行以内的改动我的建议是逐行审查重点关注逻辑正确性和API兼容性。这类变更往往是修bug或小功能风险集中一次看仔细能避免后续扯皮。中等变更100到400行适合先看整体结构再深入逻辑。我会先看PR描述、文件列表、关键类图然后按调用链从入口开始往下走。中间层代码可以扫读但核心业务逻辑要逐步推演。大变更超过400行或者波及多个模块靠人肉逐行审查既不现实也不可靠。我的做法是先要求作者拆分PR。一个能拆的PR说明原来的改动粒度就太大了。如果因为业务原因确实拆不开那就按风险优先级审查——先看公共基础类、核心数据结构的改动再看高风险模块支付、权限、数据一致性最稳妥的办法是拉上架构师一起过设计。注意我见过太多团队把几百上千行的大PR直接合并理由是每个人都在场review过了。实际上人脑对长diff的注意力衰减极快超过400行的逐行审查基本只剩形式意义。拆分PR不是流程教条是对审查质量的基本保障。4. 评审意见的表达让建议能被接受4.1 用分级表达替代模糊反馈技术能力到位了表达跟不上审查依然会翻车。我在早期吃过亏评语写得犀利一句话把人家代码贬得一文不值结果对方直接摆烂该改的问题也没改。后来我琢磨出一套分级表达法效果很不错。我习惯把评审意见分成四档Nit吹毛求疵例如多了一个空格、命名可以更好。这类意见不阻塞合并作者有空再改。Suggestion建议另一种写法或者当前实现没问题但有优化的空间。给出具体替代方案不强制。Should应当当前实现有隐患但不修复不会立即出事比如将来扩展困难、潜在边界问题。这类意见应当修复后再合并但如果有时间压力可以明确讨论后再决定。Blocking必须修明确的功能错误、安全漏洞、数据一致性破坏或者明确违背了架构原则。不修不能合并。我会在每条评论前面带上这个分级标签。没有标签的评论作者容易产生每条都要改的心理压力结果重要的被稀释次要的被放大。4.2 陈述事实而非攻击作者同样一个问题两种说法带来的效果完全不同。错误的说法你这写的什么垃圾循环里查库优化一下。我的推荐说法这个循环里每次都在查用户表数据量上来后查询次数会很多。建议先批量查出来生成Map再在循环里取这样只需要一次数据库查询。这里的 UserMapper 可以直接提供一个批量查询方法。看出区别了吗第二种说法包含了三部分具体的问题场景、问题可能导致的后果、可落地的改进方向。评论针对的是代码不是人。作者看完之后知道这里要改、为什么改、怎么改而不是我水平不行。4.3 分歧处理优先结论次优方案先落地审查中一定会遇到分歧尤其是设计取舍上。A认为用组合优于继承B认为当前继承层级已经够清晰了没有必要改动。我的处理原则是如果双方都拿得出站得住脚的理由且短期内无法验证哪个更好那就先让作者按自己的方案落地然后开一个技术债issue记录下来注明此处的方案取舍待验证后续若出现XX问题需要重构。这既保证了PR不被无限期卡住也保留了后续优化的线索。另一个常见分歧是代码风格。碰到这种情况别跟你旁边的人争论打开团队规范文档没写明的就补充进去。一旦标准形成以后就按标准讨论不再按个人审美讨论。注意代码审查中最容易被低估的工具是引用事实。当你说这个接口本来就是幂等的你这样改破坏了约定比我觉得这里应该幂等有说服力得多。平时养成读源码注释和接口文档的习惯评审时等于带了一本法律书。5. 从个人审查到团队代码质量体系的落地5.1 把审查嵌进开发流程的节奏个人再会审查如果团队流程不配合也白搭。我分享几个经过验证的团队级落地方法。首先是控制PR规模。团队约定单个PR不超过400行超过的必须拆。这条规则可以配合CI检查diff行数超限就挂起合并。前两周大家会觉得麻烦但坚持一个月后效果立竿见影——不只是审查质量提高了连代码出bug的概率都明显下降。原因很简单变更越小影响面越可控逻辑越容易被看清。其次是固定评审响应时间。团队约定在工作时间内评审者的首轮响应不超过4小时。这里强调首轮响应而不是全部解决——先让作者知道有人在看了具体的完整review可以再花时间。千万不要让作者提交PR后石沉大海那种等待最耗士气。第三是建立轮值评审制度。没有固定的御用审批人每个工程师轮流承担评审责任。新人在前几次评审时由资深工程师带边评边讲思路。一个团队里大家都会review知识才能流动起来代码风格也能互相渗透。5.2 工具链与自动化的配合人工审查应该站在自动化的肩膀上看代码而不是孤军奋战。我建议在CI流水线里至少铺三层自动化关卡。第一层是静态检查包括代码风格检查、重复代码检测、简单规则引擎比如禁止在循环里调用远程接口这样的自定义规则。第二层是自动化测试包括单测、集成测试和关键链路的契约测试。任何PR必须通过全部测试才能进入人工评审环节。第三层是增量覆盖率统计不要求整体覆盖率100%但新代码的覆盖率至少应该达到80%以上。覆盖率低于阈值的PR人工评审时要特别关注未被测试覆盖的路径。有了这三层兜底人工审查才真正可以聚焦到机器干不了的事情上业务正确性推演、架构一致性判断、安全隐患嗅觉、代码可维护性感受。5.3 技术债台账与审查反馈闭环审查中发现但暂时未修复的问题如果只是一句先记一下后续大概率就丢了。我在团队里会维护一份技术债台账每个未修复的评审意见登记为一张issue包含位置、问题描述、影响评估、提出日期和认领人。每周技术评审会上过一遍台账挑出优先级最高的一两个在当前迭代中安排修复。这会带来一个很好的副产品作者知道这里的问题不会被遗忘审查者也知道提意见不是白提。审查的真正闭环不是合并PR而是问题被跟踪、被解决、最终沉淀成经验。从人治到制度代码审查才真正嵌入了团队的质量体系。5.4 常见问题速查表我把这几年review中反复出现的典型问题整理成一张速查表每次拿不准时翻一下很有帮助现象可能原因建议处理方向循环内查数据库未意识到数据量增长改批量查询或缓存结果异常被catch后吞掉不想让接口报错至少记录日志或者上抛到合适层两个方法长得一模一样快来粘贴抽取公共方法注意差异参数缓存更新写在删除之后对一致性顺序没概念先更新缓存再发消息或先删缓存再延迟重填一个Controller写了两百行业务贫血模型Service层形同虚设把业务逻辑下沉到Service/Domain所有字段都返回给前端图省事增加VO按需返回防止泄露内部字段幂等控制缺失没意识到重复请求关键写操作上幂等号用字符串拼接SQL图方便改为参数化查询加了一堆参数但没人用预留可能性删除死参数等真正需要时再加5.5 我个人的一些体会代码审查做到最后我最大的体会是它考验的不是技术深度而是协作修养。一个真正优秀的reviewer不是能挑出最多问题的人而是能让作者在修改中感受到成长的人。我评审时经常把话说得很直白你这个命名我看了三遍没明白假如三个月后回来改这段代码的是你你想搜什么关键词能找到它这种问法常常比命名要清晰有用得多——它让作者站到未来的自己面前想问题。给刚起步的团队一个建议不要一上来就推大而全的审查流程。先从固定PR描述模板和四层清单入手让大家在review时有个抓手。等习惯养成之后再补工具链、再提响应时间、再设计轮值制度。我见过太多团队流程设计的比执行力还超前最后制度变成文档没人照做。好的代码审查体系是长出来的不是设计出来的。我已经把能踩的坑都写在前面了。如果你所在团队还在假审查的状态挑一两个方法先试起来。代码质量是一场持久战而审查是我们手里最便宜的武器。
