谷歌开源了一套代码评审(Code
Review)规范,它是谷歌一套通用的工程实战指南,几乎涵盖了所有编程语言与各种类型的项目,这个规范代表了谷歌长期发展以来最佳实战经验的集合,谷歌表示希望开源项目或其他组织能够从这套规范中受益。

时间: 2019-09-25阅读: 134标签: Reviewcr的标准

声明: 这是转载文章,转至朱赟老师文章!

澳门新葡萄京官网注册 1

cr(Code review)主要目的在于确保Google
的代码库代码质量越来越好。而所有相关的工具与流程皆是因应这个目的而生。为达到此目的,势必需要做出一连串的权衡与取舍


代码评审,也称代码复查,如果一个团队正在使用任务分支工作流,那么在所有代码编写完成并通过自动化测试之后,在代码合并之前,就会启动代码评审。通常的目的是查找系统缺陷,保证软件总体质量和提高开发者自身水平,代码评审的所有工具和过程都是为了这个目的而构建的。代码评审对于敏捷团队来说的作用如下:

首先,开发人员必须能够在自己负责的任务上有所进展。如果你从来没有向代码库提交改进过的代码,那么代码库就永远不会改进。另外,如果一个reviewer使cr都很难进行的话,那么开发人员就不愿意在将来进行改进。

今天技术管理课的主题是: Code Review,也就是我们常说的代码评审。Code
Review
主要是在软件开发的过程中,对源代码进行同级评审,其目的是找出并修正软件开发过程中出现的错误,保证软件质量,提高开发者自身水平。

  • 代码评审共享知识

  • 通过代码评审可以更好的进行工作评估

  • 代码评审能让你享受休假

  • 通过代码评审指导新工程师

另一方面,reviewer有责任确保每个change
list(下称CL)的质量,保证其代码库的整体代码质量不会越来越差。这可能很棘手,因为通常会随着时间的推移,代码需要降级才能让代码运行起来,特别是当团队受到严重的时间限制时,大家觉得必须走捷径才能实现他们的目标。

和国内的工程师聊天,发现国内公司做代码评审的比例并不算高,这可能和各公司的工程师文化有关系。不过硅谷稍具规模的公司,代码评审的流程都是比较规范的,模式也差不多。

既然代码评审要进行众多的检查,那么找一个优秀的评审者就非常重要了。一般对于变更列表的不同部分,都会有不同的评审者进行细致的审查。当然如果是结对编程,且你的队友能进行高质量的代码评审,那么这样写的代码一般可以视为已经过评审了。此外,我们也可以进行面对面的评审,评审者会问开发者一些问题。

此外,reviewer对他们正在review的代码拥有所有权和责任。他们希望确保代码保持一致、可维护,以及下文的“在cr中可以得到什么”中提到的内容。

首先是两个概念
在进入正题之前,先介绍两个概念,一个是 Commit,一个
PR。硅谷大部分公司都使用 GitHub 企业版管理自己的代码仓库,GitHub 里的
Commit 和 PR 的概念在代码审核中非常重要。

根据谷歌的项目描述,代码审核规范为两套独立文档组成,代表了两方面内容的最佳实践:

因此,我们有以下规则,作为我们在cr中所期望的标准:

1 Commit。就是 GitHub 上的一次“ Commit
”行为,这是可以单独保存的源代码的最小改动单位。

  1. 代码评审者的指南
  2. CL
    作者指南

一般来说,当CL存在的时候,reviewer应该赞成它,此时它肯定会提高正在工作的系统的整体代码质量,即使这个CL并不完美。这是所有cr中的首要原则。当然,这是有局限性的。例如,如果一个CL添加了一个reviewer不希望在其系统中使用的功能,那么reviewer当然可以拒绝,即使代码设计得很好。

2 PR。也就是 Pull Request,是一次代码提交请求。一个 PR 可以包含一次
Commit,也可以是多个。提交请求后 GitHub
会在相关页面上显示这次提交请求的代码和原代码的所有不同之处,这就是本次
PR 的所有改动。

在其中一些文档中使用了一些术语,如下:

这里的一个关键点是,没有“完美”的代码,只有更好的代码。reviewer不应该要求作者在approve之前对一篇文章的每一小段进行润色。相反,reviewer应该权衡发展的需要和他们所建议的change的重要性。reviewer不应追求完美,而应追求持续改进。作为一个整体,提高系统的可维护性、可读性和可理解性的CL不应该因为它不是“完美的”而被延迟几天或几周。

请求提交后,其他工程师可以在 PR
的页面上提出意见和建议,也可以针对某一些代码的改动进行讨论,也可以给整体评价。代码的作者也可以回复这些意见和建议,或者按照建议进行改动,新的改动将为本次
PR 中提交新 Commit(也可以覆盖之前的 Commit)。

  • CL:表示“变更列表(changelist)”,意思是已经提交到版本控制或正在进行代码检查的一个独立的更改。其他组织通常称为“改变”或“补丁”
  • LGTM:意思是“在我看来不错(Looks Good to Me)”,这是代码审阅者在批准
    CL 时说的

