高效代码审查的八条准则和十个经验

jopen 11年前

代码审查（Code Review)是软件开发中常用的手段，和QA测试相比，它更容易发现和架构以及时序相关等较难发现的问题，还可以帮助团队成员提高编程技能，统一编程风格等。
　　**1. 代码审查要求团队有良好的文化**
　　团队需要认识到代码审查是为了提高整个团队的能力，而不是针对个体设置的检查“关卡”。
　　“A的代码有个bug被B发现，所以A能力不行，B能力更好”，这一类的陷阱很容易被扩散从而影响团队内部的协作，因此需要避免。
　　另外，代码审查本身可以提高开发者的能力，让其从自身犯过的错误中学习，从他人的思路中学习。如果开发者对这个流程有抵触或者反感，这个目的就达不到。
　　**2. 谨慎的使用审查中问题的发现率作为考评标准**
码审查中如果发现问题，对于问题的发现者来说这是好事，应该予以鼓励。但对于被发现者，我们不主张使用这个方式予以惩罚。软件开发中bug在所难免，过度苛求本身有悖常理。更糟的是，如果造成参与者怕承担责任，不愿意在审查中指出问题，代码审查就没有任何的价值和意义。
　　**3. 控制每次审查的代码数量**
　　根据smartbear在思科所作的调查，每次审查200行-400行的代码效果最好。每次试图审查的代码过多，发现问题的能力就会下降，具体的比例关系如下图所示
（我想这是根据实现情况而定，如结合文档来评审，那么代码多一点也没关系）
　我们在实践中发现，随着开发平台和开发语言的不同，最优的代码审查量有所不同。但是限制每次审查的数量确实非常必要，因为这个过程是高强度的脑力密集型活动。时间一长，代码在审查者眼里只是字母，无任何逻辑联系，自然不会有太多的产出。
　　**4. 带着问题去进行审查**
　　我们在每次代码审查中，要求审查者利用自身的经验先思考可能会碰到的问题，然后通过审查工作验证这些问题是否已经解决。一个窍门是，从用户可见的功能出发，假设一个比较复杂的使用场景，在代码阅读中验证这个使用场景是否能够正确工作。
　　使用这个技巧，可以让审查者有代入感，真正的沉浸入代码中，提高效率。大家都知道看武侠小说不容易瞌睡，而看专业书容易瞌睡，原因就是武侠小说更容易产生代入感。
　　有的研究建议每次树立目标，控制单位时间内审核的代码数量。这个方法在我们的实践中显得很机械和流程化，不如上面的方法效果好。
　　**5. 所有的问题和修改，必须由原作者进行确认**
　　如果在审查中发现问题，务必由原作者进行确认。
　　这样做有两个目的：
　　(1)确认问题确实存在，保证问题被解决
　　(2)让原作者了解问题和不足，帮助其成长
　　有些时候为了追求效率，有经验的审查者更倾向于直接修改代码乃至重构所有代码，但这样不利于提高团队效率，并且会增加因为重构引入新bug的几率，通常情况下我们不予鼓励。
　　**6.利用代码审查激活个体“能动性"**
　　即使项目进度比较紧张，无法完全的进行代码审查，至少也要进行部分代码的审查，此时随即抽取一些关键部分是个不错的办法。
　　背后的逻辑是，软件开发是非常有创造性的工作，开发者都有强烈的自我驱动性和自我实现的要求。让开发者知道他写的任何代码都可能被其他人阅读和审察，可以促使开发者集中注意力，尤其是避免将质量糟糕，乃至有低级错误的代码提交给同伴审查。开源软件也很好的利用了这种心态来提高代码质量。
　　**7.在非正式，轻松的环境下进行代码审查**
　　如前所述，代码审查是一个脑力密集型的工作。参与者需要在比较轻松的环境下进行该工作。因此，我们认为像某些实践中建议的那样，以会议的形式进行代码审查效果并不好，不仅因为长时间的会议容易让效率低下，更因为会议上可能出现的争议和思考不利于进行如此复杂的工作。
　　**8.提交代码前自我审查，添加对代码的说明**
　　所有团队成员在提交代码给其他成员审查前，必须先进行一次审查。这次自我修正形式的审查除了检查代码的正确性以外，还可以完成如下的工作：
　　(1)对代码添加注释，说明本次修改背后的原因，方便其他人进行审查。
　　(2)修正编码风格，尤其是一些关键数据结构和方法的命名，提高代码的可读性。
　　(3)从全局审视设计，是否完整的考虑了所有情景。在实现之前做的设计如果存在考虑不周的情况，这个阶段可以很好的进行补救。
　　我们在实践中发现，即使只有原作者进行代码审查，仍然可以很好的提高代码质量。
　　**9.实现中记录笔记可以很好的提高问题发现率**
　　成员在编码的时候应做随手记录，包括在代码中用注释的方式表示，或者记录简单的个人文档，这样做有几个好处：
　　(1)避免遗漏。在编码时将考虑到的任何问题都记录下来，在审查阶段再次检查这些问题都确认解决。
　　(2)根据研究，每个人都习惯犯一些重复性的错误。这类问题在编码是记录下来，可以在审查的时候用作检查的依据。
　　(3)在反复记录笔记并在审查中发现类似的问题后，该类问题出现率会显著下降
　　**10. 使用好的工具进行轻量级的代码审查**
　　“工欲善其事，必先利其器”。我们使用的是bitbucket提供的代码托管服务。
　　每个团队成员独立开发功能，然后利用Pull Request的形式将代码提交给审查者。复审者可以很方便在网页上阅读代码，添加评论等，然后原作者会自动收到邮件提醒，对审阅的意见进行讨论。
　　即使团队成员分布在天南海北，利用bitbucket提供的工具也能很好的进行代码审查。

