谷歌 Code Review 指南(阅读简化版本)

Posted by luyi on October 29, 2019

本文主要参考学习 google 开源的 CR 实践指导。 原文有些地方太冗长,因此做了些精简,但未破坏整体结构,如果您碰巧看到,希望对您也有用;

Code Review工作:分为评审人( Code Reviewer )指导和提交者(CL Author);

评审人需要关注(看哪些东西)

1. 看哪些内容

• Design 设计是否合理;

• Functionality 符合作者的设计目标,功能是否对用户友好; 1)预先测试过; 2)评审些边缘场景,并发问题等; 3)影响UI的代码,如有必要可以加demo演示; 4)尤其注意并发控制的代码评审,因为难以测试保障;

• Complexity 代码是否易于理解,方便以后新人维护。 1)意味着难以理解,和容易以后引入bugs; 2)典型是“过度工程化”;

• Tests 代码逻辑是否正确,且有设计良好的自动化测试用例; 1)测试代码也是代码,同样要简单设计; 2)有断言

• Naming 变量,类,方法等命名是否合理;

• Comments 注释是否清晰且有用; 1)注释说明不应该是“解释代码做了什么”,而应该说明“这块代码为什么存在”,代码不能自解释,你就要尽量让它变得简单易懂; 2)但又不同于 class,modules,或者functions的“文档”,文档可以说明“做了什么”,“会发生什么行为”;

• Style 代码样式;

• Documentation 是否同步更新了相关的文档;(readme,使用文档等)

• 每一行&上下文; 1)大部代码你需要一行行阅读; 2)有时需要看出来变化部分的整体,获得全局的认识;

除了上面的几个层面,具体的资料推荐:《阿里巴巴Java开发手册》

2.如何写 review 的评论

• 礼貌。 (评论只针对代码不对人;)

• 解释你的理由。

• 给出明确修改方向 和 让开发者决策 保持平衡;(代码作者更接近代码;)

• 鼓励开发增加评论以及简化代码,而不是尝试给你解释复杂性;(如果对一段代码不理解,尽量也不要让开发口头解释或在评论里回复解释)

3. 开发者对 review 问题不认可

• 谁对? 多沟通,开发拥有更多需求和代码上下文,评审者旁观者清,但目标是提高代码质量

• 怕得罪开发者。 如果结果好的,一般不会,但也要注意表达方式

• 推迟处理问题; review的问题尽量立刻修复处理。 如果紧急情况,可以先通过,但必须单独再补一次CL修复问题。问题在于一般后续补CL的概率不高,所以必须同意通过前,让开发者记个issue,同时最好代码注释里加 ”TODO“

• 对review过于严格的抱怨; 1) 评审同学要加快每次review的速度,不要使得整个过程过于冗长。 2) 长久之后会发现严格的价值和好处。 3) 发现价值后一般当初反弹最大的同学,反而容易成为最忠实的卫道夫)

• 解决冲突; 1)需要达成共识; 2)可以进一步当面沟通; 3)可以引入第三者加入仲裁; 4)最后上升引入更高或更广泛地讨论)

4. Code Review的标准

cr目标: 确保持续地提高代码库的代码健康度(质量)。 有几个点需要平衡:

• 开发者,必须能持续的做出改进。 开发者无法做出改进,或者CR门槛过高,开发难以贡献改进,都无法达持续提高的目标;

• 评审者,必须确保评审通过的CL确实有利于改进代码质量,这个同样很难,因为很多”紧急需求“的容易打破计划; 因此最重要的规范: 评审者必须确保允许的CL 是确定可以既有系统的整体代码质量的,即使这个CL可能不完美;

• 即使一个良好设计的代码,但如果不适合加入既有系统,评审这可以拒绝; 指导: 1)评审过程中可以分享一些知识(语言特性,工具和框架等); 2)看到最佳实践记得分享; 看到典型缺陷记得记录,让更多人学习到!

5. CL如何评审的指南

• 第一步,总览变化 CL 是否讲得通有意义?描述是否清楚?

• 第二步,检查CL的重点部分 1)识别出重点部分,比如代码变化最大的文件,如果变化量太大,建议开发者把1个CL分为多个CLs; 2)识别重点部分是重要的,一般重点部分设计有问题,其余代码上浪费时间就没太大意义;而且有可能开发者后续CL代码会基于重点部分,继续写很多代码; 另外大的设计变更相比小的变化往往需要更长时间

• 第三步, 完整地按顺序评审其余的代码 如果确定核心变更没问题,就可以按顺序走查所有代码。另外建议可以先阅读测试用例,你可能会知道开发者做某些变更的目的。

6. 评审速度

个人独立的研发速度,没有能够提供团队速度重要。如果代码评审太慢的坏处: 1)团队速度下降; 2)开发者会开始抗拒代码评审(大部分抱怨是可以通过加快评审速度解决); 3)影响代码质量,可能会抑制提patch,持续好的改进的发生;

• 那评审者应该多快呢? 没有重要手头工作,最好立刻做掉。否则建议在一个工作日(非自然日)搞定。

• 速度 vs 中断 如果你正在写代码,或需要集中精力的联系工作,不建议中断去做评审,可以等到阶段间歇来做。