reviewer应该随时可以留下评论,表达一些更好的东西,但如果不是很重要,可以在评论前加上“nit:”这样的前缀,让作者知道这只是一个他们可以选择忽略的修饰点。

关于 GitHub 和 Pull Request,池老师最近在他的公众号里写了一篇“ GitHub
为编程世界带来了什么改变 ”,这篇文章中有比较详细的描述,你可以参考学习。

接下来我们来看看两份文档分别的主要内容是什么:

指导

其次,我来谈谈代码合并规则
一般情况下,所有的 PR
都必须有至少一个人认可,才能进行合并。如果改动的内容涉及多个项目,则需要每个项目都有相关人员认可才能合并。还有一些特别关键的代码,比如支付相关的,通常也会需要支付组的人确认才行。

1.代码评审者的指南——如何进行代码评审

cr有一个重要的功能,教开发人员一些关于语言、框架或一般软件设计原则的新知识。留下有助于开发人员学习新知识的评论是可以的。随着时间的推移,共享知识是提高系统代码健康度的一部分。你只需要记住,如果你的评论纯粹是教育性的,但对达到本文档中描述的标准并不重要,请在其前面加上“nit:”,或者以其他方式表明作者不必在本CL中解决它。

在代码合并之前,进行 Code Reivew 的工程师们会在 GitHub
的相关页上给出各种评论,页面是共享的,这些信息大家都能看到。

代码评审者指南本来是一个完整的文档,但作者将其分为了  6 
部分,读者可根据需要阅读。

原则

有些评论是询问,代码的作者直接回复或解释就行,有些是指出代码的问题,代码作者可能会据此改动,也可能不会同意,那就需要回复评论,阐述观点,你来我往。有时候一个实现细节,讨论的主题可以多达十几条或几十条,最终需要达成一致才能进行合并。

  • 代码评审标准
  • 代码评审希望达到什么
  • 在代码评审中导航
    CL
  • 代码评审的速度
  • 如何写审查的评论
  • 处理代码评审的回退

现实和数据推翻了个人喜好。在代码风格方面,风格指南(style
guide)是绝对的权威。任何不在style
guide中的一些点(如空格等)都是个人偏好的问题。代码风格应该与现有的一致。如果项目没有以前的统一样式,那就接受作者的样式。

再次,帮助别人成长,而不是帮他写代码
以前有朋友会说:“看到代码写得太差,觉得来回评论效率太低,干脆自己冲上去搞定”
。曾经我也有过类似的想法,不过我慢慢意识到这并不是一个合适的想法。

2.CL 作者指南——CL 作者批准代码的评审指南

软件设计的各个方面从来都不仅仅是一个纯粹的代码风格问题或者是个人喜好问题。它们是以基本原则为基础的,应当以这些原则为依据,而不仅仅是以个人意见为依据,有时几乎都没有选择的。如果作者能够证明(通过数据或基于原理的一些事实)他的方法是同样有效的,那么reviewer应该接受作者的偏好。否则,代码风格选择取决于软件设计的标准原则。

首先,从对方的角度来说,代码写不好,可能是对业务不熟悉,对编程语言不熟悉,也可能是对公司代码的整体架构不熟悉。如果你帮他
“写”
,而不是耐心指出哪里有问题,那么下一次他可能还是不知道怎么做。这样不仅无益于别人成长,有的时候甚至会让别人有挫败感。

CL
制定者指南包括一些进行代码评审的开发人员的最佳经验,这些经验能够帮助你更快、更高质量地完成评审。

如果没有其他规则适用,那么reviewer可以要求作者与当前代码库中的内容保持一致,只要这些代码不会恶化系统的整体代码健康状况。

并且,帮别人写代码的方式可扩展性很差。即使 Code Review
会花掉十倍于你自己写的时间和精力,但它会让人明白代码应该怎么写,从长远来看,这其实是在一定程度上
“复制” 你的生产力。

  • 写一个好的 CL
    描述
  • 构建一些小的
    CL
  • 如何处理代码评审者的评论

解决冲突

你不能什么都自己写,尤其是开始带项目、带新人以后。每天 Review
五个人的代码和写五个人的代码,长期而言哪个更合算呢?答案显然是前者。

在谷歌看来,代码审核的目的是确保谷歌代码库的整体代码健康程度。谷歌将以下规则作为代码评审的标准:

在cr的任何冲突中,第一步应该始终是开发人员和reviewer根据本文和《CL
Author’s Guide》,尝试达成共识。

写代码是一个学习过程,怎么做一个好的代码审核人更是一个学习和成长的过程。自己绕过一个坑不难,难的是看到别人那么走,远远地你就能告诉他那里有个坑,而他在你的指导下,以后也会帮忙指出别人的类似问题。

一般来说,一旦 CL 能提升整体代码的健康程度,那么即使 CL
不完善,评审者同样也应该倾向于批准该列表。这是所有代码评审指南中的高级原则。它也会有一些限制,例如,如果
CL
添加了一些评审者不需要的特性,那么即使代码做了不错的设计,评审者也应该不予通过。

