@Rookie
2022-06-22T07:07:04.000000Z
字数 9631
阅读 385
赢海
本节是基于过往经验编写的 Code Review 最佳方式建议。其中分为了很多独立的部分，共同组成完整的文档。虽然您不必阅读文档，但通读一遍会对您自己和团队很有帮助。
代码审查的主要目的是确保逐步改善 Google 代码库的整体健康状况。代码审查的所有工具和流程都是为此而设计的。
为了实现此目标，必须做出一系列权衡。
首先，开发人员必须能够对任务进行改进。如果开发者从未向代码库提交过代码，那么代码库的改进也就无从谈起。此外，如果审核人员对代码吹毛求疵，那么开发人员以后也很难再做出改进。
另外，审查者有责任确保随着时间的推移，CL 的质量不会使代码库的整体健康状况下降。这可能很棘手，因为通常情况下，代码库健康状况会随着时间的而下降，特别是在对团队有严格的时间要求时，团队往往会采取捷径来达成他们的目标。
此外，审查者应对正在审核的代码负责并拥有所有权。审查者希望确保代码库保持一致、可维护及 Code Review 要点中所提及的所有其他内容。
因此，我们将以下规则作为 Code Review 中期望的标准：
一般来说，审核人员应该倾向于批准 CL，只要 CL 确实可以提高系统的整体代码健康状态，即使 CL 并不完美。
这是所有 Code Review 指南中的高级原则。
当然，也有一些限制。例如，如果 CL 添加了审查者认为系统中不需要的功能，那么即使代码设计良好，审查者依然可以拒绝批准它。
此处有一个关键点就是没有“完美”的代码，只有更好的代码。审查者不该要求开发者在批准程序前仔细清理、润色 CL 每个角落。相反，审查者应该在变更的重要性与取得进展之间取得平衡。审查者不应该追求完美，而应是追求持续改进。不要因为一个 CL不是“完美的”，就将可以提高系统的可维护性、可读性和可理解性的 CL 延迟数天或数周才批准。
审核者应该随时在可以改善的地方留下审核评论，但如果评论不是很重要，请在评论语句前加上“Nit：”之类的内容，让开发者知道这条评论是用来指出可以润色的地方，而他们可以选择是否忽略。
注意：本文档中没有任何内容证明检查 CL 肯定会使系统的整体代码健康状况恶化。您会做这种事情应该只有在紧急情况时。
代码审查具有向开发人员传授语言、框架或通用软件设计原则新内容的重要功能。留下评论可以帮助开发人员学习新东西，这总归是很好的。分享知识是随着长年累月改善系统代码健康状况的一部分。请记住，如果您的评论纯粹是教育性的，且对于本文档中描述的标准并不重要，请在其前面添加“Nit：”或以其他方式表明作者不必在此 CL 中解决它。
如果在代码审查过程中有任何冲突，第一步应该始终是开发人员和审查者根据本文档中的 CL 开发者指南和审查者指南达成共识。
当达成共识变得特别困难时，审阅者和开发者可以进行面对面的会议，或者有 VC 参与调停，而不仅仅是试着通过代码审查评论来解决冲突。 （但是，如果您这样做了，请确保在 CL 的评论中记录讨论结果，以供将来的读者使用。）
如果这样还不能解决问题，那么解决该问题最常用方法是将问题升级。通常是将问题升级为更广泛的团队讨论，有一个 TL 权衡，要求维护人员对代码作出决定，或要求工程经理的帮助。 不要因为 CL 的开发者和审查者不能达成一致，就让 CL 在那里卡壳。
审查中最重要的是 CL 的整体设计。CL 中各种代码的交互是否有意义？此变更是属于您的代码库（codebase）还是属于库（library）？它是否与您系统的其他部分很好地集成？现在是添加此功能的好时机吗？
这个 CL 是否符合开发者的意图？开发者的意图对代码的用户是否是好的？ “用户”通常都是最终用户（当他们受到变更影响时）和开发者（将来必须“使用”此代码）。
大多数情况下，我们希望开发者能够很好地测试 CL，以便在审查时代码能够正常工作。但是，作为审查者，仍然应该考虑边缘情况，寻找并发问题，尝试像用户一样思考，并确保您单纯透过阅读方式审查时，代码没有包含任何 bug。
当要检查 CL 的行为会对用户有重大影响时，验证 CL 的变化将变得十分重要。例如 UI 变更。当您只是阅读代码时，很难理解某些变更会如何影响用户。如果在 CL 中打 patch 或自行尝试这样的变更太不方便，您可以让开发人员为您提供功能演示。
另一个在代码审查期间特别需要考虑功能的时机，就是如果 CL 中存在某种并行编程，理论上可能导致死锁或竞争条件。通过运行代码很难检测到这些类型的问题，并且通常需要某人（开发者和审查者）仔细思考它们以确保不会引入问题。 （请注意，这也是在可能出现竞争条件或死锁的情况下，不使用并发模型的一个很好的理由——它会使代码审查或理解代码变得非常复杂。）
CL 是否已经超过它原本所必须的复杂度？针对任何层级的 CL 请务必确认这点——每行程序是否过于复杂？ 功能太复杂了吗？类太复杂了吗？ “太复杂”通常意味着“阅读代码的人无法快速理解。”也可能意味着“开发者在尝试调用或修改此代码时可能会引入错误。”
其中一种复杂性就是过度工程（over-engineering），如开发人员使代码过度通用，超过它原本所需的，或者添加系统当前不需要的功能。审查者应特别警惕过度工程。未来的问题应该在它实际到达后解决，且届时才能更清晰的看到其真实样貌及在现实环境里的需求，鼓励开发人员解决他们现在需要解决的问题，而不是开发人员推测可能需要在未来解决的问题。
将要求单元、集成或端到端测试视为应该做的适当变更。通常，除非 CL 处理紧急情况，否则应在与生产代码相同的 CL 中添加测试。
确保 CL 中的测试正确，合理且有用。测试并非用来测试自己本身，且我们很少为测试编写测试——人类必须确保测试有效。
当代码被破坏时，测试是否真的会失败？ 如果代码发生变化时，它们会开始产生误报吗？ 每个测试都会做出简单而有用的断言吗？ 不同测试方法的测试是否适当分开？
请记住，测试也是必须维护的代码。不要仅仅因为它们不是主二进制文件的一部分而接受测试中的复杂性。
开发人员是否为所有内容选择了好名字？ 一个好名字应该足够长，可以完全传达项目的内容或作用，但又不会太长，以至于难以阅读。
开发者是否用可理解的英语撰写了清晰的注释？所有注释都是必要的吗？通常，注释解释为什么某些代码存在时很有用，且不应该用来解释某些代码正在做什么。如果代码无法清楚到去解释自己时，那么代码应该变得更简单。有一些例外（正则表达式和复杂算法通常会从解释他们正在做什么事情的注释中获益很多），但大多数注释都是针对代码本身可能无法包含的信息，例如决策背后的推理。
查看此 CL 之前的注释也很有帮助。 也许有一个 TODO 现在可以删除，一个注释建议不要进行这种改变，等等。
请注意，注释与类、模块或函数的文档不同，它们应该代表一段代码的目的，如何使用它，以及使用时它的行为方式。
Google 提供了所有主要语言的风格指南，甚至包括大多数小众语言。确保 CL 遵循适当的风格指南。
如果您想改进风格指南中没有的一些样式点，请在评论前加上“Nit：”，让开发人员知道这是您认为可改善代码的小瑕疵，但不是强制性的。不要仅根据个人风格偏好阻止提交 CL。
CL 的作者不应在主要风格变更中，包括与其他种类的变更。它会使得很难看到 CL 中的变更了什么，使合并和回滚更复杂，并导致其他问题。例如，如果作者想要重新格式化整个文件，让他们只将重新格式化变为一个 CL，其后再发送另一个包含功能变更的 CL。
如果 CL 变更了用户构建、测试、交互或发布代码的方式，请检查相关文档是否有更新，包括 README、g3doc 页面和任何生成的参考文档。如果 CL 删除或弃用代码，请考虑是否也应删除文档。 如果缺少文档，请询问。
查看分配给您审查的每行代码。有时如数据文件、生成的代码或大型数据结构等东西，您可以快速扫过。但不要快速扫过人类编写的类、函数或代码块，并假设其中的内容是 OK 的。显然，某些代码需要比其他代码更仔细的审查——这是您必须做出的判断——但您至少应该确定您理解所有代码正在做什么。
如果您觉得这些代码太难以阅读了并减慢您审查的速度，您应该在您尝试继续审核前要让开发者知道这件事，并等待他们为程序做出解释、澄清。在 Google，我们聘请了优秀的软件工程师，您就是其中之一。如果您无法理解代码，那么很可能其他开发人员也不会。因此，当您要求开发人员澄清此代码时，您也会帮助未来的开发人员理解这些代码。
如果您了解代码但觉得没有资格做某些部分的审查，请确保 CL 上有一个合格的审查人，特别是对于安全性、并发性、可访问性、国际化等复杂问题。
在广泛的上下文下查看 CL 通常很有帮助。通常，代码审查工具只会显示变更的部分的周围的几行。有时您必须查看整个文件以确保变更确实有意义。例如，您可能只看到添加了四行新代码，但是当您查看整个文件时，您会看到这四行是添加在一个 50 行的方法里，现在确实需要将它们分解为更小的方法。
在整个系统的上下文中考虑 CL 也很有用。 这个 CL 是否改善了系统的代码健康状况，还是使整个系统更复杂，测试更少等等？不要接受降低系统代码运行状况的 CL。大多数系统通过许多小的变化而变得复杂，因此防止新变更引入即便很小的复杂性也非常重要。
如果您在 CL 中看到一些不错的东西，请告诉开发者，特别是当他们以一种很好的方式解决了您的的一个评论时。代码审查通常只关注错误，但也应该为良好实践提供鼓励。在指导方面，比起告诉他们他们做错了什么，有时更有价值的是告诉开发人员他们做对了什么。
在进行代码审查时，您应该确保：
现在您已经知道了 Code Review 要点，那么管理分布在多个文件中的评论的最有效方法是什么？
查看 CL 描述和 CL 大致上用来做什么事情。这种变更是否有意义？如果在最初不应该发生这样的变更，请立即回复，说明为什么不应该进行变更。当您拒绝这样的变更时，向开发人员建议应该做什么也是一个好主意。
例如，您可能会说“看起来你已经完成一些不错的工作，谢谢！但实际上，我们正朝着删除您在这里修改的 FooWidget 系统的方向演进，所以我们不想对它进行任何新的修改。不过，您来重构下新的 BarWidget 类怎么样？“
请注意，审查者不仅拒绝了当前的 CL 并提供了替代建议，而且他们保持礼貌地这样做。这种礼貌很重要，因为我们希望表明，即使不同意，我们也会相互尊重。
如果您获得了多个您不想变更的 CL，您应该考虑重整开发团队的开发过程或外部贡献者的发布过程，以便在编写CL之前有更多的沟通。最好在他们完成大量工作之前说“不”，避免已经投入心血的工作现在必须被抛弃或彻底重写。
查找作为此 CL “主要”部分的文件。通常，包含大量的逻辑变更的文件就是 CL 的主要部分。先看看这些主要部分。这有助于为 CL 的所有较小部分提供上下文，并且通常可以加速代码审查。如果 CL 太大而无法确定哪些部分是主要部分，请向开发人员询问您应该首先查看的内容，或者要求他们将 CL 拆分为多个 CL。
如果在该部分发现存在一些主要的设计问题时，即使没有时间立即查看 CL 的其余部分，也应立即留下评论告知此问题。因为事实上，因为该设计问题足够严重的话，继续审查其余部分很可能只是浪费宝贵的时间，因为其他正在审查的程序可能都将无关或消失。
立即发送这些主要设计评论非常重要，有两个主要原因：
一旦您确认整个 CL 没有重大的设计问题，试着找出一个逻辑顺序来查看文件，同时确保您不会错过查看任何文件。 通常在查看主要文件之后，最简单的方法是按照代码审查工具向您提供的顺序浏览每个文件。有时在阅读主代码之前先阅读测试也很有帮助，因为这样您就可以了解该变更应当做些什么。
在Google，我们优化了开发团队共同开发产品的速度，而不是优化单个开发者编写代码的速度。个人开发的速度很重要，它并不如整个团队的速度那么重要。
当代码审查很慢时，会发生以下几件事：
如果您没有处于重点任务的中，那么您应该在收到代码审查后尽快开始。
一个工作日是应该响应代码审查请求所需的最长时间（即第二天早上的第一件事）。
遵循这些指导意味着典型的 CL 应该在一天内进行多轮审查（如果需要）。
有一种情况下个人速度胜过团队速度。如果您正处于重点任务中，例如编写代码，请不要打断自己进行代码审查。研究表明，开发人员在被打断后需要很长时间才能恢复到顺畅的开发流程中。因此，编写代码时打断自己实际上比让另一位开发人员等待代码审查的代价更加昂贵。
相反，在回复审查请求之前，请等待工作中断点。可能是当你的当前编码任务完成，午餐后，从会议返回，从厨房回来等等。
当我们谈论代码审查的速度时，我们关注的是响应时间，而不是 CL 需要多长时间才能完成整个审查并提交。理想情况下，整个过程也应该是快速的，快速的个人响应比整个过程快速发生更为重要。
即使有时需要很久才能完成整个审查流程，但在整个过程中获得审查者的快速响应可以显着减轻开发人员对“慢速”代码审查感到的挫败感。
如果您太忙而无法对 CL 进行全面审查，您仍然可以发送快速回复，让开发人员知道您什么时候可以开始，或推荐其他能够更快回复的审查人员，或者提供一些大体的初步评论。 （注意：这并不意味着您应该中断编码，即使发送这样的响应，也要在工作中的合理断点处发出响应。）
重要的是，审查人员要花足够的时间进行审查，确信他们的“LGTM”意味着“此代码符合我们的标准。”但是，理想情况下，个人反应仍然应该很快。
在处理时区差异时，尝试在他们还在办公室时回复作者。 如果他们已经下班回家了，那么请确保在第二天回到办公室之前完成审查。
为了加快代码审查，在某些情况下，即使他们也在 CL 上留下未解决的评论，审查者也应该给予 LGTM/Approval，这可以是以下任何一种情况：
审查者确信开发人员将适当地处理所有审查者的剩余评论。
其余的更改很小，不必由开发者完成。
如果不清楚的话，审查者应该指定他们想要哪些选项。
当开发者和审查者处于不同的时区时，带评论的 LGTM 尤其值得考虑，否则开发者将等待一整天才能获得 “LGTM，Approval”。
如果有人向您发送了代码审查太大，您不确定何时有时间查看，那么您应该要求开发者将 CL 拆分为几个较小的 CL 而不是一次审查的一个巨大的 CL。这通常可行，对审查者非常有帮助，即使需要开发人员的额外工作。
如果 CL 无法分解为较小的 CL，并且您没有时间快速查看整个内容，那么至少要对 CL 的整体设计写一些评论并将其发送回开发人员以进行改进。作为审查者，您的目标之一应该在不牺牲代码健康状况的前提下，始终减少开发者能够快速采取某种进一步的操作的阻力。
如果您遵循这些准则，并且您对代码审查非常严格，那么您应该会发现整个代码审核流程会随着时间的推移而变得越来越快。开发者可以了解健康代码所需的内容，并向您发送从一开始就很棒的 CL，且需要的审查时间越来越短。审查者学会快速响应，而不是在审查过程中添加不必要的延迟。但是，从长远来看，不要为了提高想象中的代码审查速度，而在代码审查标准或质量方面妥协，实际上这样做对于长期来说不会有任何帮助。
还有一些紧急情况，CL 必须非常快速地通过整个审查流程，并且质量准则将放宽。请查看什么是紧急情况？ 中描述的哪些情况属于紧急情况，哪些情况不属于紧急情况。
一般而言，对于那些正在被您审查代码的人，除了保持有礼貌且尊重以外，重要的是还要确保您（的评论）是非常清楚且有帮助的。你并不总是必须遵循这种做法，但在说出可能令人不安或有争议的事情时你绝对应该使用它。 例如：
糟糕的示例：“为什么这里你使用了线程，显然并发并没有带来什么好处？”
好的示例：“这里的并发模型增加了系统的复杂性，但没有任何实际的性能优势，因为没有性能优势，最好是将这些代码作为单线程处理而不是使用多线程。”
关于上面的“好”示例，您会注意到的一件事是，它可以帮助开发人员理解您发表评论的原因。 并不总是需要您在审查评论中包含此信息，但有时候提供更多解释，对于表明您的意图，您在遵循的最佳实践，或为您建议如何提高代码健康状况是十分恰当的。
一般来说，修复 CL 是开发人员的责任，而不是审查者。 您无需为开发人员详细设计解决方案或编写代码。
但这并不意味着审查者应该没有帮助。一般来说，您应该在指出问题和提供直接指导之间取得适当的平衡。指出问题并让开发人员做出决定通常有助于开发人员学习，并使代码审查变得更容易。它还可能产生更好的解决方案，因为开发人员比审查者更接近代码。
但是，有时直接说明，建议甚至代码会更有帮助。代码审查的主要目标是尽可能获得最佳 CL。第二个目标是提高开发人员的技能，以便他们随着时间的推移需要的审查越来越少。
如果您要求开发人员解释一段您不理解的代码，那通常会导致他们更清楚地重写代码。偶尔，在代码中添加注释也是一种恰当的响应，只要它不仅仅是解释过于复杂的代码。
仅在代码审查工具中编写的解释对未来的代码阅读者没有帮助。这仅在少数情况下是可接受的，例如当您查看一个您不熟悉的领域时，开发人员会用来向您解释普通读者已经知道的内容。
有时开发人员会拖延（Pushback）代码审查。他们要么不同意您的建议，要么抱怨您太严格。
当开发人员不同意您的建议时，请先花点时间考虑一下是否正确。通常，他们比你更接近代码，所以他们可能真的对它的某些方面有更好的洞察力。他们的论点有意义吗？从代码健康的角度来看它是否有意义？如果是这样，让他们知道他们是对的，把问题解决。
但是，开发人员并不总是对的。在这种情况下，审查人应进一步解释为什么认为他们的建议是正确的。好的解释在描述对开发人员回复的理解的同时，还会解释为什么请求更改。
特别是，当审查人员认为他们的建议会改善代码健康状况时，他们应该继续提倡更改，如果他们认为最终的代码质量改进能够证明所需的额外工作是合理的。提高代码健康状况往往只需很小的几步。
有时需要几轮解释一个建议才能才能让对方真正理解你的用意。只要确保始终保持礼貌，让开发人员知道你有听到他们在说什么，只是你不同意该论点而已。
审查者有时认为，如果审查者人坚持改进，开发人员会感到不安。有时候开发人员会感到很沮丧，但这样的感觉通常只会持续很短的时间，后来他们会非常感谢您在提高代码质量方面给他们的帮助。通常情况下，如果您在评论中表现得很有礼貌，开发人员实际上根本不会感到沮丧，这些担忧都仅存在于审核者心中而已。开发者感到沮丧通常更多地与评论的写作方式有关，而不是审查者对代码质量的坚持。
开发人员拖延的一个常见原因是开发人员（可以理解）希望完成任务。他们不想通过另一轮审查来完成该 CL。所以他们说会在以后的 CL 中清理一些东西，所以您现在应该 LGTM 这个 CL。一些开发人员非常擅长这一点，并会立即编写一个修复问题的后续 CL。但是，经验表明，在开发人员编写原始 CL 后，经过越长的时间这种清理发生的可能性就越小。实际上，通常除非开发人员在当前 CL 之后立即进行清理，否则它就永远不会发生。这不是因为开发人员不负责任，而是因为他们有很多工作要做，清理工作在其他工作中被丢失或遗忘。因此，在代码进入代码库并“完成”之前，通常最好坚持让开发人员现在清理他们的 CL。让人们“稍后清理东西”是代码库质量退化的常见原因。
如果 CL 引入了新的复杂性，除非是紧急情况，否则必须在提交之前将其清除。如果 CL 暴露了相关的问题并且现在无法解决，那么开发人员应该将 bug 记录下来并分配给自己，避免后续被遗忘。又或者他们可以选择在程序中留下 TODO 的注释并连结到刚记录下的 bug。
如果您以前有相当宽松的代码审查，并转而进行严格的审查，一些开发人员会抱怨得非常大声。通常提高代码审查的速度会让这些抱怨逐渐消失。
有时，这些投诉可能需要数月才会消失，但最终开发人员往往会看到严格的代码审查的价值，因为他们会看到代码审查帮助生成的优秀代码。而且一旦发生某些事情时，最响亮的抗议者甚至可能会成为你最坚定的支持者，因为他们会看到审核变严格后所带来的价值。
如果上述所有操作仍无法解决您与开发人员之间的冲突，请参阅 “Code Review 标准”以获取有助于解决冲突的指导和原则。