技巧提示代码审查的八个技巧

审查可能是一项艰巨的任务,尤其是当您之前没有做过许多审查时。 准备至关重要-开始寻找之前,请先了解您的需求。

这是不好的时光吗?

在开始检查任何代码之前,请问自己是否合适。 这有两个方面,答案取决于您团队的流程和您自己的时间表。

首先,代码是否准备好进行审查? 在审查代码之前需要完成哪些工作,这些工作都已经完成了吗? 例如,在FlexMR,我们通常在任务通过验收测试并且任何相关的PR也都已被审查和合并之前,才审查代码。 当然也有例外。 即使是非正式的代码审查过程,也可以成为协助寻求帮助的同事的好工具。

其次,您现在有时间进行审查吗? 所需时间可能难以估计。 虽然PR的大小和所需的时间之间存在相关性,但有太多其他因素要考虑将其用作唯一变量。 您一定不要急着进行代码审查。 它不会对您,您的同事或代码有任何帮助。 如果您知道要进行审核,请留出合理的时间进行审核。 屏蔽每天的一部分(例如早晨的第一件事,或午餐时间后立即休息),并利用这段时间进行审核,通常会很有帮助。

使用明智的代码审查工具

我们不得不通过电子邮件进行评论的日子已经一去不复返了。 有大量可用的工具,因此请找到适合您和您的团队的工具。 考虑开放您的评论,以便团队中的其他成员(不仅是作者和评论者)也可以观察到该过程。 这使每个人都有机会看到正在进行的更改,以及编写和审查这些更改的想法。 考虑一下如何解决问题,然后将其与提供的解决方案进行比较,这是一个很好的学习练习。

在FlexMR,我们使用其PR审查工具通过GitHub进行审查。 它没有最广泛的功能集,但是对我们来说确实很好。 它直接插入我们的开发和CI流程,并且我们通过Slack将警报和提醒发送给我们。

如何积极地要求变更

一些团队具有对抗性的,有时甚至是敌对的代码审查实践。 也许在您不了解合作者的项目中这样做是有争议的,因为您无法确定作者的意图。 考虑开源或具有外包开发的项目。 对于内部团队而言,则不需要它。 如果您不信任您的团队将项目的最大利益考虑在内,那么您就会遇到麻烦,因为代码审查中的脾气暴躁是无法解决的。

别让您的同事以某种方式写一段代码来让您感到不高兴。 除非他们的日子不好过,否则他们可能会付出相当大的努力。 如果最终结果不完全正确,则可能是由于经验不足,知识缺失或他们只是犯了一个错误。

那么,您如何指出需要改进的代码呢? 冷静地解释问题所在; 为什么有问题(如果不明显); 然后提供有关如何解决它的建议。 根据问题的不同,建议可能很简单,只需将它们链接到相关文档即可。 对于非平凡的问题,代码示例可能会很有帮助。 如果这是一个特别复杂的问题,也许甚至愿意与他们讨论。

最重要的是,让您的意见公开讨论!

1.我在看什么?

在开始查看代码之前,请确保您至少对代码尝试解决的问题有基本的了解。 如前所述,我们使用GitHub审查PR。 每个打开的PR都有问题和解决方案的快速摘要。 它还具有指向可以看到解决方案演示的链接,以及指向带有问题原始描述的问题跟踪器的链接。

如果尚不清楚代码应执行的操作,则需要请作者进行澄清。

2.空格变化和其他多余

理想情况下,PR中的唯一更改应该是解决眼前问题完全必要的更改。 多余的更改(例如空格调整)是一个很好的示例,它对提交历史记录的完整性具有负面影响。 能够将更改与更改的时间和原因相关联,对于跟踪错误和大型应用程序的常规维护至关重要。 GitHub的Atom具有一项功能,可以在保存文件时从文件中剥离所有不必要的空格。 此功能默认为“开”,因此使用时请务必小心。 请考虑关闭此功能。

另一方面,不应引入新的空白问题。 如果您在代码审查期间看到任何内容,请指出来,以便作者修复。

您在Tabs vs.Spaces辩论中的立场如何? 没关系。 作为一个团队,只需选择一个并坚持下去即可。

3.安全编码实践

信息安全是软件开发中的热门话题。 作为开发人员和代码审阅者,安全性始终在您的脑海中。 在审查代码时,请考虑一下那些有恶意的人可以利用它的方式。

至少,请确保您赶上OWASP十强。 了解这些漏洞在您使用的技术堆栈中的外观,并了解如何防范这些漏洞。 在审核期间要对他们勤奋。

您选择的语言/框架可能具有可用的工具,可以帮助确保您的代码安全。 例如,Ruby on Rails具有Brakeman,Python具有Bandit。 利用它们,但不要依赖它们为您做任何事情。

4.足够的测试范围

是否已对所有代码更改进行了适当的测试? 新功能意味着新的测试。 错误修复意味着新的测试。 新的UI或API端点意味着新的测试。 通常,如果要添加或更改代码,则应该在代码旁边进行一些测试。 但是,不应随意添加它们。 测试应该是有用的-它的核心是,它们需要证明更改正在按预期进行,并防止将来发生退化。

您的测试策略应由您的团队决定。 有很多不同的方法,因此您需要进行讨论并确定最适合您的方法。 一旦您的团队制定了计划,请确保在代码审查期间遵守该计划。

5.丢失的连接

有些更改应始终成对出现。 其中一些可能真的很容易错过评论。 例如,在Rails应用程序的上下文中:

  • db/schema.rb更改应具有相应的迁移。
  • Gemfile更改应对Gemfile进行相应的更改