当达成共识变得特别困难时,reviewer和作者需要进行面对面会议,而不是仅仅试图通过代码审查注释来解决冲突。(不过,如果这样做,请确保将讨论结果记录在CL的评论中,以供将来的读者阅读。)

澳门新葡萄京官网注册,我在这一点上最近感触尤为深刻。随着团队越来越大,新人也越来越多,有一段时间我每天工作的一半时间都在
Review 别人的代码,写评论。

没有所谓的“完美”代码,只有更好的代码。评审人员不应要求作者在批准前对 CL
的每一小部分过分完美。相反,评审者应该权衡向前继续开发的需求和修改建议的重要性。评审者要求的是持续性地改进,而不是追求完美的代码。CL
作为一个整体,如果它能提升系统的可维护性、可读性和可理解性,那么就不要因为它还不完美而推迟数天或数周更新。

如果这不能解决问题,最常见的解决方法就是升级。通常情况下,升级的途径是进行更广泛的团队讨论,让team
leader参与进来,请求代码维护人员做出决定,或者请求技术经理提供帮助。不要因为作者和审稿人不能达成一致意见而让一个其他人袖手旁观。

这样做的初期确实比较辛苦,不过效果也随之慢慢显现,大部分时间工程师们已经可以进行相互
Reivew
代码了,于是我可以腾出很多时间来做别的工作,代码质量也得到了保障。

评审者应该经常留下一些评论,以表达能导致更好性能的做法。如果这些做法并不是非常重要的,那么需要加上前缀「Nit:」,从而令代码作者知道这些内容是可以忽略的。

在cr中要看些什么

提交代码的类型
在进行 Code Review
之前,要搞清楚提交的代码到底是干嘛的,然后进行针对性的审核。我们一般把提交的代码分成四类。

评审指导

注意:在考虑每一点时,一定要考虑cr的标准。

Bug 修复。一般公司都有独立的 Bug 追踪和管理系统,每个 Bug
都有一个票据。代码提交的 PR,一般和票据是关联的。
代码优化。比如文件的移动和拆分,部分函数的重构等。
系统迁移。包括代码库拆分,用另一种语言重写等。
新系统和新功能。新功能在实现之前都需要进行设计审核,最终版本的设计文档会包括数据库的
Schema、API 的签名( Signature
)、代码的流程和模块等内容;相关代码的提交,也就是
PR,一般是和设计文档挂钩的。
了解了提交代码的作用,审核就会更有针对性和效率,也更容易从作者的角度阅读代码。

代码评审有一个很重要的功能,即教开发者一些开发经验,不论是语言、框架还是一般软件设计准则。留一些评论总会帮助开发者学习一些新的知识,共享知识也是改善系统代码健康状态的重要部分。当然,如果评审者的评论仅仅只是教育性的,且对于标准要求不那么重要,那么还是要加上前缀「Nit:」的。

设计

最后说一下 Code Review 的注意事项

评审准则

在cr中重要的是看CL的总体设计。CL中不同代码段的交互是否有意义?此更改属于你的业务代码库还是属于引进来的其他代码库?它是否与系统的其他部分很好地集成?现在是添加此功能的合适时机吗?

从代码提交者的角度,在代码审核中需要注意哪些问题呢?

技术事实和数据要优先于观点与个人风格。

功能

第一,为什么要进行 PR
?原因一定要在提交的时候写得非常清楚,才能帮助审核者理解这个改动是不是合理。上面说的四种提交代码的类型,具体是哪一种,应该写到
PR 的小结中,写得越详细越好。

在代码风格方面,谷歌的代码风格指南是最权威的参考资料。任何不在风格指南中的代码习惯,都属于个人风格,但我们应该保证基本的风格和谷歌风格指南是一致的。

这个CL做了开发者想要的吗?开发者对这些代码的设计初衷用户有好处吗?“用户”通常既是最终用户(当他们受到更改的影响时)又是开发人员(他们将来必须“使用”此代码)。

这在以后需要进行回溯或追踪系统变化时,也是很有益的。如果改的是前端代码,最好贴一个改动前和改动后的截屏,让改动效果一目了然。

软件设计方面几乎不会有纯粹的风格问题,或者纯粹个人的习惯问题。很多风格问题都基于一些基本准测,它们并不是简单地由个人观点决定的。此外,如果代码作者通过数据或基本工程原则证明了几种方法同样有效,那么评审者应该接受作者的风格。否则,偏好的选择还是取决于软件设计的标准原则。

大多数情况下,我们希望开发人员在进行cr时能够对CL进行充分的测试,使其能够正常工作。但是,作为reviewer,仍然应该考虑边缘状况,寻找问题,尝试像用户一样思考,并确保仅通过阅读代码就不会看到错误。

第二,除非是极其明显的单词拼写问题,尽量不要引入不是这个 PR
目的的改动。PR
要尽可能保持目标的单一性。每次遇到有人把一些代码结构的优化合并到功能相关的改动时,我都有一种肝火上升的感觉。

如果没有其它适用规则,那么评审者可以要求作者的偏好与当前代码库保持一致,同时不对整体的代码健康水平产生影响。