• 快速响应 如果你确实没有大块时间来做,还是尽量先给出个阶段反馈,让开发知道你在处理了,或者你会逐步处理;

• 跨时区评审 尽量在开发者还在办公的时间,如果他们回家了,尽量在第二天上班前完成评审;

• 评论 LGTM 有几种情况你应该给出允许(LGTM),即使还有待修复评论: 1)评审者确信开发者已经很好记录了剩下的评论问题; 2)剩下的问题不是很重要,开发者不是非得做。

• 大 CLs 尽量拆成多个小的CLs; 如果很难做到,至少要对整体设计给出评审意见;在不牺牲代码健康的前提下,评审人要确保不block开发者做进一步工作;

• 代码评审是逐步改进的过程 整个reivew过程多经历几次,慢慢开发者和评审者就会形成共识,整体质量和速度都会逐步提高; 长期来看,任何短期的再标准和质量上的妥协都是有害的;

• 紧急情况 线上问题修复,处理紧迫的法律问题安全问题修复,可以跳过整个流程;

代码提交者需要关注

1. 写个好的 CL 描述

CL描述: 做了什么变化,为什么做这个变化;这将是存在版本控制的历史记录中,被很多后人反复查看。 以后开发者可能会通过描述来搜 CL,没有描述对应定位一个CL是困难的。

• 首行 1)做聊什么的简单概要 2)完整句子,祈使句式 3)紧跟着空一行 例如:”Delete the FizzBuzz RPC and replace it with the new system.” 而不是 “Deleting the FizzBuzz RPC and replacing it with the new system.”

• “内容部分”应该提供有用信息的 应该包括: 修复的问题的简要描述,为什么这是个最佳方案,如果有其他捷径应该同样提及。 其他相关信息包括: 背景信息,比如 bug号,性能测试结果,设计文档的链接;

• 糟糕 的 CL 描述 “Fix bug”是个不充分的描述。 什么bug?你做了修复它?其他相似的描述包括:“Fix build”,”add convenience functions”.

• 好的 CL 描述

• 在提交CL前,再检查下对应的描述 在审核期间,CL可能会发生重大变化。 在提交CL之前,有必要检查一下CL描述,以确保该描述仍能反映CL所做的工作。

2. 小 CLs

• 为什么写小 CLs? 1)更快 2)更聚焦和完全 3)更不容易引入bugs
4)如果被驳回,影响小,更少无效工作 5)更容易合并 (合并冲突) 6) 小的变更,更容易设计好
7)对评审者更少的阻塞
8)更容易回滚

• 什么是“小” 总的来说,是一个自包含的变更。 意味着: 1)最小更改仅解决一件事; 2)评审者需要知道的关于该CL的所有信息; 3)CL 并入后这个CL后,这个系统继续运行正常; 4)难以理解其含义,也是不行的 (比如加个api,没有使用说明和看到,很可能是无用的,难以判断);

• 什么时候大的 CLs 是ok的 1)可以把“整个文件”删除类似变更,视为一行变更对待,不会让费评审者较多时间; 2)自动化构建工具产生的; 尽量拆分:小的可以自包的变更,分配给不同的评审者,(比如一部分代码,一部分配置;或者一部分序列化处理部分,另外一部是使用序列化的逻辑)

• 单独的重构 尽量把重构相关的工作(比如变量名,类结构调整等)单独CL来做,不要混在“features” 或“bug fixs”中。

• 保持相关测试用例关小对应的小 CLs 不要加测试代码和代码分离开。尽量一个CL 做到变更有对应测试的覆盖;

• 不要中断构建过程; 如果你有多个CLs相互依赖,你要找到个方式使得任何 CL 提交后不影响整个系统,否则造成影响或中断团队其他同学的构建。

• 不能再更小的程度 尽量去思考如何把功能解构为一系列小的变更,可以咨询团队成员给出建议。因为无法拆分的大变更,一般很少是正确的! 如果都无法做到,那么和评审者沟通,审查过程要保持引入错误的警惕,尽量写更详细的测试用例来保证;

3. 如何处理评审者的意见

• 对事不对人 Review目的在于提高代码库和产品质量。 需要注意对于评论意见不要怒气相向,建议离开电脑一会儿,等冷静了再回复。保持建设性和礼貌。

• 修复代码 如果评审者反映你的代码有些地方不好理解,首先要做的是把代码调整清楚,然后代码不能更清晰,尝试加上相关注释。如果代码注释还是不行,那么就在 codereview 工具上评论回复。 为什么这个顺序,因为评审者看不懂,很大可能以后其他人也看不懂,所以不要上来就口头或者在 cr 工具上解释就完事了。

• 自己思考 不要盲目“自我感觉良好”,心理上不能接受评审者的建议。无论你有多笃定,还是要退一步自习考虑下评审建议的价值。 如果你坚持自己对的。评审者常常是提供些建议,他们希望你自己可以思考下哪个最好。也许你拥有更多上下文知识,那么你可以耐心解释给他们知道。

• 解决冲突 在解决冲突时,首先要做的是和评审者达成一致。


著作权声明

首次发布于此,转载请保留以上链接