评审GitHub上的3500多个拉取请求，我学到了什么

我写这篇文章是为了帮助大家更好地完成代码评审工作，因为我觉得这项工作被误解了，或者被认为是无用功。

什么是代码评审？

对于许多人（也许是绝大多数的开发人员或数据人员）来说，是这样定义的：

这项任务就是通过分析代码来检测错误和 bug，可能是功能问题（代码没有做它们应该做的事情）、回归（新代码破坏了已有的行为）、语法错误（遗漏了符号、调用了未定义的变量）或项目合规问题（缩进、命名规范、语言标准）等。

也就是说，评审人员评判同事甚至是外部开发人员（在开源项目中）的工作，并且必须指出其中所有负面的东西。

如果这是你期望从同事那里得到的代码评审，那么你很难找到一个评审志愿者。

在不质疑这个定义的情况下，我发现它其实是有局限性的，甚至还有点问题——我将与你讨论代码审查的其他作用。

我要求或做的大多数代码评审都是在这种情况下完成的。

例如，在 2019 年的 PrestaShop 项目中，没有哪一份被接受的代码贡献逃过了我的眼睛。

根据 GitHub 的统计，这些占了我贡献的 40%，而在我看来，这占了我工作时间的 60% 到 70%。

为什么会这样？因为我们都知道，不是所有的东西都会被记录下来，不是所有的东西都会被测试到，但必须有人能够维护开发分支上已经接受的所有东西。

说真的，认为贡献代码的人会在需要他们的时候出现，这是一种妄想。在一个大多数开发人员每两年就会换工作的世界里，这种想法太天真了。

在代码评审过程中，我可以问一些关于背景或者为什么要这么做的问题，如果看到了一些脏代码，就要求做出解释。我的评审记录中经常会出现“为了……”，因为当你不得不回头去理解为什么“那个混蛋”会这样写代码时，了解当时的背景会很有帮助……

在代码评审中，我越来越多地融入了“非开发人员”的意见。我会拿出一段代码，解释它的里里外外，以此来获得他们的想法。

例如，当添加了一个新特性时，我意识到新的实现可能会对另一个特性产生影响。这时，我会告知产品团队的人，向他们解释我的解决方案的优缺点，并请他们做出决定。

理想情况下，我希望所有参与评审的人都能参与进来——架构师确认新的实现不会破坏已有的架构，内容经理对内容进行验证，开发人员检查是否存在代码错误或做出进一步优化，“产品 /QA”经理确认新特性符合预期……有了评审工具，我们可以让每个人都参与到代码评审过程中。

PrestaShop 项目就是这么做的，根据贡献的不同，会有以下这些角色参与：

• 负责验证国际化的人员；

• 提供意见的社区成员；

• 至少一到两个开发人员；

• 一个来自产品团队的人，因为必须确保产品的一致性；

• 最后是 QA 团队，如果没有他们，不能接受任何代码！

很多人围绕着一段代码展开交流，这很好。

当然，这始终是代码评审的目标之一。确切地说，代码总是可以改进的。问题是，什么时候才足够好？

只要代码具有足够的可扩展性，被测试覆盖到，并且不存在明显的 bug 风险，那就很好了。

对于我来说，更重要的是命名、结构和文档——一致性是必不可少的，特别是对于难以理解的大型和老项目来说。

有时候，良好的命名或把代码写成良好命名的函数（并且只有一个职责）可以减少对文档的依赖。

我个人有一个原则——没有被记录的东西就不存在。

我还禁止 PrestaShop 中出现任何带有布尔参数的函数——不出三周，肯定没有人知道这个参数发生了什么变化，更重要的是，如果函数的行为取决于布尔值，就表示它有两种行为。

我通过代码评审改进得越多，就越是要求使用包含较少参数（零到一个）的短函数和具有特定职责的短类。

大型遗留项目的主要问题不是拥有太多的文件，而是很难了解每个文件的作用……

就像 Symfony、API Platform、PrestaShop 或任何其他严肃的项目一样，配置好你需要的检查和修复工具，尽量不要自己制定标准，让工具来处理它们就可以了！

PrestaShop最终使用的代码评审

如果说有什么事会让我恼火，也可能会让所有人恼火，那就是看到一个评审人员浪费了一个半小时在一段代码上，结果只提出说是因为空格、逗号或大括号没有对齐之类的问题 。

下面是我在过去几个月里做的 / 理解的一些有积极影响的小事情。

• 指出错误和你认为做得不错的事情；

• 在任何情况下都要感谢评审和验证你代码的人；

• 在任何情况下都要感谢做出贡献的人，即使你拒绝了他们的代码；

• 说明你贡献的代码的背景！我们说 RTFM（去阅读手册），而不是 RTFC（去阅读代码），是有原因的：代码并不会包含所有的业务逻辑知识，即使你是一个 DDD 成瘾者；

• 永远不要说“这太愚蠢了……”，如果你必须做出强烈的批评，可以说“这段代码或这个代码块”，你要针对的是工作，而不是人；

• 我坚持认为善意和幽默是有帮助的；

• 如果有不清楚的地方，不要犹豫，把代码拿到你的机器上尝试运行。

• 我越来越经常问我的同事：“你检查过它是否可以正常运行了吗，还是你只是看了下代码而已?”“通常情况下，一些代码需要检查其可能存在的诱导性行为。这不是额外的工作，而是预期工作的一部分；

• 评审代码比编写代码更有价值。为什么？因为一个代码贡献，只要它不被接受，就没有价值——每周有 20 个贡献，10 个被接受，比每周有 70 个贡献，5 个被接受要好。无论是什么项目，这个规则总是有效的。