如果你愿意的话,你可以自己去验证CL。如果改动会直接带来的用户可见的影响,比如说ui改动,验证CL的变化是非常关键的。

这种行为不仅会增加审核者的困难,降低效率,还会掩盖一些简单的错误。并且,如果因为功能的修改导致线上出了问题,一般需要退回到之前的版本,也就是反转
PR ,这时候,针对优化相关的改动也就必须被反转。总之是弊远远大于利。

解决冲突

在阅读代码时,很难理解某些更改会对用户产生怎样的影响。对于这样的更改,如果不方便自己测试,则可以让开发人员演示该功能(demo)。

第三,找谁审核?除了本组的人外,有时候代码还会和其他项目组的代码相关,需要找该组的成员审核,这时具体找谁呢?

在代码评审中,如果发生了任何冲突,第一步应该是开发者和评审者基于本项目的
CL
指南达成共识。当达成共识非常困难时,开发者与评审者应该面对面地交流,而不只是通过审查中的评论来交流。如果开会讨论还解决不了,那么就要扩大会议了,我们可以通过与代码维护人员、工程经理等开发者的交流,达成最终的共识。

另外,在cr期间考虑功能性特别重要的点是,cl中是否有平行式编程,理论上可能导致死锁或竞争条件。这些类型的问题很难通过运行代码来发现,通常需要有人(开发人员和reviewer)仔细考虑,以确保不会引入问题。(注意,这也是不使用平行式编程的一个很好的理由,在这种情况下,可能出现竞争条件或死锁,这会使代码检查或理解代码变得非常复杂。)

一般有两个机制来解决这个问题。一是在 GitHub 里 @ 一个组,比如 Payment
组,Risk
组等等,这些组会通知组里所有的人,相关的人看到了就回去审核;二是有一些组的代码,不希望其他组的人在自己不知道的情况下进行改动,就会设置规则,如果有人动了这些代码,也会通知到整个组。

如果想要深入了解谷歌的这套代码审核规范,可查看该项目。地址如下:

复杂性

最后,也是最重要的,一定确保所有的改动都是测试过的,无一例外。

一个CL是否复杂到超过预期的必须?针对任何层级的CL必须确认这几点:单行代码是否过于复杂?函数是否过于复杂?class是否过于复杂?“复杂”通常意味着该代码很难阅读,很有可能也意味着其他开发者在修改这段代码的时候引入其他bug

从代码审核者的角度,又需要注意哪些问题呢?

来源:机器之心

其中一种复杂性就是过度设计(Over-engineering)造成的,开发者会让那段代码过度通用化,超过了原本所需,或者还新增了系统目前不需要的功能。reviewer应特别注意一下过度设计。鼓励开发者解决他们知道现在需要解决的问题,而不是推测将来可能需要解决的问题。当那些将来出现的问题出现的时候才开始解决它们,因为那时候你可以更加清晰看见问题的原样子

审核的粒度要多细?是不是每次审核都要花很多时间?当然,如果时间足够,自然是看得越细越好。如果特别忙的时候,可以做一些筛选。

Donald Knuth说过:过早的优化是万恶之源(Premature optimization is the
root of all evil)

比如,你可以看一下算法或者编程思路,然后加一个评论
“算法部分看来没有问题”;也可以只看你关心的部分,然后加评论
“支付部分没问题”,或者 “API 部分没问题”。还可以再 @
一些你觉得可以对其他部分加评论的人。

测试

另外,如果是新人的代码,尽可能地在代码风格、性能等各方面都加以审查。如果是一个老员工,这些方面可以给予更多信任。

请将单元测试、整合测试、端到端测试视为要求CL所做的适当变更。一般CL内除了生产环境的业务代码外,测试也应该要被加入其中。除非该CL是为了处理某个紧急事情而存在。

具体哪些地方需要审核呢?总结一下。

另外,也要确保测试是正确、合理、有用的。测试并非来测试它们本身,一般也极少为了测试而测试(如测试一下测试代码有没有问题又走了测试流程),因此我们要保证测试是有效的。

1 代码格式方面。很多公司都有编程语言风格指南( Coding Style Guideline
),这是大家的约定俗成,避免公司的代码风格不一致,也避免了一些
“要不要把闭括号另起一行”
的无谓争论。老员工除非不小心,通常大家都不会弄错;新员工在这方面不太熟悉,就有可能出问题。这一类问题是比较容易指出的。

当代码真的有问题,测试是否会失败?如果被测试的程序发生改动时,测试是否会产生误报?每一个测试是否做出了简单而有用的断言?不同的测试方法之间的测试是否适当分开?

2
代码可读性方面。比如函数不要太长,太长就进行拆分。所有的变量名要能说明它的用意和类型(比如
hosting_address_hash,一看就知道是房东地址,而且是个哈希类型)。

请记住,测试代码也是必须维护的代码,不要因为它们不在主要关注的范围内。

不要有嵌套太多层的条件语句或者循环语句。不要有一个太长的布尔类型(
Boolean
)判断语句。如果一个函数别人需要看你的长篇注释才能明白,那这个函数就一定有重构的空间。另外,如果不可避免有一些注释,则一定要保证注释准确且与代码完全一致。

