代码审查和不良编程习惯

有时候，做为一个程序员，我觉得我的职业生涯会被我开发软件使用的开发工具和技术架构明显的分割成几个阶段。一部分是因为使用的编程语言——在大学时是Smalltalk，在Gog Creek公司是C#和Python，而另一方面是开发工具。我在Fog Creek公司里工作了8年，在那里，我们有一个非常固定的技术架构：bug管理、客户支持和文档管理用FogBugz；开发管理用Trello；代码审查用Kiln；版本控制用Mercurial；编码用Vim和 Visual Studio ；持续集成用我们的内部工具Mortar；随着时间的流逝，这些工具在慢慢的变化，但变化从来都是缓慢逐步的，一个组件一个组件的。所以，我的工作流程和工作效率一直没有巨大的变化。

大概一个月前，我加入了Knewton公司，整个技术架构一下子完全变了。Visual Studio换成了IntelliJ；Mortar换成了Jenkins；Mercurial换成了Git；FogBugz换成了JIRA。

也许你会觉得这会让我头大，还会有些不知所措，但事实上并不是这样，这些工具的改变并没有对我的工作流程产生多大的影响。我发现Git和Mercurial惊人的相似，JIRA基本上是一个半成品的FogBugz，而IntelliJ算是和Visual Studio差不多吧。也许我需要从新学习一些快捷键和了解按钮的位置，但事实上我的开发模式没有实质的变化。

但有一个例外：我不喜欢使用Gerrit做代码审查。不喜欢它的原因并不是它的程序写的很烂；不喜欢的原因是它的流程会鼓励一种不良编程习惯。

Knewton公司对代码审查非常、非常的看重。这非常好，因为我也是这样，而且我开发过整套关于代码审查的工具。所以，我的意思绝对不是反对代码审查。

而且，Gerrit的设计跟最初的 Kiln 原型的设计几乎完全一致。代码审查的实施有两种基本的方式：pre-merge，是指在代码进入主代码库之前进行代码审查。和post-commit，是指之后审查。新版本的Kiln对两种方式都支持，但在2008年，当Tyler和我通过一个项目——也就是Kiln的前身——在Django Dash中取胜时，我们俩都认同pre-merge的工作流程。直接提交到主代码库是不允许的；你需要先创建一个审查区，把修改的代码放进去，讨论，然后，等待批准，系统会自动合并这些代码。这一种是我最欣赏的工作流程，所以Kiln一直支持这种方式(通过“Read and Branch”权限)，而巧的事，这也是Gerrit唯一支持的方式，按理说我应该喜欢它才是。

我差一点就喜欢它了，但问题出在一个致命问题上：代码审查的粒度。在Kiln中，审查是基于被修改的相关代码。而在Gerrit里，审查是基于单次代码修改提交。在Kiln中，一个单一审查会涉及很多次代码提交，审查的批准和拒绝是整体的，而Gerrit里审查的是一次孤立的提交。

这两种方法模式在各自的阵营里都有大量的受欢迎的系统实现。GitHub和Bitbucket都和Kiln一样都属于“批量提交”审查阵营，而Review Board, Barkeep, 和 Phabricator 都加入了“单一提交”审查阵营。所以，情况并不是我所说的某一种方式、某一款软件是对的，而其它都是错的。但我还是要坚持说，批量审查的方式是对的，而其余的都是错的，因为单一提交审查系统在鼓励一种不良编程习惯。

单一提交审查系统有两个最根本的问题：

1. 它在向你暗示各个修改提交之间没有关联。经常的，每当我实现一个新功能时，我都会有三个步骤：首先，重构现有的代码，让代码整洁，方便添加新功能；接下来，加入新功能；最后，写单元测试代码。功能越复杂，各个步骤里越有可能各自包含多个逻辑步骤。如果能将多个不同的提交放的一个代码审查中进行，那你就可以简单的将这些修改分组提交。但如果使用的是单一提交审查系统，那就是在迫使我将所有修改全部完成后进行一次全量提交。这样一来，重构的代码，新添加的代码，都混在一起，让人非常不爽，而且在审查过程中需要我付出大量额外的精力来指出各部分代码都是干嘛的。你也许会争辩，说你可以把修改的代码拆分提交，每一个提交对应一次审查。但事实上这样做会更糟。最好的情况下，你可以把测试程序和新功能代码分开提交，可以把重构代码和后加代码分开提交，但真正的问题是，众多的单一修改审查系统都怂恿对某个提交在孤立的状态下进行批准，这完全会和你的愿望相反。于是，“一次提交一次审查”的折中就从“麻烦”变成了“危险”。的确不是一种改进。
2. 它在怂恿你隐藏历史记录。版本控制系统的最重要的功能就是告诉你代码演变的历史、是如何变成今天这个样子的。我经常会查看昨天代码是什么样的，上周二下午2点代码是什么样的，期间发生了什么变化。很多时候是因为我发现代码以前好用而现在不行，我想知道为什么。而更多时候，我是想知道为什么会对代码做这样的修改。关联的上下文是什么？动机是什么？如果你总是保持所有代码一次提交——为了审查，那我就丧失了很多历史信息：所有我能找到的就是一次完整软件的一次提交，完全没有开发过程中的过程信息。

这就是我为什么对Gerrit极度失望的原因。并不是Gerrit是一个糟糕的软件，而是他在鼓励一种在使用版本控制时不良的开发习惯。这就是为什么所有工具中唯独不喜欢它的原因，是唯一让我对放弃Kiln感到失望的系统。