如果您在接受测试之前进行代码审查,那么您还需要寻找更明显的代码。 模型上的新方法将需要新的单元测试。 新的控制器可能需要在config/routes.rb一个新条目。

不要忘记文档! 如果已记录的代码片段的行为发生了变化,则文档也需要进行更改。

6.对应用程序性能的潜在影响

让您的应用程序尽快工作通常被认为是一个好主意。 这对用户来说更好,对您的系统基础架构也更好。

在寻找可以优化代码的方法。 调用数据库是一个很好的起点。 N + 1个查询会大大降低速度,并且通常很容易看到并建议修复方法。 了解您选择的数据库何时将锁定表以执行读取或写入操作,以及如何在应用程序中进行操作以最大程度地减少由此带来的负面影响。

如果代码正在处理大量数据,那么它在检索和保存数据方面是否明智? 如果一次检索成千上万条记录,那么以较小的批次进行记录会更好吗? 以及检索实际数据所需的时间,如果您使用的是ORM,则还需要考虑初始化这些对象的时间和内存要求。

如果实施不当,JOIN可能会成为另一个杀手。 检查JOIN本身是否有用。 有时可以更容易,更快捷地运行单独的查询。 自然地,它取决于JOIN的复杂性以及您对数据的处理方式。 只需注意,当它怀疑基准时。

也不要忽略与数据库无关的代码。 熟悉如何用您选择的语言编写快速代码,并了解如何在您的评论中应用它。 大多数任务可以通过多种方式解决,有些任务比其他任务更快。 您不一定会在第一次尝试时就以最快的速度淘汰出局,但是如果您努力学习一些新技术,您的代码将得到改善,并且在检查同事的代码时,您会说出更多有趣的事情。

查看您的技术堆栈是否具有可以集成到工作流程中的任何性能分析工具。 通常需要注意的是:充分利用它们,但不要依靠它们为您做这件事。 对于Ruby,请查看Bullet和Fasterer。

关于优化需要牢记的一件事-您可能不需要从代码中挤出所有最后一点性能。 通常,当然,您希望您提交的代码能够快速运行,但是您必须在这与对可维护性的潜在影响之间取得平衡。

7.遵循团队和语言编码约定

每种语言至少都有一个样式指南,每个团队都有自己的变体。 选择一种风格并坚持下去。 也可以考虑添加一些自动棉绒以在其进入审核阶段之前实施它。 这样可以节省您的团队时间。

Linting只能在可见的代码上强制使用样式指南。 作为人工代码审查员,查找未编写的代码您的工作。 适当使用设计模式就是一个很好的例子。

例如,如果PR向模型添加了很多方法,请考虑是否应将它们重构为服务对象。 如果要向控制器添加其他端点,也许是时候添加更多控制器了?

另一方面,如果PR实施了团队以前从未使用过的激进设计模式,则应提起讨论。 尝试新想法很棒,应予以鼓励,但是应该在团队知识的基础上进行,因为他们早晚需要修复其中的错误。

您的团队是否有命名流程? 命名很难,但是保持一致是一件好事。 如果您的几个模型可以有一个“名称”,那么所有这些模型都称为name吗? 还是其中的一些title ? I18n键呢? 或缓存键?

理想情况下,您需要事先决定所有这些并记录下来。 然后,您既可以作为审阅者,又可以作为编码者来引用它。

8.分享知识的机会

代码审查是双方相互学习的绝佳机会。 忘记了学习是初级开发人员向高级开发人员学习的单向道路的想法。 您可以通过关注并提出问题来学习新事物。 有趣的人编写有趣的代码,有趣的代码导致有趣的对话。

复习了很多代码之后,毫无疑问,您将经历过笔者以您原本不会考虑的方式解决给定问题的经历。 使用编程,不仅有正确的方法和错误的方法。 有很多正确的方法,还有更多错误的方法。

如果您发现确实有问题,请不要仅仅说需要进行更改。 指出问题所在,并提供有关如何使其正确的建议。 如果您觉得一段代码完全不合时宜,请给作者一些时间,以帮助他们将代码移到应有的位置。 也许问他们如何找到解决方案。 这对您来说可能更有意义。 他们可能会考虑一些意外情况或不可靠的数据。 也许他们只是弄错了。 无论哪种方式,你们俩都有机会从中学习。

如果代码看起来不错,是否有任何方法可以改进? 如果初级开发人员一直在进行一些持续的出色工作,也许是时候给他们带来更多挑战了。 是否可以优化他们的算法? 他们是否在充分利用数据库查询? 他们可能准备学习一些其他技能,或者加倍学习他们已经知道的知识。

如果您采用开放友好的代码审查方法,通常会发现自己与更广泛的团队共享有趣的代码。 这可以导致一些愉快的对话。 在FlexMR,这些对话经常导致对某些问题采取新的方法,例如同意使用某些设计模式或试用新工具。 他们还引发了一些有趣的对话,涉及我们如何解决假设情况,例如如何在不使用正则表达式的情况下验证密码是否符合安全要求; 或者我们如何在拒绝它们的数据库系统上支持Emoji unicode字符。


  • 别混蛋-友好的代码评审对团队和项目都更好。
  • 事先知道您应该寻找什么。
  • 事先确定重要的事情。 您的测试策略和编码风格应优先考虑。
  • 您的任何评论都应公开讨论。 尽可能最好地提问和回答,每个人都会受益。
  • 互相学习。