命名

3
业务边界和逻辑死角问题。你可以帮助代码作者想想,他有没有漏掉任何业务边界和逻辑死角问题。很多时候这是业务逻辑相关的,尤其需要资深一点的工程师帮助指出需要处理的所有情况。

开发者是否为了每一个东西都挑了一个适当的名字?一个好的命名意味着,通过名字就足以完整表达该东西的作用是什么或者要做什么。但是同时名字也不要长得难以阅读

4 错误处理(Error
Handling)。这是最常见的问题,也是代码审核最容易帮别人指出来的问题。

推荐参考文章「Clean code 101 — Meaningful names and
functions」:-skills/clean-code-101-meaningful-names-and-functions-bf450456d90c

我在文稿中举出了一个例子,一段简单到不能再简单的代码就至少有三个潜在的问题。这些都是需要审核者注意的。

注释

def update_user_name(params)
user = User.find(params[:user_id])
user.name = params[:new_name]
user.save!
end
params 里面需要 validate 是不是有 user_id 和 new_name 这两个 key
能不能找到这个 user_id 对应的 user
save 的时候会不会有 DB level 的 exception,应该怎么处理
5
确保测试用例覆盖到了所有的功能路径。严格来说,每段代码都应该有测试用例。如果开发者能够预见到其他人的代码改动会引发自己的代码问题,一定要增加额外的测试用例防止这种情况的发生。

开发者是否用可理解的英文留下清晰的注释? 这些注释是否真的必要?

6
代码质量和规范。遵循公司制定的编程规范,比如,如果有重复的代码段,就应该提取出来公用,不要在代码里随意设常数,所有的常数都应该文件顶部统一定义,哪些变量应该是私有的,哪些应该是公有的,等等。

通常注释是解析这段代码为什么存在的时候是相当有用的,而不应该去解释某段代码正在做什么。如果代码本身不能解释清楚的话,意味着它更加需要简化了。当然也有例外,比如解释正规的表达式或者复杂的算法正在做什么的时候,注释解释这段代码正在做什么就相当有用。但对于大部分注释来说是用来说明那些不包含在程序本身但资讯,比如说为什么要这样子做的理由

7 代码架构。包括代码文件的组织方式,函数是不是抽象到 lib 或者 helper
文件里;是不是应该使用继承类;是不是和整个代码库的风格一致;API
的定义是不是 RESTful 的等等。

查看该CL之前的注释也很有帮助,或许有一个todo项目现在可以移除、一个评论建议为什么不要进行这种更改等等。

公司层面的支持
从公司层面应该有哪些措施帮助员工有效地进行代码审核呢?

要注意的是,注释与class、module、function的文件不同。后三者要能够表达一段代码的目的、如何使用它、使用时行为

统一的代码提交和审核流程与工具,并确保大家使用同样的工具,遵循相同的流程。
鼓励员工帮助别人审核代码,甚至可以做到效绩评估中。
制定统一的编程规范和代码风格,尤其是在有争议的地方,这样可以解决一些因为程序员偏好带来的矛盾。
代码审核和编程一样,都是日常工作,不要情绪化。我曾经做过一件事,一个外组同事的代码,别人已经认可合并了,可是我觉得有问题,于是反转了流程,在评论里写了原因和建议的改法;当时心里还觉得会不会得罪人。可是后来发现同事反而很客气地接受并道谢了。

风格

另外,评论里经常会出现不同意见,其实是两个人在编程习惯和约定俗成上没有达成共识。如果在不违背公司风格指南的情况下,没必要一定让对方和你有一样的习惯。

Google 对于主要语言都有提供风格指南(style guide:
),甚至包括大多数冷门语言,因此要确保CL遵循适当的指南上的说明。

如果你真的觉得这样做更好,可以委婉地提出来,比如 “我个人是更偏向于 A
类型的编程风格,不过这不是一个硬性规定。” 或者 “嗯,改成 A
类型的编程风格如何,不过这不是强制的。”

如果你想改进CL中某些不包含在风格指南中的要点时,请在评论前加上Nit:
,让开发人员知道这是你认为可以改善代码的小问题且并非强制性的。但记住,不要仅根据个人风格偏好阻止提交CL。

今天我和你讨论了 Code Review,主要内容包括 Code Review
中的重要概念,代码合并的规则,帮助别人评审代码而不是写代码,提交代码的类型和做
Code Review 时的注意事项。

开发者不应该在
CL内同时包含主要风格的改动与其他代码的修改,这样会导致难以看出CL到底做出什么改动。同时也会让合并和回滚更为复杂,并产生其他问题。例如,如果作者想要重新格式化代码的话,让他们将新格式在一个新CL里面重构。

事实上工程师们在编程的时候很难保证万无一失,多几双眼睛一起帮你看一遍,就可以很大程度地减少代码里的错误。另外,相互审核的过程中还能从彼此那里学到很多编程的小技巧和好习惯。细想来,很多
Java 和 Ruby 的特别好用的技巧,我都是在做代码审查的过程中学到的。

