代码审查

人工检查其它开发人员代码的过程称为代码审查。这包括所有更改,即不仅包括新功能,还包括错误修复,甚至包括简单的配置更改。

代码审查总是由至少一名同行开发人员完成,通常发生在拉取请求中,也就是在功能或错误修复分支的代码合并到主分支前不久;只有审查人员批准了更改,这些更改才会成为实际应用程序的一部分。

在本节中,我们将讨论在代码审查中应注意哪些事项、代码审查为何如此重要,以及如何进行代码审查才能使其成为工具包中的成功工具。

为什么你应该进行代码审查

这听起来可能有点显而易见,因为这正是本书的主旨所在。然而,再怎么强调也不为过—​代码审查将提高代码的质量。让我们仔细研究一下原因:

  • 易于引入:引入代码审查通常无需额外成本(除了所需的时间)。所有主要的 Git 仓库服务,如 Bitbucket、GitLab 或 GitHub,都有内置的审查功能,你可以立即使用。

  • 快速见效:代码审查不仅易于引入,而且在引入后很快就会显示出其实用性。

  • 知识共享:由于代码审查通常会引发开发人员之间的讨论,因此它是在团队中传播最佳实践知识的绝佳工具。当然,初级开发人员尤其会从指导中受益匪浅,但经验最丰富的开发人员也会不时学到新东西。

  • 不断改进:通过定期讨论,编码指南将得到改进,因为它们会不断受到挑战,并在必要时得到更新。

  • 尽早避免问题:代码审查会在流程的早期阶段进行(参见 第 11 章,持续集成),因此很有可能在进入测试环境之前就发现错误、安全问题或架构问题。

如果你还不了解代码审查的好处,请查看下一部分,我们将在其中详细讨论代码审查应包括哪些内容,以及不包括哪些内容。

代码审查应该涵盖哪些内容

在进行代码审查时,我们应该检查哪些方面?

  • 代码设计:代码设计是否合理,是否与应用程序的其它部分保持一致?是否遵循了一般的最佳实践,如可重复性、设计模式(见下一节)或 SOLID 设计原则(见第 2 章,谁有权决定什么是 "良好实践"?)

  • 功能性:代码是否做了它应该做的事,或者是否有任何副作用?

  • 可读性:代码是易于理解还是过于复杂?注释是否必要?是否可以通过重命名函数或变量,或将代码提取到一个名称有意义的函数中来提高可读性?

  • 安全性:代码是否引入了潜在的攻击向量?是否对所有输出进行了转义处理以防止 XSS 攻击?是否对数据库输入进行了消毒以避免 SQL 注入?

  • 测试覆盖范围:新代码是否包含自动测试?是否测试了正确的内容?是否需要更多测试用例?

  • 编码标准和指南:代码是否遵循团队商定的编码标准和编码指南?

你的团队还应考虑是否应将在本地开发环境中测试代码作为审核流程的一部分。但目前还没有明确的建议。

代码审查的最佳实践

虽然代码审查有很多好处,而且实施起来也相当容易,但也有一些误区需要注意,而且既定的最佳实践会让审查更加成功。

谁应该审查代码?

首先,最理想的是由谁来进行代码审查?当然,这也取决于你的设置。如果你与另一位 PHP 开发人员在一个团队中工作,那么他肯定是第一个要问的人。这样,你就积累了共同的领域知识;虽然你的同事没有直接参与你的项目,但他们至少能了解你的工作内容。

然而,时不时地与其它团队(如果有的话)的成员进行接触,可以避免被困在泡沫中,促进知识共享。如果你对某些主题不确定,可以向领域专家寻求帮助。这通常包括性能、架构或安全相关的变更。

自动化

代码审查不应涉及的一个问题是编码标准是否得到遵守。在第 7 章 "代码质量工具" 中,我们介绍了自动实现这一目标的必要工具,在第 11 章 "持续集成" 中,我们将它们集成到了 CI 管道中。

确保只有那些通过了所有检查(如代码嗅探器、代码分析器和自动测试)的拉取请求才会被审核。否则,你将在根本不应该讨论的主题上花费大量时间。

避免冗长的代码审查

需要审核的代码变更应该有多少行?研究表明,200 到 400 行应该是最多的,因为随着时间的推移,审核人员的集中度会降低。因此,应尽量将单个变更保持在相对较小的范围内。对于一长串的差异,审查员也更有可能抽出时间审查较小的变更。

代码审查,即使是较小的审查,也需要时间,而在这段时间里,审查员将无法编写代码。但应该花多少时间呢?这同样取决于你的设置。一个好的大致数字是最多 60 分钟,以避免审查员疲劳。要留出足够的空间让审核人员逐行审核代码。审核应被视为日常工作的一部分,否则很快就会成为一种负担,因此任何人都不应匆忙完成审核。

