既然您已经知道Code Review应该Review什么,那么管理跨多个文件的评审的最有效方法是什么呢?
- 这种变更合理吗?它有一个好的描述吗?
- 首先查看最主要部分的变更。它的整体设计好吗?
- 按照适当的顺序查看CL的其余部分。
第一步:从更广阔的角度看待变更
查看CL的描述和CL的一般功能。这种变更合理吗?如果这个变更一开始就不应该发生,请立即解释为什么不应该变更。当您拒绝这样的变更时,向开发人员建议他们应该做什么也是一个好主意。
例如,你可以说“看起来你在这方面做得不错,谢谢!但是,我们实际上是要删除您在这里修改的FooWidget所在的系统,所以我们现在不想对它进行任何新的修改。不如重构我们的新BarWidget类?”
请注意,评审人不仅拒绝了当前的CL并提供了一个替代的建议,而且还很有礼貌。这种礼貌是很重要的,因为我们想要表现出我们作为开发者彼此尊重,即使我们意见不一致。
如果您得到了多个您不希望进行变更的CLs,那么您应该考虑为外部贡献者或您的团队重新制定开发流程或发布的流程,以便在编写CLs之前进行更多的沟通。在人们做了大量的工作,最好在不得不扔掉或彻底重写之前,对他们说“不”。
第二步:检查CL的主要部分
找到一个或多个文件是这个CL的“主要”部分。通常,有一个文件的逻辑更改数量最多,它是CL的主要部分。首先看看这些主要部分。这有助于为CL的所有更小的部分提供上下文,并且通常会加速代码检查。如果CL太大,您无法确定哪些部分是主要部分,请询问开发人员应该首先查看哪些部分,或者请他们将CL分成多个CL。
如果您看到CL的这一部分存在一些主要的设计问题,您应该立即发送这些评论,即使您现在没有时间来查看CL的其余部分。
实际上,检查CL的其余部分可能是浪费时间,因为如果设计问题足够严重,那么很多其他正在检查的代码将会被删除,不管怎样都无关紧要。
有两个非常重要原因需要我们立即评论这些关于主要的设计的问题:
- 开发人员通常会发送一个CL,然后在等待评审期间立即根据这个CL开始新的工作。如果您正在审查的CL中存在主要的设计问题,那么他们也将不得不重新处理后面的CL。你想在他们在有问题的设计上做太多额外的工作之前避免这种情况的发生。
- 主要的设计变更比小的变更需要更长的时间。开发人员几乎都有最后期限;为了在最后期限前完成这些工作,并且在代码库中仍然有高质量的代码,开发人员需要尽快开始对有设计问题的CL进行重写。
第三步:按照适当的顺序查看CL的其余部分
一旦您确认了CL整体上没有重大的设计问题,那么请尝试找出一个逻辑顺序来检查这些文件,同时确保您不会错过检查任何文件。通常,在您浏览完主要文件之后,最简单的方法就是按照代码审查工具显示文件的顺序浏览每个文件。有时,在阅读主代码之前先阅读测试也是有帮助的,因为这样您就会对变更应该做什么有一个概念。
下一章: Review的速度