文档

你的团队有没有做 Code Review
呢?有好的经验可以在留言里告诉我。感谢你的收听,我们下期再见。

如果CL更改了构建、测试、交互、发布的时候,请检查是否也同时更新相关文档,
包括README,g3doc 页面和其他生成(generated) 的参考文件。如果CL
删除或弃用
了一些代码,请考虑是否应该删除对应文档,如果缺少文档时请询问。

每一行代码

仔细review分配给你的每一行代码。有些东西,比如资料文件(data
files)、生成的代码(generated code)、大型数据结构(large data
structures),你可以稍微扫过。千万不要在扫过开发者写的
class、函数、代码区块
时,去假设它内部是没问题的。很显然的某些代码需要比其他代码更仔细的review。这是必须由你做出的判断,但至少你应该确定你理解所有代码在做些什么。

如果阅读代码过于复杂并且减慢review速度时,那么你再继续review前,要让开发者知道这件事,并等待他们为这段代码做出解释、说清楚。在Google
我们聘请许多优秀的软件工程师,而你也是其中的一员。如果连你也无法理解的话,很可能其他人也不会。因此,你要求开发者去说清楚这段代码时,同时也在帮助未来的开发人员理解这些代码。

如果你能够理解,但觉得没有资格进行某部分的审核,请确保
reviewer中有一个适合(合格)
的人来review该部分。尤其是针对安全性、并发性、可访问性、国际化等复杂问题时。

上下文

在充足的上下文下查看CL通常很有帮助。一般来说,cr工具只会显示修改部分周围的几行代码而已。但有时你必须查看整个文件以确保改动是否合理。比方说,你可能只看到添加4行新代码,但实际上查看整个文件时会发现这4行是被加在有50行的代码里,此时需要将它拆解为更小的函数。

以整个系统的角度出发来思考CL也是很有用的。该CL是否改善整体系统的代码质量,亦或是会让整个系统更加复杂?是否缺少测试?千万不要接受会降低整体系统的代码质量的CL。因为大多数系统是由于许多小改动的积累而变得复杂,因此阻止新的改动引入复杂性(尽管很小)也非常重要。

优点

当你在CL里看到优点时记得告诉开发者,尤其是当他们用很棒的方式来解决你的评论时。cr通常只关注存在的错误,但它们也应该同时应该为良好实践提供鼓励与欣赏。这点在指导开发者时尤其重要:与其告诉他们做错什么,还不如告诉他们做对了什么。

个人认为并非不要指出错误,而是多以鼓励来代替指出错误,让其他开发者更有动力想将事情做好。其实透过简单的一句话让对方知道哪里做得很好,未来他们会将继续保持下去,并为其他开发者带来的正面影响

标明每个commit 修改什么,可以帮助reviewer快速进入状况

此时就不要吝啬你的称赞了!

总结

在cr时,请务必确保:

代码经过完善的设计

功能性对于使用者们是好的

对于任何UI改动要合理且好看

任何并行编程的实现是安全

代码不应该复杂超过原本所必须的

开发者不该实现一个现在不用而未来可能需要的功能

代码有适当的单元测试

测试经过完善的设计

开发者对于每样东西有使用清晰、明了的命名

注释要清楚且有用,并只用来解释why而非what

代码有适当的写下文件(一般在g3doc)

代码风格符合style guide

确保你查看被要求review的每一行代码、确认上下文、确保你正在改善代码质量,并赞扬开发人员所做的好事与优点吧!

如何浏览CL

现在你已经知道review时要看些什么,但怎样才是review分散在多个文件中的改动最有效的方法?

改动是否合理?他是否有好的描述(description)

优先看CL 最重要的改动。它整体是否有完善的设计?

用合理的顺序看CL 剩余的改动

步骤1: 用宏观的角度来看待改动,查看CL描述以及它做什么

该改动是否有意义、合理?如果在第一时间认为不应该发生这种变化,请立即说明为什么不该这样做的原因。当拒绝类似这样的更改时,向开发人员提供建议告诉他们应该怎么做什么也是一个好主意。

例如,你可以说:“看起来你已经完成一些不错的事情,谢谢!但我们实际上正朝着删除你正在修改的FooWidget系统的方向前进,所以现阶段我们不想对它进行任何新的修改。不如重构我们的新BarWidget
class如何?”

需要注意的是,reviewer在委婉拒绝该CL的同时也要提供替代方案,而且仍然保持礼貌。这种礼貌是很重要,因为我们希望表明即使不同意也会相互保持尊重。

如果你有几个CL里包含你不想这样做的改动时,你应该重新考虑开发团队的开发过程,或发布开发流程给外部贡献者知道,以便在任何CL被撰写前有更多的沟通。最好是在他们开始投入前说“不”,避免那些已经投入心血的工作现在必须被抛弃或彻底重写。

提供替代方案让对方知道该怎么做,而非让其自行独自摸索。

指出问题,告知替代方案或指点方向

步骤2: 检查CL主要的部分

