谷歌开源内部代码评审规范 策划 | 小智，赵钰莹 作者 | 谷歌官方博客 译者 | 无明 编辑 | Natalie AI 前线导读： 谷歌成立于 1998 年，以搜索起家，到目前为止 已经发展了 21 年。在过去的 21 年中，谷歌不断创新，开发了七款产品，拥有超过 10 亿级活跃用户，谷歌的工程师文化一直被认为是优秀且特别的。近日，谷歌 开源了其内部一直在使用的代码评审规范，InfoQ 对其进行了翻译和整理，分享给广大开发者，看看谷歌工程师是如何评审代码的。 更多优质内容请关注微信公众号“AI 前线”（ID：ai-front） 代码评审标准 代码评审的主要目的是确保代码库的整体质量随时间推移逐步得到提升，所有代码评审工具和过程都是为了实现这一目标而设计的。 为了实现这个目标，必须做出一系列权衡。首先，开发人员的开发任务必须要有所进展。如果他们不提交改进的代码，代码库质量就得不到改善。此外，如果评审人员过于严格，开发人员就没有动力进行持续改进。 评审人员的职责是确保每个 CL（变更列表）的质量，保证代码库整体质量不会随着时间的推移而下降。这是一项艰巨的任务，因为代码库整体质量常常会随着每次提交代码质量的小幅下降而退化，特别是有时候开发团队时间很紧，并认为必须走捷径才能完成交付任务。 评审人员要对他们评审的代码负起责任，确保代码库保持一致性和可维护性。 以下是可在代码评审中使用的准则： 一般来说，如果 CL 达到可以提升系统整体代码质量的程度，就可以让它们通过了，即使它们可能还不完美。 这是所有代码评审准则的最高原则。 当然，也有例外的时候。例如，如果 CL 中包含了系统不需要的功能，那么即使代码写得很好，评审人员也可以拒绝让它们通过。 这个世界上没有“完美”的代码，只有更好的代码。评审人员不应该要求开发人员对 CL 中的每一个微小部分都进行细致入微的打磨，而应该在满足需求和变更重要性之间做出权衡。评审人员不应该追求完美，而应该追求持续改进。如果一个 CL 能够从整体上提高系统的可维护性、可读性和可理解性，那它就不应该仅仅因为它不够“完美”而被延迟几天甚至几周。 评审人员应该提供建议，告诉开发人员哪些方面可以做得更好。但如果这些建议不是很重要，可以在前面加上像“Nit:”这样的前缀，让开发人员知道这只是一个改进建议，他们也可以选择忽略。 指 导 代码评审的一个作用是向开发人员传授知识，比如关于一门语言、一个框架或一般软件设计原则的知识。分享知识是提升系统代码质量的一个组成部分。但要注意，如果你的建议纯粹是带有教育性质的，并且对于满足本文所描述的标准来说并不是那么重要，那么请在前面加上“Nit:”，或者以其他方式告诉开发人员，他们并不一定要在 CL 中解决这些问题。 原 则 客观的技术和数据比个人意见和偏好更重要。在代码风格方面，可以参考谷歌风格指南（http://google.github.io/styleguide）。任何没有在这个风格指南中出现的东西（比如空格等）都属于个人偏好。代码风格应该与原有代码保持一致，如果之前没有规定代码风格，可以使用代码提交者的代码风格。软件设计从来就不只风格问题，也不只是个人偏好问题。它们建立在一些基本原则之上，所以我们应该基于这些原则做出权衡，而不只是基于个人偏好。有时候，一个问题有多种解决方案，如果开发人员能够证明（通过数据或基于可靠的工程原理）几种解决方案是同样有效的，那么评审人员应该接受开发人员的选择，否则就应该基于软件设计标准原则做出决定。如果没有其他适用的原则，评审人员可以要求开发人员与当前代码库保持一致，只要不破坏系统的整体代码质量。 解决冲突 在代码评审过程中出现冲突时，开发人员和评审人员首先要尝试根据本文、CL 作者指南（https://google.github.io/eng-practices/review/developer/）和评审人员指南（https://google.github.io/eng-practices/review/reviewer/）达成一致意见。 如果很难达成一致意见，评审人员和开发人员可以进行面对面会议或者视频会议，而不是只是试图通过代码评审评论板来解决冲突。 如果还不能解决问题，那么就要考虑把问题升级，进行更广泛的团队讨论。让团队负责人参与进来，请求代码维护人员作出决定，或请求工程经理提供帮助。不要因为开发人员和评审人员无法达成一致意见就让 CL 一直挂在那里。 代码评审要注意哪些事情？ 设计 代码评审中最重要的部分是 CL 的总体设计。CL 中不同代码段之间的交互是有意义的吗？这个变更应该属于代码库，还是属于某个包？它与系统的其他部分可以良好地集成吗？现在是引入这个变更的好时机吗？ 功能 这个 CL 是否达到了开发人员的目的？开发人员的意图对代码用户来说有好处吗？代码“用户”可以是指最终用户（他们受代码变更的影响）和开发人员（将来要“使用”这些代码）。 大多数情况下，我们希望开发人员先测试好 CL，确保它们能够正确运行。但作为评审人员，你仍然要考虑一些边缘情况，比如查找并发问题，尝试像用户一样思考问题，并找出只是通过阅读代码无法看到的错误。 如果愿意，你也可以验证一下 CL。如果一个 CL 会影响用户，比如做出了 UI 变更，那么这是验证 CL 的好时机。如果只是看代码，很难理解一些变更将如何影响用户。对于这样的更改，如果不方便自己运行，可以让开发人员提供功能演示。 另一个重要的考虑点是 CL 中是否存在可能导致死锁或竞态条件的并发问题。只是简单地运行代码很难发现这类问题，通常需要有人（开发人员和评审人员）仔细思考这些问题，确保不会把它们引入到系统中。 复杂性 CL 比实际需要的更复杂吗？从每一层面检查 CL，细到每一行代码，它们是不是太复杂了？函数是否过于复杂？类复杂吗？“太复杂”通常意味着“阅读代码的人难以很快理解它们”，也意味着“开发人员在调用或修改这些代码时可能会引入 bug”。 过度设计是一种特殊的复杂性，开发人员把代码写得比实际需要的更通用，或者增加了系统当前不需要的功能。评审人员要警惕过度设计，鼓励开发人员只解决现在需要解决的问题，而不是将来可能需要解决的问题。未来的问题应该在它们出现之后再去解决，因为到了那个时候我们可以看到它们的实际状况和需求。 测试 要求开发人员进行单元测试、集成测试或端到端测试。一般来说，CL 中应该包含测试，除非这个 CL 只是为了处理紧急情况。 确保 CL 中的测试是正确、合理和有用的。因为测试本身无法测试自己，而且我们很少会为测试编写测试，所以必须确保测试是有效的。 如果代码出了问题，测试会失败吗？如果代码发生改动，它们会误报吗？每一个测试都有断言吗？是否按照不同的测试方法对测试进行分类？ 请记住，测试代码也是需要维护的。 命名 开发人员是否使用了良好的命名方式？好的命名要能够充分表达一个项（变量、类名等）是什么或者用来做什么，但又不至于让人难以阅读。 注释 开发人员有没有用自然语言写出清晰的注释？他们所写的注释都是必需的吗？通常，注释应该用于解释代码的用处，而不是解释它们在干什么。如果代码不够清晰，无法自解释，那就应该简化代码。当然也有一些例外（例如，正则表达式和复杂的算法，如果能够解释它们在做什么，会让阅读代码的人受益匪浅），但大多数注释都应该指出代码中不可能包含的信息，比如这些代码背后的缘由。 CL 附带的其他注解也很重要，比如告知一个可以移除的待办事项，或者一个不要做出代码变更的建议，等等。 注意，注释不同于类、模块或函数文档。文档的目的是为了说明代码的用途、用法和行为。 代码风格 谷歌为主要编程语言和大多数次要编程语言提供了代码风格指南（http://google.github.io/styleguide/），所以要确保 CL 遵循了适当的指南。 如果你想对指南中没有提及的风格做出改进，可以在注释前面加上“Nit:”，让开发人员知道这是一个你认为可以改进的地方，但不是强制性的。但请不要只是基于个人偏好来阻止 CL 的提交。 开发人员不应该将风格变更与其他变更放在一起，这样很难看出 CL 发生了哪些变化，导致合并和回滚变得更加复杂。如果开发人员想要重新格式化整个文件，让他们将重新格式化后的文件作为单独的 CL，并将功能变更作为另一个 CL。 文档 如果 CL 导致用户构建、测试、交互或发布代码的方式发生了变化，请确保相关的文档也得到了更新，包括 README、g3doc 页和其他生成的参考文档。如果 CL 有移除或弃用代码，请考虑一下是否也应该删除相关的文档。如果文档缺失，要向开发人员索要。 查看每一行代码 查看每一行代码。有些东西可以看一看，比如数据文件、生成的代码或大型数据结构，但不要只是粗略地扫一下类、函数或代码块，并假定它们都能正常运行。显然，有些代码需要仔细检查，至于是哪些代码完全取决于你，但你至少应该要理解这些代码都在做些什么。 如果代码很复杂或者你难以快速看懂它们，导致评审速度变慢，你要让开发人员知道，并在进行进一步评审之前让他们做一些澄清。如果你看不懂这些代码，其他开发人员很可能也看不懂。因此，要求开发人员澄清代码其实也是在帮助未来的开发人员更好地理解代码。 如果你理解代码，但又觉得没有资格做代码评审，可以确保有资格的 CL 评审人员在代码评审时考虑到了安全性、并发性、可访问性、国际化等问题。 上下文 代码评审工具通常只显示被修改的代码，但有时候你需要查看整个文件，确保代码变更是有意义的。例如，你可能只看到新添加了四行代码，但如果你看一下整个文件，会发现这四行代码位于一个 50 多行的方法中，这个时候需要将这个方法拆分为更小的方法。 你需要基于整个系统来考量 CL。这个 CL 是提升了系统的代码质量，还是让整个系统变得更复杂、更不可测？不要接受导致系统代码质量退化的 CL。大多数系统都是因为累积了很多小的变更而变复杂的，所以要尽量避免小的变更带来的复杂性。 好的一面 如果你在 CL 中看到一些不错的东西，要让开发人员知道，特别是当他们以一种很好的方式解决了问题。代码评审通常只关注错误的东西，但其实也应该鼓励和赞赏好的代码实践。有时候，让开发人员知道他们做对了事情比让他们知道做错了事情更有价值。 总结 在进行代码评审时，你要确保： 良好的代码设计。功能对代码用户来说是有用的。UI 变更应该是合理的。并行编程是安全的。代码复杂性不要超过应有的程度。不需要实现可能会在未来出现的需求。有适当的单元测试。精心设计的测试用例。使用了清晰的命名方式。清晰而有用的代码注释，要解释“为什么”，而不是“什么”。恰如其分的代码文档化。代码要遵循风格指南。检查每一行代码，查看上下文，确保你正在改进代码质量，并为表现不错的开发人员点赞。 检查 CL 在知道了代码评审要关注哪些东西之后，如何有效地进行跨文件代码评审呢？ 代码变更有意义吗？它们有没有良好的描述？先看一下代码变更中最重要的部分，它整体设计得如何？按照适当的顺序检查 CL 的其余部分。 第一步：从整体查看代码变更 先看一下 CL 描述，看看这个 CL 做了些什么。做出这个变更有意义吗？如果这个变更是不必要的，请立即做出回复，并解释为什么不应该发生这个变更。在你拒绝这样的变更时，可以向开发人员建议他们应该做些什么。 例如，你可以说：“看起来你在这方面做得不错，谢谢！不过，我们正打算移除这个系统，所以现在不想对它做任何修改。或许你可以重构一下另外一个类”？ 注意，评审人员在拒绝一个 CL 并提供替代建议时要做得很有礼貌。礼貌是很重要的，因为作为开发人员，我们要彼此尊重，即使可能意见不一致。 如果有很多 CL 是你不希望出现的，就要考虑重新调整开发团队或外部贡献者的开发流程，以便在开发新的 CL 之前进行更多的沟通。提前告诉人们哪些事情不要做，这比等他们做完了这些事情再把它们扔掉或者进行彻底重写要好得多。 第二步：检查 CL 的主要部分 找到 CL 的主要文件。通常一个 CL 会有一个包含了主要逻辑变更的文件，也就是 CL 的主要部分。先看看这些主要部分，有助于了解整个上下文，加快代码评审速度。如果 CL 太大，以致于你无法确定哪些部分是主要的，可以询问开发人员，或者让他们把 CL 拆分成多个 CL。 如果 CL 的主要部分存在严重的设计问题，要立即回复开发人员，即使你还没有时间检查 CL 的其余部分。这个时候检查 CL 的其余部分可能是在浪费时间，因为如果主要部分存在严重的设计问题，那么其他部分就变得无关紧要了。 为什么要立即回复开发人员？原因有二： 开发人员在发出一个 CL 之后会继续开始后续的开发工作。如果你正在评审的 CL 存在严重的设计问题，他们也需要重写后续的 CL。所以，最好赶在开发人员在有问题的设计上花费不必要的时间之前告诉他们。大的设计变更比小的变更需要更长的时间。为了让开发人员能够在截止日期之前提交代码，同时又能保持代码库的质量，要尽早让他们开始重写工作。 第三步：按照适当的顺序检查 CL 的其余部分 在确认整体 CL 没有严重的设计问题之后，试着按照某种逻辑顺序来检查其他文件，确保不会错过任何一个需要检查的文件。通常，在你检查完主要文件之后，按照代码评审工具显示它们的顺序来浏览每个文件就可以了。你也可以在检查主要代码之前先查看测试代码，这样可以对代码变更有一个大致的概念。 代码评审的速度 为什么代码评审要快速进行？ 在谷歌，我们对开发团队的整体交付速度（而不是针对个体开发人员写代码的速度）进行了优化。个体开发速度也很重要，但其重要性比不上整个团队的开发速度。 如果代码评审的速度很慢，就会发生以下这些事情： 团队的整体开发速度降低了。如果个体开发人员无法快速地对评审做出响应，可能是因为他们有其他事情要做。但是，如果每个 CL 都要等待一次又一次的评审，那么其他成员的新特性和 bug 修复就会被延迟，可能是几天、几周甚至是几个月。开发人员开始对代码评审流程提出抗议。如果评审人员要隔几天才回复一次，但每次都要求对 CL 进行重大修改，开发人员可能会觉得很沮丧。通常，他们会抱怨评审人员太过严苛。如果评审人员能够快速提供反馈，抱怨就会消失，即使他们要求做出的修改是一样的。代码评审过程的大多数抱怨实际上可以通过加快评审速度来解决。代码质量受影响。如果评审速度很慢，开发人员的压力也会随之增加，因为他们不能提交不甚完美的 CL。缓慢的评审流程还会阻碍代码清理、重构和对现有 CL 做出进一步改进。 代码评审应该要多快？ 如果你不是在集中精力完成手头的任务，那就应该在第一时间评审代码。 对代码评审做出响应最好不要超过一个工作日。 如果遵循这些原则，那么一个典型的 CL 在一天内（如果需要的话）可以进行多轮评审。 速度和中断 有一种情况，即如果你正在集中精力完成手头的任务，比如写代码，那就不要打断自己去做代码评审。研究表明，开发人员被中断之后可能需要很长时间才能恢复到之前的状态。因此，从团队整体上看，在写代码时打断自己比让另一个开发人员等待代码评审要付出更大的代价。 所以，对于这种情况，可以等到你手头工作可以停了再开始代码评审。可以是在完成手头的编码任务之后，午饭后，会议结束后，休息结束后等。 快速响应 我们所说代码评审速度指的是响应时间，而不是 CL 完成整个评审过程并提交到代码库所需的时间。理想情况下，整个评审过程也应该是很快的，但单次评审请求的响应速度比整个过程的响应速度更重要。 有时候可能需要很长时间才能完成整个评审过程，但在整个过程中评审人员的快速响应可以极大减轻开发人员对“慢”评审的沮丧感。 如果你太忙了，可以先向开发人员发送一个响应，让他们知道你什么时候可以开始评审，或者建议让其他可以更快做出响应的评审人员来评审代码，或者提供一些初步反馈。 最重要的是评审人员要花足够的时间进行评审，确保代码符合标准。但不管怎样，最好响应速度还是要快一些。 跨时区代码评审 在进行跨时区代码评审时，试着在开发人员还在办公室的时候做出响应。如果他们已经回家了，那么最好可以确保他们在第二天回到办公室时可以看到代码评审已经完成。 带有注解的 LGTM 为了加快代码评审速度，对于以下两种情况，评审人员应该给出 LGTM（Look Good to Me，没有问题）或者通过，即使他们在 CL 中留下了未解决的问题： 评审人员确信开发人员将会处理好评审人员给出的建议和意见。其余的改动是次要的，不一定要求开发人员完成。当开发人员和评审人员处于不同的时区时，最好可以使用带有注解的 LGTM，否则开发人员可能需要等上一整天才能获得“LGTM，批准”。 大型的 CL 如果有人给你发了一个很大的代码评审，而你不确定是否有足够时间完成评审，通常的做法是要求开发人员把 CL 拆分成几个较小的 CL。这样做通常是合理的，对评审人员来说是有好处的，即使开发人员需要做点额外的工作。 如果一个 CL 不能拆分成更小的 CL，并且你没有足够的时间进行快速评审，至少要对 CL 的总体设计写一些注解，并发给开发人员。评审人员的目标之一是在不影响代码质量的情况下快速对开发人员做出响应，或者让他们能够快速采取进一步行动。 持续改进代码评审 如果你遵循了这些指导原则，并且对代码评审过程严格要求，你会发现，随着时间的推移，整个代码评审过程会变得越来越快。开发人员知道为了保证代码质量需要做些什么，并从一开始就向你发送非常棒的 CL，这样评审所需的时间就会越来越少。评审人员也学会了如何快速做出响应。但不要为了提高评审速度而牺牲代码评审标准或质量——从长远来看，这样做并不会让任何事情变得更快。 紧急情况 在一些紧急情况下，CL 必须非常快速地通过整个评审过程，在质量方面会有些许的放松。请参看这些 “紧急情况”，看看哪些符合紧急情况标准，哪些不符合。 评审注解 概要 礼貌。 解释你的理由。 给出明确的方向，指出问题，并让开发人员决定如何在两者之间做出权衡。 鼓励开发人员简化代码，或者添加代码注释，而不只是让他们解释代码的复杂性。 礼貌 一般来说，礼貌和尊重是很重要的。一个是要确保你的评论是针对代码而不是针对开发人员。你不一定要一直这么做，但当你想说一些可能会让开发人员感到激动或有争议的话时，绝对有必要这么做。例如： 不好的说法：“为什么你要在这个地方使用线程，这样做显然不会获得任何好处”。 好的说法：“在这里使用并发模型增加了系统复杂性，但我看不到任何实际的性能好处，所以这段代码最好使用单线程，而不是多线程”。 解释理由 从上面的正面示例可以看出，这样有助于开发人员理解你为什么要给出这些建议。你并不一定总是要在评审中提供这些信息，但如果你能够为你的意图、所遵循的最佳实践或你的建议将如何改进代码质量给出更多的解释会更好。 给予指导 一般来说，修复 CL 是开发人员的责任，而不是评审人员的责任。你不需要为开发人员提供详细的解决方案或者为他们写代码。 不过，这并不意味着评审人员就不应该帮助开发人员。你最好可以在指出问题和给予指导之间做出权衡。指出问题，并让开发人员做出决策，这样有助于开发人员学到东西，并让代码评审变得更容易。这样还可以产出更好的解决方案，因为开发人员比评审人员更了解代码。 不过，有时候直接给出指令、建议或代码会更有用。代码评审的主要目的是获得尽可能好的 CL。第二个目的是提高开发人员的技能，这样以后需要的评审就会越来越少。 接受注解 如果你要求开发人员解释一段你不理解的代码，他们通常会去重写代码，并把代码写得更清晰。有时候在代码中添加注解也是一种恰当的做法，只要它不只是用来解释太过复杂的代码。 不要只是把注解写在代码评审工具里，因为这对于将来要阅读代码的人来说并没有多大帮助。只有少数情况可以接受这种做法，例如，你对评审的东西不太熟悉，而开发人员的解释却是很多人所熟知的。 代码评审回推 有时候，开发人员会回推代码评审。他们可能不同意你的意见，或者他们抱怨你太严格了。 谁是对的？ 如果开发人员不同意你的意见，先花点时间想一下他们是不是对的。通常，他们比你更熟悉代码，所以可能对代码的某些方面更了解。他们的论点有道理吗？从代码质量角度来看，他们的回推是有道理的吗？如果是，就让他们知道他们是对的，这个问题就解决了。 然而，开发人员并不总是正确的。在这种情况下，评审人员要进一步解释为什么他们的建议是正确的。 如果评审人员认为他们的建议可以改善代码质量，并认为评审所带来的代码质量改进值得开发人员做出额外的工作，那么他们就应该坚持。改善代码质量往往是由一系列的小步骤组成的。 有时候你需要花很多时间反复解释，但要始终保持礼貌，并让开发人员知道你知道他们在说什么。 激动的开发人员 有时候，评审人员会认为如果他们坚持要开发人员做出改动，会让开发人员感到不安。开发人员有时候确实会感到沮丧，但这种感觉通常都很短暂，之后他们会非常感谢你帮助他们提高了代码质量。如果你在评审过程中表现得很有礼貌，开发人员一点都不会感到不安，这种担心可能是多余的。通常，令开发人员感到不安的是你写注解的方式，而不是你对代码质量的坚持。 稍后再解决 一种常见的回推原因是开发人员希望尽快完成任务。他们不想经过一轮又一轮的代码评审，他们说他们会在后续的 CL 中解决遗留问题，你现在让 CL 通过就可以了。一些开发人员会做得很好，他们在提交 CL 后立即就开发后续的 CL。但经验表明，开发人员开发原始 CL 的时间越长，他们进行后续修复的可能性就越小。除非开发人员在提交 CL 之后立即进行修复，否则在通过之后通常不会再去做这件事情。这并不是因为开发人员不负责任，而是因为他们有很多工作要做，而修复工作通常会被遗忘。所以，最好让开发人员马上把 CL 修复掉。 如果 CL 引入了新的复杂性，在提交之前必须将其清理掉，除非是紧急情况。如果 CL 暴露了一些目前还无法解决的问题，开发人员需要把 bug 记录下来，并将其分配给自己，这样它就不会被遗漏。他们还可以在代码中加入 TODO 注释，指向已经记录好的 bug。 抱怨评审太严格 如果你之前的代码评审很放松，然后突然变得严格起来，可能会引起一些开发人员的抱怨。不过没关系，加快代码评审速度通常会让这些抱怨逐渐消失。 有时候，这些抱怨可能需要几个月的时间才能消除，但开发人员到最后通常会看到代码评审的价值，因为他们看到了严格的代码评审有助于产出优秀的代码。有时候，抗议最大声的人甚至会成为你最坚定的支持者。 解决冲突 如果你遵循了上述方法，但仍然会在评审过程中遇到无法解决的冲突，请再次参阅代码评审标准，了解那些有助于解决冲突的指导原则。 原文链接： https://google.github.io/eng-practices/review/reviewer/ 你也「在看」吗？?? 阅读原文