数据结构论坛

首页 » 分类 » 定义 » 谷歌开源内部代码评审规范
TUhjnbcbe - 2024/3/14 16:08:00

谷歌成立于年,以搜索起家,到目前为止已经发展了21年。在过去的21年中,谷歌不断创新,开发了七款产品,拥有超过10亿级活跃用户,谷歌的工程师文化一直被认为是优秀且特别的。近日,谷歌开源了其内部一直在使用的代码评审规范,InfoQ对其进行了翻译和整理,分享给广大开发者,看看谷歌工程师是如何评审代码的。

目录[隐藏目录]

1代码评审标准

2代码评审要注意哪些事情?

3检查CL

4代码评审的速度

5怎样写评审注解

6代码评审回推

代码评审标准

代码评审的主要目的是确保代码库的整体质量随时间推移逐步得到提升,所有代码评审工具和过程都是为了实现这一目标而设计的。

为了实现这个目标,必须做出一系列权衡。首先,开发人员的开发任务必须要有所进展。如果他们不提交改进的代码,代码库质量就得不到改善。此外,如果评审人员过于严格,开发人员就没有动力进行持续改进。

评审人员的职责是确保每个CL(变更列表)的质量,保证代码库整体质量不会随着时间的推移而下降。这是一项艰巨的任务,因为代码库整体质量常常会随着每次提交代码质量的小幅下降而退化,特别是有时候开发团队时间很紧,并认为必须走捷径才能完成交付任务。

评审人员要对他们评审的代码负起责任,确保代码库保持一致性和可维护性。

以下是可在代码评审中使用的准则:

一般来说,如果CL达到可以提升系统整体代码质量的程度,就可以让它们通过了,即使它们可能还不完美。

这是所有代码评审准则的最高原则。

当然,也有例外的时候。例如,如果CL中包含了系统不需要的功能,那么即使代码写得很好,评审人员也可以拒绝让它们通过。

这个世界上没有“完美”的代码,只有更好的代码。评审人员不应该要求开发人员对CL中的每一个微小部分都进行细致入微的打磨,而应该在满足需求和变更重要性之间做出权衡。评审人员不应该追求完美,而应该追求持续改进。如果一个CL能够从整体上提高系统的可维护性、可读性和可理解性,那它就不应该仅仅因为它不够“完美”而被延迟几天甚至几周。

评审人员应该提供建议,告诉开发人员哪些方面可以做得更好。但如果这些建议不是很重要,可以在前面加上像“Nit:”这样的前缀,让开发人员知道这只是一个改进建议,他们也可以选择忽略。

指导

代码评审的一个作用是向开发人员传授知识,比如关于一门语言、一个框架或一般软件设计原则的知识。分享知识是提升系统代码质量的一个组成部分。但要注意,如果你的建议纯粹是带有教育性质的,并且对于满足本文所描述的标准来说并不是那么重要,那么请在前面加上“Nit:”,或者以其他方式告诉开发人员,他们并不一定要在CL中解决这些问题。

原则

客观的技术和数据比个人意见和偏好更重要。

在代码风格方面,可以参考谷歌风格指南。任何没有在这个风格指南中出现的东西(比如空格等)都属于个人偏好。代码风格应该与原有代码保持一致,如果之前没有规定代码风格,可以使用代码提交者的代码风格。

软件设计从来就不只风格问题,也不只是个人偏好问题。它们建立在一些基本原则之上,所以我们应该基于这些原则做出权衡,而不只是基于个人偏好。有时候,一个问题有多种解决方案,如果开发人员能够证明(通过数据或基于可靠的工程原理)几种解决方案是同样有效的,那么评审人员应该接受开发人员的选择,否则就应该基于软件设计标准原则做出决定。

如果没有其他适用的原则,评审人员可以要求开发人员与当前代码库保持一致,只要不破坏系统的整体代码质量。

解决冲突

在代码评审过程中出现冲突时,开发人员和评审人员首先要尝试根据本文、CL作者指南和评审人员指南达成一致意见。

如果很难达成一致意见,评审人员和开发人员可以进行面对面会议或者视频会议,而不是只是试图通过代码评审评论板来解决冲突。

如果还不能解决问题,那么就要考虑把问题升级,进行更广泛的团队讨论。让团队负责人参与进来,请求代码维护人员作出决定,或请求工程经理提供帮助。不要因为开发人员和评审人员无法达成一致意见就让CL一直挂在那里。

代码评审要注意哪些事情?

设计

代码评审中最重要的部分是CL的总体设计。CL中不同代码段之间的交互是有意义的吗?这个变更应该属于代码库,还是属于某个包?它与系统的其他部分可以良好地集成吗?现在是引入这个变更的好时机吗?

功能

这个CL是否达到了开发人员的目的?开发人员的意图对代码用户来说有好处吗?代码“用户”可以是指最终用户(他们受代码变更的影响)和开发人员(将来要“使用”这些代码)。

大多数情况下,我们希望开发人员先测试好CL,确保它们能够正确运行。但作为评审人员,你仍然要考虑一些边缘情况,比如查找并发问题,尝试像用户一样思考问题,并找出只是通过阅读代码无法看到的错误。