二、代码审查：大家都应该做的事情

　　Google的代码之所以优秀原因其实很简单：他们非常重视代码审查。代码审查并不是Google独有的，它被公认为是一个很好的（提高代码质量的）手段，很多人已经在日常开发中采用代码审查。但我还没有看到哪一家大公司（像Google这样）应用得如此广泛。在 Google，任何的产品或者项目代码在检入（代码仓库）之前都需要进行有效的审查。

　　每个人都要参与代码审查，而且这里我指的不是非正式的审查：它是软件开发环节中非常重要而且通用的规则。不仅是产品代码，所有的代码都需要进行审查。审查代码不需要投入很多的精力，但是（与不做审查相比）产生的效果却是天壤之别。

　　关于代码审查（code review），Jonathan Danylko 的看法是“代码要经常检查（包括自查和其他同事检查）。不要把别人的检查，看成是对代码风格的苛求。应该把它们看作是有建设性的批评。对个人来说，经常检查你的代码并且自问，“我怎样才能写得更好呢？” 这会加速你的成长，让你成为一个更优秀的程序员。”

　　**你能从代码审查中收获什么？**

　　事实显而易见，有另外一个人检查即将提交的代码，能够帮助找到bug。这是代码审查众所周知且经常被提及的好处。但依据我的经验，这是最没有价值的一个好处。人们确实可以在代码审查中找到bug。然而坦率地说，在代码审查中找到的bug绝大多数都是一些代码作者花上几分钟就能找到的小bug。那些真正需要花时间才能找到的bug在代码审查中是检查不到的。

　　代码审查最大的好处在于它是一种社交的途径。如果你编程的时候就知道会有同事检查你的代码，那么你的程序会有所不同。你写的代码会更加整洁，有着较好的注释，结构也组织的不错——因为你知道会有人来检查你的代码，而且你很在意他们的意见。如果没有代码审查，你知道代码会在最后才会审查。因为不是马上就要检查，所以对你而言并不紧迫，因而你不会想着先自检一遍。

　　代码审查还有一个更大的好处，就是可以分享知识。在很多的开发团队中，每个人都会负责并且专注于一个核心模块。除非别的同事负责的模块出现问题导致自己的代码不能运行，否则他们是不会去关注别人的工作。这样产生的结果是，每一个模块的代码只有一个人比较熟悉。假如事不凑巧，那位程序员正好休假或者离开了公司，那么没有人了解那些代码了。如果有代码审查的环节，那么至少会有两个人熟悉代码——代码的作者和审阅者。审阅者虽然没有作者对代码那么了解 ——但是他同样熟悉代码的设计和结构，这些信息是无价之宝。

　　当然，没有什么事情是那么简单的。以我的的经验看来，要做好代码审查需要一段时间练习。我注意到经验不足的审阅者通常会落入一些代码审查的陷阱，这些陷阱往往会造成很多的麻烦，给那些希望尝试代码审查的人们留下了坏印象，成为了他们采纳代码审查的一个主要障碍。

　　代码审查最重要规则是对即将提交的代码中查找问题——你需要做的就是确认代码是正确的。而通常会犯的一个错误，也是刚刚接触代码审查的新手容易犯的一个错误，即审阅者会判断这段代码是否按照自己思路来实现。

　　当有一个问题需要解决时，通常会有几十种的办法。当选定一个解决方法时，会有百万种代码实现。因此，作为一个审阅者，你的工作不是确保代码是按照你的方式来编写的——因为这是不可能的事情。审阅者的工作是确保原作者编写的代码是正确的。如果你没有遵守这个规则，你可能会到处碰壁，审查结束时你的心情很糟糕，对你来说肯定不是一件好事情。