找到CL最核心的部分的那些文件。通常CL内会有文件包含大量的逻辑改动,而它正是CL的主要部分。因此我们要首先查看这些主要部分。这有助于为CL的其他较小部分提供适当上下文,而且这样通常可以提高review速度。如果CL太大导致于无法确定哪里是主要部分时,请向开发者询问首先应当查看的内容,或者要求他们将CL拆分为多个CL。

如果在主要部分发现存在一些主要的设计问题时,即使没有时间立即查看CL的其余部分,也应立即留下评论告知此问题。因为事实上,因为该设计问题足够严重的话,继续review其他部分的代码可能只是浪费时间,因为其他正在审查的其他代码可能都将无关紧或消失。

立刻发送关于主要设计的评论非常重要有两个主要原因:

通常开发者在送出CL后,在等待review过程中便已经开始着手基于该CL的新工作。此时如果正在review的CL存在重大设计问题的话,开发者将不得不重新写所有基于它的后续所有CL。因此你要在他们有问题的设计上投入之前阻止他们。

主要设计变更通常需要较长时间才能完成,但每个开发人员几乎都有自己deadline。为了赶上deadline并保证代码质量,开发者需要尽快开始或重做CL
任何的主要设计变更。

步骤3: 用合理的顺序看CL 其余的改动

一旦确认整个CL没有重大的设计问题时,试着找出一个有逻辑顺序来review剩余档案,并确保不会错过其中任何一个。通常在浏览主要部分后,最简单的方式是按照cr工具提供的顺序来浏览每个文件。有时在阅读主要代码前先阅读测试也是非常有帮助的,如此一来你就可以了解应该做、看些什么。

review的速度为什么review速度要快

在Google我们优化开发团队共同生产产品的速度,而不是优化个人开发的速度。个人的开发速度很重要,但它不如整个团队的开发速度重要。当cr很慢时,会发生以下几件事:

团队整体的速度下降。review慢的话,对于团队其他人来说重要的新功能与缺陷修复将会被延迟数天、数周甚至数月,只因为它们正在或者等待review。

开发人员开始抗议cr。若reviewer每隔几天才回应一次,但每次都要求对CL进行重大更改的话,那么开发人员可能会感到非常沮丧与觉得困难,而这通常会演变成抱怨。如果reviewer请求相同的实质性更改(且确实可以改善代码质量状况),但在每次开发人员提交新的改动时都能快速反应的话,这些抱怨往往会消失。大多数关于cr的抱怨,往往可以通过加快流程来解决。

代码质量会受到影响。当review很慢时,会造成开发者提交不完全尽如人意的CL
的压力越来越大。评论的越慢也会阻止他人进行代码清理、重构、甚至是对现有CL
的进一步改进。

cr应该要多快?

如果你并没有处于需要专注工作的时候,那么你应该在CL被提交后尽快进行review。review回复最长的极限是一个工作日。若遵循以上指南,意味着一般的CL应该在一天内得到多轮review(如果必要的话)。

速度vs中断

但有时候个人的速度优先度会胜过团队速度。如果你处于需要专注工作的时候(比方说写代码),不要打断自己去做cr。

研究证实,若开发者在被打断后会需要很长时间才能恢复到原本顺畅的开发流程。因此,开发的时候,打断自己实际上会比让另一位开发者等待review来得更加昂贵。

取而代之的是,我们可以在投入到处理他人给的review评论之前,找个适当的时机点来进行cr。这有可能是当你的当前开发任务完成后、午餐、刚从会议脱身或从微型厨房回来等等。

快速回应

当我们谈论cr的速度时,我们关注的是回应时间,而非CL需要多长时间才能完成并提交。在理想情况下,整个过程应该是快速的。

总的来说个人回应评论的速度,比起让整个cr过程快速结束来得更为重要。即使有时需要很长时间才能完成整个流程,但若在整个过程中能快速获得来自reviewer的回应,这将会大大减轻开发人员对于缓慢的cr过程的挫败感。

如果真的忙到难以抽身而无法对CL进行全面review时,你依然可以快速的回应让开发者知道你什么时候会开始审核、建议其他能够更快回复reviewer,又或者提供一些初步的广泛评论。(注意:这并不意味着你应该中断开发去回复——请找到适当的中断时间点去做)

很重要的是,reviewer员要花足够的时间来进行review,确保他们给出的LGTM,意味着“此代码符合我们的标准”。

尽管如此,理想的个人的回应速度还是越快越好。

跨时区review

在面对时区不同的问题时,尝试在他们还在办公室时回复作者。如果他们已经回家了,那么请确保在他们第二天回到办公室前完成。

LGTM 评论

为加快速度,在某些情况下reviewer可以给予LGTM或Approval,即便CL上仍有未解决的评论。类似情况会发生在:

reviewer确信开发人员会适当地处理所有剩余的评论

剩余的评论是微不足道的或它们不需由开发者来处理

reviewer必须清楚指明他们是指上面哪种情况

LGTM
评论对双方处于不同的时区时尤其值得考虑,否则开发人员会等待一整天才得到“LGTM,Approval”。

大型改动