如果愿意,你也可以验证一下CL。如果一个CL会影响用户,比如做出了UI变更,那么这是验证CL的好时机。如果只是看代码,很难理解一些变更将如何影响用户。对于这样的更改,如果不方便自己运行,可以让开发人员提供功能演示。

另一个重要的考虑点是CL中是否存在可能导致死锁或竞态条件的并发问题。只是简单地运行代码很难发现这类问题,通常需要有人(开发人员和评审人员)仔细思考这些问题,确保不会把它们引入到系统中。

复杂性

CL比实际需要的更复杂吗?从每一层面检查CL,细到每一行代码,它们是不是太复杂了?函数是否过于复杂?类复杂吗?“太复杂”通常意味着“阅读代码的人难以很快理解它们”,也意味着“开发人员在调用或修改这些代码时可能会引入bug”。

过度设计是一种特殊的复杂性,开发人员把代码写得比实际需要的更通用,或者增加了系统当前不需要的功能。评审人员要警惕过度设计,鼓励开发人员只解决现在需要解决的问题,而不是将来可能需要解决的问题。未来的问题应该在它们出现之后再去解决,因为到了那个时候我们可以看到它们的实际状况和需求。

测试

要求开发人员进行单元测试、集成测试或端到端测试。一般来说,CL中应该包含测试,除非这个CL只是为了处理紧急情况。

确保CL中的测试是正确、合理和有用的。因为测试本身无法测试自己,而且我们很少会为测试编写测试,所以必须确保测试是有效的。

如果代码出了问题,测试会失败吗?如果代码发生改动,它们会误报吗?每一个测试都有断言吗?是否按照不同的测试方法对测试进行分类?

请记住,测试代码也是需要维护的。

命名

开发人员是否使用了良好的命名方式?好的命名要能够充分表达一个项(变量、类名等)是什么或者用来做什么,但又不至于让人难以阅读。

注释

开发人员有没有用自然语言写出清晰的注释?他们所写的注释都是必需的吗?通常,注释应该用于解释代码的用处,而不是解释它们在干什么。如果代码不够清晰,无法自解释,那就应该简化代码。当然也有一些例外(例如,正则表达式和复杂的算法,如果能够解释它们在做什么,会让阅读代码的人受益匪浅),但大多数注释都应该指出代码中不可能包含的信息,比如这些代码背后的缘由。

CL附带的其他注解也很重要,比如告知一个可以移除的待办事项,或者一个不要做出代码变更的建议,等等。

注意,注释不同于类、模块或函数文档。文档的目的是为了说明代码的用途、用法和行为。

代码风格

谷歌为主要编程语言和大多数次要编程语言提供了代码风格指南,所以要确保CL遵循了适当的指南。

如果你想对指南中没有提及的风格做出改进,可以在注释前面加上“Nit:”,让开发人员知道这是一个你认为可以改进的地方,但不是强制性的。但请不要只是基于个人偏好来阻止CL的提交。

开发人员不应该将风格变更与其他变更放在一起,这样很难看出CL发生了哪些变化,导致合并和回滚变得更加复杂。如果开发人员想要重新格式化整个文件,让他们将重新格式化后的文件作为单独的CL,并将功能变更作为另一个CL。

文档

如果CL导致用户构建、测试、交互或发布代码的方式发生了变化,请确保相关的文档也得到了更新,包括README、g3doc页和其他生成的参考文档。如果CL有移除或弃用代码,请考虑一下是否也应该删除相关的文档。如果文档缺失,要向开发人员索要。

查看每一行代码

查看每一行代码。有些东西可以看一看,比如数据文件、生成的代码或大型数据结构,但不要只是粗略地扫一下类、函数或代码块,并假定它们都能正常运行。显然,有些代码需要仔细检查,至于是哪些代码完全取决于你,但你至少应该要理解这些代码都在做些什么。

如果代码很复杂或者你难以快速看懂它们,导致评审速度变慢,你要让开发人员知道,并在进行进一步评审之前让他们做一些澄清。如果你看不懂这些代码,其他开发人员很可能也看不懂。因此,要求开发人员澄清代码其实也是在帮助未来的开发人员更好地理解代码。

如果你理解代码,但又觉得没有资格做代码评审,可以确保有资格的CL评审人员在代码评审时考虑到了安全性、并发性、可访问性、国际化等问题。

上下文

代码评审工具通常只显示被修改的代码,但有时候你需要查看整个文件,确保代码变更是有意义的。例如,你可能只看到新添加了四行代码,但如果你看一下整个文件,会发现这四行代码位于一个50多行的方法中,这个时候需要将这个方法拆分为更小的方法。

你需要基于整个系统来考量CL。这个CL是提升了系统的代码质量,还是让整个系统变得更复杂、更不可测?不要接受导致系统代码质量退化的CL。大多数系统都是因为累积了很多小的变更而变复杂的,所以要尽量避免小的变更带来的复杂性。

好的一面

如果你在CL中看到一些不错的东西,要让开发人员知道,特别是当他们以一种很好的方式解决了问题。代码评审通常只

1
查看完整版本: 谷歌开源内部代码评审规范