　　**问题在于这是不自觉就会犯的一个错误。**假定你是一个程序员，当你在看一个问题的时候，你会得到一个自己的解决方案——并且你认为你看到的就是这个问题（应该采用的）解决办法。如果想要成为一名好的审查者，你需要知道这是不对的。

　　**第二个误区就是人们感觉一定要说点什么（才算是做了代码审查）。**代码的作者花了很多的时间和精力来编写代码——你难道不应该说点什么吗？

　　答案是：你不应该。

　　如果只是说“哦，这看起来这不错！”，这永远没错。反之，如果你不断地去查找一些“问题”并加以指责，那么我肯定你的信誉会荡然无存。如果你不断地去制造一些事情来说些什么，那么代码的作者会认为，当你的言论只是为了避免冷场。从此，你的意见不会受到重视。

　　**第三个误区就是速度。**你不应该匆忙完成一次代码审查——但是也不要拖延。你的同事在那里等着你的审查结果。如果你和同事不愿意抽出时间来做代码审查或者一直拖延，大家会对这次的审查感到厌烦，也会认为以后的代码审查也只会带来麻烦。看起来好像代码审查会打断你的工作，其实不必如此。你不必要在别人要求你审查的时候马上丢掉手头上的事情。但是在几个小时之内，当你工作中间休息的时候——喝杯茶，去一下洗手间或者聊聊天，散散步。当你再回来工作的时候，你可以开始并完成这个代码审查。如果你这么做了，没有人会站在你身边一直等着你给出审查结果。

21世纪的代码审查

在软件工程领域里代码审查可以结束程序员之间无谓的争执。开发者常常会因为一些愚蠢的小事斗嘴，冒犯对方，甚至是在Q&A问答之前抓住 Bug而喋喋不休，争执总是围绕在你左右。OK，千万不要误会我的意思，因为我们有理由相信代码审查绝对是个不错的好方法。原因如下：
1. 越早发现bug也就意味着可降低项目成本。无须释放一个修复补丁，因为它正处在开发阶段。
2. 代码变得越来越重要。
3. 知识贯穿于你的团队中，不再像以前那样一大块代码只有某一个人知道。
4. 开发者需要加倍的努力。如果开发者知道别人要对他的工作进行评估时，就会采取额外的努力做好工作，同时他还喜欢用文档注释标出异议。

如今，在21世纪的今天很多项目都没有使用代码审查。**本文将提供8条准则**，供开发者学习与参考。
1. 永远别忘了TDD
再确认测试代码前，先找别人帮你检查下是否无误。在别人做之前尽量检查出bug并且将其处理好。代码审查最重要规则是对即将提交的代码中查找问题——你需要做的就是确认代码是正确的。
2.尽可能的自动化
这里有几个非常好的Java工具比如：PMD, Checkstyle, Findbugs等等。问题是当利用这些工具查找后人们还肯花时间去做代码审查吗？
使用这些工具前，为这些工具制定一套细则是非常重要的。这能够确保你使用同一个代码审核标准从而区别于那些常被用于20世纪老式的代码审查规范。在理想的状态下，这些工具可运行在各种版本控制系统上通过hook审查每个代码。如果该代码不好将被阻止在外。
3.尊重设计
在我开始从事Java项目早期时，用代码审查的方式已为时已晚。因为当你检查代码问题时实际上给你的设计造成了缺陷。设计模式被误解，一些繁杂的附属物质混入进来或者开发者脱离了主题。
审查会混乱你的观点。或许你会反驳：“这是代码审查而不是设计审查”。这时一些烂摊子必然会接踵而至。为了避免这些问题发生，我们改变了设计的初衷。代码审查会牵连到很多面，无论是设计还是设计审查。事实上，我们通过设计审查要比代码审而得多的冲击要多的多。设计需要更高的质量和灵感，我们应该避免一些复杂的思维。
4. 统一的风格指南
即使是使用自动化工具（诸如Checkstyle，Findbugs 等）也应避免不必要的风格冲突，你的项目应该具备有风格指南。（在尽可能的情况下）坚持Java协议的规范标准。尝试着为你的项目介绍制定一个“词典”，这就意味着，当涉及这个代码时，查看该代码的用法和环境是否适宜，这些都很容易被检测出。
5. 挑选适宜的工具
如果开发者都在使用Eclipse开发工具（ Eclipse IDE插件Jupiter），你可以通过你的方式来查看代码、调试代码甚至可使用Eclipse IDE上的一切东西当来帮助你在审查代码时更加的便捷。但是，如果大家没有使用同一个IDE（或者该IDE没有给你的工作带来方便）你可以考虑 Review Board. ，它是个不错的选择。
6.请记住每个项目都不同
也许你在采用以前的项目方法工作，但是，请记住每个项目之间是不同的。每一个项目都有特定的架构（高并发或是高分散），有特定的文化（或许很多人喜欢使用Eclipse），并使用特定的工具（maven or ant）。难道你想照葫芦画瓢？OK，请记住，不同的项目有不同的工作方法。
7.懂得取舍
代码审查需要积极和细致而不是卖弄学问。你会因为一些细微的琐事让你紧张而导致项目失败或是花费公司成本吗？记住，千万不要这样。理清头绪，换个角度想想，改变自己的心态而不是记挂着去改变别人。
8. Be buddies
在我看来，称之为“buddy reviews”（别人会叫“over the shoulder”）非常好。A buddy review是指与其他团队成员每隔一到两天以非正式的形式讨论，并且快速的浏览（5-10分钟）对方的代码。这种方法可以帮助你：
1. 及早的发现问题
2. 总是很快的知道该干什么
3. 代码审查无须过长，因为你只需要查看新的代码，旧的代码会很快赶上
4. 这种非正式的场合——没有紧张感，很有趣！
5. 可以定期的交换想法
buddy reviewing在团队中是一种很好的工作方式，当某人在团队中出现问题时可以及早的发现。这不仅可以帮助大家，还可以交换彼此的进度和想法。
总之，如果你的项目正在进行代码审查，应该做到快速、有效、不浪费别人的时间。正如文章所说的，这几点非常重要。代码审查用意是在代码提交前找到其中的问题。

