原文:https://medium.com/palantir/code-review-best-practices-19e02780015f 作者:Robert F. (https://github.com/uschi2000)
图片来自 https://xkcd.com/1513/
本文谈论了以下话题:
之所以要执行代码审查(code reviews),就是为了籍此改善代码质量,并向团队和公司文化中注入正能量。比如:
对于这个问题没有四海皆准的答案,每个开发团队都应该达成自己的共识。有些团队乐于审查向 master 分支的每次合并,而其他一些团队有自己的细化标准以判断是否需要审查 -- 要有效使用工程师(包括作者和审查者)的时间,也要维护代码质量,得在两者之间兼顾平衡。在某些需要监管的环境中,即便是微小的调整也需要代码审查。
代码审核不分尊卑长幼:作为团队中最资深的人也并不意味着其代码就不需要审查。即便在很少的情况下代码真的完美无瑕,审查也向团队成员和伙伴们提供了至少能从多元化的角度认识库中的代码的机会。
代码审查应该晚于自动检查(自动测试、样式检查、持续集成)成功完成后,但要早于代码合并到仓库的主线分支之前。通常并不对最后一次发布前积累的总量改变执行正式的代码审查。
对于那些应该作为一个整体被合并到主线分支的复杂改变,只做一次代码审查似乎太大了,这时可以考虑一种堆叠式的审查模式:创建一个基础分支如 feature/big-feature
,以及一些二级分支(如 feature/big-feature-api
、feature/big-feature-testing
等等);这些二级分支各自封装一个功能性子集,并独自进行代码审查;一旦所有二级分支被合并到基础分支 feature/big-feature
,再创建一次为了把后者合并到主分支的代码审查。
提交易于审查的代码,以免浪费审查者的时间和积极性,对于作者来说是责无旁贷的:
cherry-pick
、rebase
等施展的版本控制小魔法,遇到大的重构也会玩不转。想要撤销一次因为重构而将行为改变引入到版本库中的提交是极为麻烦的。下面是一个不错的 commit message 示例,来自被广为引用的 http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html
简短扼要的概述,是必选的一个部分。有些情况下,首行信息被视为标题,剩下的部分则作为正文对待。 可选的更多细节性的描述文字。 用祈使句(用于表达命令、请求、劝告、警告、禁止等的句子) 描述 commit message, 如:"Fix bug" ,而不是 "Fixed bug" 或 "Fixes bug"。 用 git merge 或 git revert 等命令生成的 commit message 也符合这个约定。 可用空行分割段落。 - 用列表符号表示段落也不错
应该同时表述 commit 的 what 和 how:
> 孬 再次编译
> 好 增加 jcsv 依赖项以修复 IntelliJ 编译
建议提交者去找一两个熟悉代码库的审查者,其中的一个通常应该是项目领导或高级别工程师。超过两个审查者就容易因为意见相左而效率底下甚至产生相反效果了,三个和尚没水喝。
一次代码审查就是一个在不同的团队成员之间同步观点的过程,自然也有延宕进程的隐患。因此代码审核应该麻利些(以小时计而非天),团队成员和领导也需要优先对待审查并承诺完成的时间。如果你觉得不能按时完成一次审查,请让提交者马上知晓,以便另请高明。
一次审查应该足够彻底,也就是审查者能以一个合理的详细程度向其他开发者解释代码的改变。这确保了代码库中的细节可以被不止一个人所熟知。
作为审查者,你有责任强制实施编码规范并保证编码质量不断提升。审查代码与其说是一门科学,不如说是一门艺术。学习它的唯一途径就是去实行它;一个有经验的审查者应该考虑让其他经验不足的审查者参与进来,并让他们优先评审。
假如代码作者已经遵循了上述原则(特别是自我审查和确保代码能运行了),这里还有一份审查者应该注意的清单:
不要忘记赞美 简介/可读/有效/优雅 的代码。反之,在一次审查中婉拒或反对也并非无礼。若改动是多余或不对的,解释后拒绝掉就是了。如果你因为一个或多个大的瑕疵觉得这次改动不可接受,那就不要赞成,同样得解释清楚。有时一次代码审查的正确结局就是 “让我们用完全不同的方法来解决这个吧” 甚至 “干脆别这么干了”。
尊重提交审核的伙伴。虽然“对手思维”很有效,但那毕竟不是你的代码,你考虑的并不一定那么周全。如果通过代码你无法苟同,那么面对面交流一下,或是寻求第三方意见或许更有效。
核实 API 端与代码库中其他部分保持一致,执行了适当的认证和鉴权。检查其他常见薄弱环节,如弱配置、恶意用户输入、缺少 log 事件 等等。如果有疑问,寻求安全专家的帮助。
审查者的注释 应该简明,并且用人话写。评论代码,而不是用作者的口气。 当有些问题不甚清楚时,询问后弄清楚好过假设那就是愚蠢的。避免人物之间的比较,带上评价就更不好:“你改代码之前我的代码明明能运行的”、“你的函数有 bug” 等等。避免绝对的判断:“这样根本运行不了”、“结果总是错的”。
尝试着分清 建议(如 “建议:抽取成方法以增加可读性”)、需要改变(如 “增加 @Override”),和 澄清(如 “该行为的确恰当吗?如果是的话,请增加一个注释来解释这个逻辑”)。也可以考虑提供链接等,以深入解释问题。
当你完成一个代码审核之后,指明你希望作者在何种程度上响应你的评论,以及是否想要在本次审查出的问题都被解决后重新审查一次(举例来说,"放轻松些,完成那几个小建议的地方后合并一下就行了" 对比于 "请考虑我的建议,并让我知道何时能再看一眼")。
代码审查的目的之一就是督促作者不断改进;因此,不要因为审查者的建议闷闷不乐,即便你不以为然也要严肃对待。回应每一条注释,即便只是用 “知道了” 或 “已完成”。解释你做出某些决策的原因,为何一些函数存在等等。如果你不能和审查者达成共识,线下交流或寻求外部的意见。
修复应该被提交到相同的分支,但要独立的提交。在审查过程中就不断挤进新的提交会让审查者无所适从。
不同的团队有不同的 merge 策略:有些团队只允许项目拥有者合并,其他的则允许贡献者在代码审查得到肯定后合并代码。
对于多数代码审查,基于 diff 的异步工具,诸如 Reviewable、Gerrit 或 GitHub 是很好的选择。而在专业和经验迥异的各方进行复杂的变动或审查时,面对面进行可能更有效;无论是在同一块屏幕或投影仪面前,或者利用远程分享屏幕工具,都是可以的。
在下面的例子中,代码块中的建议性审查注释以 //R: ...
的形式标出:
class MyClass {
private int countTotalPageVisits; //R: 变量命名要统一
private int uniqueUsersCount;
}
interface MyInterface {
/** 当 s 无法被提取时,返回 {@link Optional#empty} */
public Optional<String> extractString(String s);
/** 当 {@code s} 不能被重写时,返回 null */
//R: 应该统一返回值:都用 Optional<>
public String rewriteString(String s);
}
//R: 移除这个,并用 Guava 的 MapJoiner 库代替
String joinAndConcatenate(Map<String, String> map, String keyValueSeparator, String keySeparator);
int dayCount; //R: 那谁: 我更喜欢用 numFoo 而非 fooCount; 是你说了算,但我们应该在这个项目中保持一致性
//R: 这里执行了 numIterations+1 迭代, 是故意的吗?
// 如果是的话,考虑改变 numIterations 的含义吧?
for (int i = 0; i <= numIterations; ++i) {
...
}
otherService.call(); //R: 我觉着我们应该避免对 OtherService 的依赖。我们当面聊一下如何?