如果有人要求reivew时,但由于改动过于庞大导致你难以确定何时才有时间review它时,你通常该做的是要求开发人员将CL拆解成多个较小的CL,而不是一次review巨大的CL。这种事是可能发生的,而且对于reviewer非常有帮助,即便它需要开发人员付出额外人力来处理。

如果CL无法分解为较小的CL,且你没有足够时间快速查看整个CL内容时,那么至少对它的整体设计写些评论,并发送回开发人员以便进行改进。身为reviewer,你的目标之一是在不牺牲代码质量的状况下,避免阻碍开发人员进度,或让他们能够快速采取其他更进一步的动作。

cr的能力会随着时间进步

如果你遵循这些准则,并且对于cr非常严格的话,后面你会发现整个cr流程会越来越快。因为开发者学到什么是质量好的代码,并于开头就提交一个很棒的CL,而这正恰好只需要越来越少的时间。而reviewer则学会快速回应,而非在过程中加入不必要的延期。

但不要为提高想象中的速度,而对cr标准和代码质量做出妥协,毕竟从长远来看它实际上并不会让任何事情发生得更快。

紧急状况

在某些紧急情况下,CL会希望放宽标准以求迅速地通过整个cr过程。但请看什么是紧急情况(-practices/review/emergencies.html#what)来知道哪些情况实际上属于紧急情况,而哪些不符合。

如何写review评论如何面对被推迟处理的评论

有时开发人员会推迟处理cr产生的评论。要么他们不同意你的建议,要么他们会抱怨你太严格了。

谁是对的

当开发人员不同意你的建议时,请先花点时间考虑一下他们是否是正确的。因为通常他们比你更了解代码,所以他们可能真的比起你来说对代码的某些层面具有更好的洞察力。他们的论点有意义吗?从代码质量的角度来看它是否合理?如果是的话,让他们知道他们是对的,然后让问题沉下去。

但开发人员也并不总是对的。在这种情况下,reviewer应该更进一步解释为什么认为自己的建议是正确的。一个好的解释通常展示了“对开发人员的回覆的理解”以及有关“为什么被要求做出改动”等信息。尤其是当reviewer认为给出的建议会改善代码质量时,便应该继续宣扬自己的论点。只要他们相信所需的额外的工作量最终会改善整体代码质量。提高整体代码质量这件事,往往是经由每个微小的改动来发生。有时往往需要几番解释一个建议才能让对方真正理解你的用意。切记,始终保持礼貌,让开发人员知道你有听到他们在说什么,只是你不同意该论点而已。

让开发者沮丧

reviewer有时会认为若自己坚持改进的话,会让开发人员觉得沮丧不安。的确开发人员有时会感到很沮丧,但这通常是十分短暂的,甚至后来他们非常感谢你帮助他们提高代码质量。一般来说,只要你在评论中表现得很有礼貌,开发人员实际上根本不会感到沮丧,这些担忧都仅存在于reviewer心中而已。沮丧很多时候是对于cr评论的写作方式有关,而并非来自reviewer对于代码质量的坚持。

晚点再来整理干净

一个常见的推迟原因是开发人员希望完成任务(这可以理解)。他们不想通过另一轮cr来批准这个CL。此时他们通常会说在以后的CL进行整理,所以你现在应该要给LGTM。当然部分开发人员非常擅长这一点,并且立即送出一个修复问题的后续CL
(follow-up
CL),但根据过往经验,这种后续“清理行为”非常少见。除非开发者在CL获得批准之后立刻进行清理动作,否则这事根本不可能发生。这不是因为开发人员不负责任,而是因为他们可能有很多其他工作要完成,于是清理工作便会在成堆的工作中被遗忘。因此,通常最好坚持开发人员在代码在合并后清理它们。因为让人们「稍后清理」是致使代码库质量状况下降最常见的状况。

如果CL引入了新的复杂性的话,除非是紧急情况,否则必须在提交之前将其处理掉。如果CL导致暴露周围的问题且现在无法解决它们的话,开发人员应该将缺陷记录下来并分配给自己,避免后续被遗忘。又或者他们可以选择在代码中留下TODO的注释并连jie到刚记录下的缺陷。

关于review严格性的常见抱怨

如果你先前以相当宽松的标准并转趋严格的进行cr的话,一些开发人员会开始大声地抱怨。一般来说,提高review的速度会让这些抱怨逐渐消失。这些抱怨可能需要数个月才会消失,但最终开发人员会看到严格的review所带来的价值,因为严格的review帮助他们产生的优秀代码。而且一旦发生某些事情时,最大声的抗议者甚至可能会成为你最坚定的支持者,因为他们会看到变得review变严格后所带来的价值。

解决冲突

如果你遵循前面所有操作,但仍然遇到无法解决的双方之间的冲突时,请参考前面的cr的标准以获取有助于解决冲突的准则和原则。

原文地址:-practices/review/reviewer/

本文的名词解释:

cr: code review

cl: change list,大概意思是指这次改动

reviewer: cr的那个review人

nit: 全称nitpick,意思是鸡蛋里挑骨头

作者:
也就是本次CL的开发者,原文中是以author来称开发者的。以老外的思维,意思是“CL的作者”