代码审查

代码审查可以帮助提高代码质量，避免由于代码习惯而造成的 bug。下面列出的这些要点因该可以作为大部分代码审查的指导，如果是 Java 应用的话，这些建议应该被视作最佳实践。

文档

  1. Javadoc 应该在每一个类和方法中添加。

  2. 如果是修复某个 bug，应该添加 bug ID。

  3. 走捷径的方法或者复杂的逻辑要有解释。

  4. 如果代码会被公开，每个文件头都要标注版权信息。

  5. 复杂的 HTML，JavaScript，CSS 应该包含文档。

功能

  1. 如果类似的逻辑被使用了多次，应该把它写成一个帮助类，然后在多出调用。

  2. 鼓励使用 API 而不是重复编写代码解决相同的问题。

  3. 要强调代码的单元测试。

  4. 任何新加的代码不应该破坏已有的代码。

  5. 假如是 Web 应用，JSP 不应该包含 Java 代码。

安全

  1. 任何代码都不能执行用户的输入，除非转义过了。这个常常包含 JavaScript 的 eval 函数和 SQL 语句。

  2. 禁止那些在短时间内提交非常多请求的 IP。

  3. 任何类，变量，还有方法都应该有正确的访问域。

  4. 尽量避免使用 iframe。

性能

  1. 所有数据库和文件操句柄在不需要的时候都应该被关闭。

  2. SQL 语句的写法会导致性能千差万别。

  3. 鼓励创建不可变（immutable）的类。

  4. 类似的逻辑代码，尽量通过 if else 语句来实现更多的重用。

  5. 尽量避免使用重对象（heavy objects）。

  6. 如果是 Web 项目，请检查是否使用了合适的图片尺寸，CSS sprites 和浏览器缓存等技术。

  7. 全局都需要的信息保存在 application context 中。

编码习惯

  1. 没有被使用的变量要删除。

  2. 针对不同的 Exception 要用不同的 catch 语句，而不是一个 Exception 解决所有问题。

  3. 针对变量，方法和类要用相同的命名方法。

  4. 常量应该被写在独立的常量类中。

  5. 每行代码的尾部不要有多余的空格。

  6. 对于括号，循环，if语句等等要用统一的格式。

  7. 每一个单独的方法不应该超过100行。

  8. 一个单独的语句不应该超过编辑器的可视区域，它可以被拆分成几行。

  9. 检查 String 对象既不是null也不是空的最好方法是 if(“”.equals(str))

  10. 假如类有很多成员变量，并且实例化的时候只需要少数变量传入的话，最好使用静态工厂方法，而不是重载构造函数。

  11. 给方法添加适当的访问控制，而不是所有都是 public。

  12. 遵守项目中使用的框架的最佳实践建议，例如 Spring，Struts，Hibernate，jQuery。

以上的某些注意点可以通过静态代码检查工具完成，例如 CheckStyle，FindBugs 和 JTest。