stay human

如何提出反馈意见是评审成功的关键。注意语气,尽量避免指责,如 "这是错的!"或绝对不允许的 "这是愚蠢的"。开发人员,尤其是经验较少的开发人员,不要急于让自己的代码接受评审。

请记住,人类会阅读你的注释。如果你从 "我 "的角度写评论,通常效果会很好,例如,"我不明白这一行,能请你解释一下吗?"或 "我想我们也可以这样做…​…​"。

不要忘记利用评论来表扬做得好的部分。一句简短的 "好主意!"或 "我很喜欢你的方法 "或 "谢谢你的代码清理 "就能表达你对其它开发人员工作的赞赏,并提高他们的积极性。

通常,代码审查是通过在你使用的 Git 平台上写评论来完成的。当然,你也可以面对面地进行代码审查。有些开发人员更欣赏直接反馈而非评论,因为书面文字缺少很多元信息,比如语气或面部表情。

不要过度,但也不要大意

牢记 Pareto 原则,不要矫枉过正。也许代码中仍有一些小部分是你想修改的,但它们并没有明显的错误,因为它们符合团队的所有标准。编程仍然是个人风格的问题,在代码审查中进行无休止的讨论只会导致挫败感,而不会带来更多益处。

但不要接受会降低整个系统健康度的变更。如果你确信某项变更有害或违反了编码准则,就一定不能批准该变更。如果有疑问,请让其它开发人员参与进来。

拥抱变化

最后,如果你觉得在评审中讨论的问题应该成为指南的一部分,请记下来,并在下次团队会议上讨论,不要直接提及其它开发人员。也许你是对的,今后会修改指南以避免出现这个问题。

但你也可能是错的,团队的其它成员并不觉得这是一个问题。如果你不能提出令人信服的论据和例子,你也必须接受这些决定。

确保完成代码审查

在紧张的日常工作中,面对高优先级的错误修复和紧迫的最后期限,很容易忘记进行代码审查。幸运的是,所有 Git 服务提供商都能在这方面为你提供帮助:

  • 强制审核:大多数 Git 仓库服务都可以配置为只有在修改获得至少一次批准后,才允许将修改合并到主分支。你一定要启用这一功能。

  • 轮流审核:如果团队人数较多,尽量不要总是让同一个人审核。有些工具甚至允许为你随机选择审核员。

  • 使用核对表:清单已被证明是有用的,所以你也应该使用它们。为代码审查中需要注意的所有方面建立一个核对表。在下一节中,我们将向你介绍如何确保它得到使用。

完成的定义

如果你使用敏捷方法工作,你可能已经听说过 "已完成" 的定义。在这里,团队就任务完成前应完成的行动列表达成一致。

典型的 "已完成" 定义包含检查,如测试是否已编写或文档是否已更新。你也可以将其用于代码审查。

同样,我们的 Git 工具也提供了拉取请求(也称为合并请求)的模板。这些文本将用于自动预填拉取请求的描述。

具体如何操作取决于你使用的软件,因此我们无法在此给出准确的说明。不过,下面的文本为你提供了一个示例:

# Definition of Done

## Reviewer
[ ] Code changes reviewed
    1. Coding Guidelines kept
    2. Functionality considered
    3. Code is well-designed
    4. Readability and Complexity considered
    5. No Security issues found
    6. Coding standard and guidelines kept
[ ] Change tested manually
## Developer
[ ] Acceptance Criteria met
[ ] Automated Tests written or updated
[ ] Documentation written or updated

检查表中包含哪些内容由你和你的团队决定。如果作为模板使用,这些项目将始终默认出现在拉取请求描述中。审核人员和开发人员都可以使用该清单,以免忘记在批准拉取请求并将其合并到主分支之前需要完成的工作。

有些工具(如 GitHub)使用 Markdown 风格的标记语言来制作这些模板。它们会在浏览器中将复选框(每个项目前的两个方括号)显示为可点击的复选框,并跟踪它们是否被点击。就是这样!不费吹灰之力,你就建立了一个简单易用的有用核对表!

代码审查结论

我们希望本节内容能让你深入了解代码审查对团队和你自身的益处。既然可以毫不费力地引入代码审查,那就值得一试。本节中的最佳实践将帮助你避免代码审查可能带来的一些问题。

但与往常一样,它们也有一些缺点:审查需要花费大量时间,而且可能导致团队成员之间的冲突。但我们相信,花费的时间会得到很好的回报,因为积极方面远远超过消极方面。无论如何,冲突很可能会发生,而审查只是发泄情绪的工具。如果你在一个团队中工作,这种情况是无法完全避免的,但应及早与你的经理沟通。他们的工作就是处理这类问题。

在本章的最后一部分,我们将详细介绍设计模式。它们可以作为解决软件开发中一般问题的指南。