检查CL
在知道了代码评审要关注哪些东西之后,如何有效地进行跨文件代码评审呢?
代码变更有意义吗?它们有没有良好的描述?
先看一下代码变更中最重要的部分,它整体设计得如何?
按照适当的顺序检查CL的其余部分。
第一步:从整体查看代码变更
先看一下CL描述,看看这个CL做了些什么。做出这个变更有意义吗?如果这个变更是不必要的,请立即做出回复,并解释为什么不应该发生这个变更。在你拒绝这样的变更时,可以向开发人员建议他们应该做些什么。
例如,你可以说:“看起来你在这方面做得不错,谢谢!不过,我们正打算移除这个系统,所以现在不想对它做任何修改。或许你可以重构一下另外一个类”?
注意,评审人员在拒绝一个CL并提供替代建议时要做得很有礼貌。礼貌是很重要的,因为作为开发人员,我们要彼此尊重,即使可能意见不一致。
如果有很多CL是你不希望出现的,就要考虑重新调整开发团队或外部贡献者的开发流程,以便在开发新的CL之前进行更多的沟通。提前告诉人们哪些事情不要做,这比等他们做完了这些事情再把它们扔掉或者进行彻底重写要好得多。
第二步:检查CL的主要部分
找到CL的主要文件。通常一个CL会有一个包含了主要逻辑变更的文件,也就是CL的主要部分。先看看这些主要部分,有助于了解整个上下文,加快代码评审速度。如果CL太大,以致于你无法确定哪些部分是主要的,可以询问开发人员,或者让他们把CL拆分成多个CL。
如果CL的主要部分存在严重的设计问题,要立即回复开发人员,即使你还没有时间检查CL的其余部分。这个时候检查CL的其余部分可能是在浪费时间,因为如果主要部分存在严重的设计问题,那么其他部分就变得无关紧要了。
为什么要立即回复开发人员?原因有二:
· 开发人员在发出一个CL之后会继续开始后续的开发工作。如果你正在评审的CL存在严重的设计问题,他们也需要重写后续的CL。所以,最好赶在开发人员在有问题的设计上花费不必要的时间之前告诉他们。
· 大的设计变更比小的变更需要更长的时间。为了让开发人员能够在截止日期之前提交代码,同时又能保持代码库的质量,要尽早让他们开始重写工作。
第三步:按照适当的顺序检查CL的其余部分
在确认整体CL没有严重的设计问题之后,试着按照某种逻辑顺序来检查其他文件,确保不会错过任何一个需要检查的文件。通常,在你检查完主要文件之后,按照代码评审工具显示它们的顺序来浏览每个文件就可以了。你也可以在检查主要代码之前先查看测试代码,这样可以对代码变更有一个大致的概念。