code review的几条规则

最新推荐文章于 2022-07-28 14:37:13 发布

weixin_47143210

最新推荐文章于 2022-07-28 14:37:13 发布

阅读量191

点赞数

文章标签： java 编程语言人工智能 python 面试

原文链接：https://mp.weixin.qq.com/s?__biz=MzAwMDU1MTE1OQ==&mid=2653552269&idx=1&sn=7feafb0518d10499293e494820cadbb2&chksm=813a6f15b64de6032caf9bf781c6ffbedc0c37b235ef5b7804914f5cd56c98a668e93bc3f078&scene=21#wechat_redirect

版权

规则1

每个 PR 审查至少需要 2 个同组开发者的批准，管理者的审批不统计。

首先要注意的是，由于我所在的是一个 3 人团队，这是最理想的。所有的修改 3 个开发者都 100% 知情。如果团队规模更大，情况可能会有所不同。你所追求的是邓布利多和死亡圣器(Dumbledore Horcruxes)的情况，如果你死了，至少有 2 - 3 人知道死亡圣器的事。我们的团队现在有 5 个开发者，2 个老手，1 个新人，2 人中等水平。我们仍在努力坚持每个人都要审核每个人 PR 的模式，但至少 1 个老手 + 1 个其他开发者的模式，将来可能更可行一点。

管理者审批为什么不算数？你也许认为“我是经理，也是个了不起的程序员，我的审核完全算数”，经理们的 review 确实有帮助，有大把管理者也是优秀的程序员，当他们有时间 code review 时，他们往往会有很好的建议或想法来帮助其他开发者。

但你必须考虑管理者花在编码上的时间有多少？如果超过 50%，那么他们也可以算作开发人员，这种情况作为审核者也是可以的。但是在我们公司，经理只是偶尔做一些非常偶然的编码工作，因此，主力还是要靠开发人员自己来说话。我不希望看到经理和开发人员之间的无限审批周期，一个管理者不断地审批一个个开发人员的PR，即使团队里还有 3 个开发人员有大把时间可以来做这个工作。

规则2

每个 PR 必须有一个好的描述，通过阅读描述，审核者能够理解代码的目的是什么。即使有一个 Jira ticket 或需求页面，也必须满足此要求。

没有描述的 PR —— 在我的团队中，这种情况永远不会通过。最好的情况是：他们没有写，是因为有另外一个 Jira ticket 页面有很好的描述。最坏的情况是什么也没有。

遇到没有描述的 PR，审核员有 2 个工作：

通过阅读 PR 中的代码修改，了解代码的作用。

审阅者试图夜观天象来判断代码是否做了它应该做的事情？

如果没有清楚地说明代码要做什么，就不可能审查它的正确性。你根本无法知道什么是正确的，你的操作是基于你认为正确的假设。

规则3

PR 必须有足够的单元测试和集成测试覆盖。

理想情况下，审阅者可以很容易地看到测试用例列表。

如果测试覆盖率已经存在，或者由于某种原因测试覆盖率不完整（工作被拆分到另一个依赖的 ticket），PR 描述中应该提到这一点。

给出好的 PR 评审意见很难、也很费时间。这是因为一旦你明白了代码应该做什么，你就必须停止审查并去查看你想要的测试覆盖率。你要合适的测试用例列表，并决定是哪种类型的测试：单元测试/集成测试/端到端。

你还必须考虑是否有可能对生产环境产生影响，以及是否需要在部署前对真实数据进行测试。

现在你知道了这些，你再去回顾一下 PR，看看他们是否涵盖了你所想的一切。理想情况下，这是一个混合体：你想到了他们没有想到的东西，他们也想到了你没有想到的东西，然后你们一起做得很好。这就是能够很容易看到 PR 中涵盖的测试用例列表的地方（相对于阅读 100 行或 1000 行的测试，必须弄清楚哪些用例是真正被测试的）。

注意，如果遵循了规则 2，你可以通过阅读描述就知道代码应该做什么。所以你可以在审查任何实际的代码之前，就可以完成审查测试覆盖率的步骤!

我个人喜欢把所有这些东西：相关的测试，真实数据测试，对线上客户端影响，生产环境配置更改都放在 PR 描述中。或者其他合适的地方，在一个地方统一查看。

规则4：

如果 PR 是一个 bug 修复，它必须包含一个测试，如果这个 bug 被还原，这个测试就会报错。

我以为这很明显，但就像大多数明显的事情一样，事情常常不是这样。

假设有一个打字错误的 bug，你用 != 代替了 ==，当时是晚上10点，你不应该去写代码，但你觉得那天晚上你想多做一些事情。于是你打开了一个 BUG 修复 PR，把 != 改成了 ==，有一个好心人也在工作，批准了它，然后把它送到了主分支。

三天后，你的好友 Duke 的 PR 也终于准备好了，他以为这是一个小功能，没有把它拆成小的 ticket，但事实并非如此，现在他有一个 2000 行的PR，他恳求团队进行代码审查。

Duke 执行了代码合并，代码冲突的地方你把 == 换成了 !=，Duke 有些疲惫，他没注意到这些细节，因此 != 又回来了。

对，因为没有测试来捕捉它，bug 就又回来了。

这是个虚构的例子，但如果你长期观察，这种情况经常发生。把团队的 bug 铺开 3 年，你会看到那些没有被测试捕获的 bug 又出现了。

总结

对，就这些! 实际只有 3 个规则，第 4 个更像是常识性的东西。