Code Review

Why Code Reivew：

浅层次的Code Review是基于代码规范的，代码规范是通过可读性、易修改性来解决团队协作、提升项目可维护性。深层次的Code Review会检查技术和业务逻辑实现的正确、优雅性，类似于黑盒测试。

1. Code reviews  通过大家的建议增进代码的质量。
2. Code reviews  是一个传递知识的手段，可以让其它并不熟悉代码的人知道作者的意图和想法，从而可以在以后轻松维护代码。
3. Code reviews 鼓励程序员们相互学习对方的长处和优点。
4. Code reviews 确认自己的设计和实现是否清楚和简单。

我们没有提到可以帮助找到程序的bug和保证代码风格和编码标准。这是因为我们认为：

1. Code reviews 不应该承担发现代码错误的职责。Code Review主要是审核代码的质量，如可读性，可维护性，以及程序的逻辑和对需求和设计的实现。代码中的bug和错误应该由单元测试，功能测试，性能测试，回归测试来保证的（其中主要是单元测试，因为那是最接近Bug，也是Bug没有扩散的地方）
2. Code reviews 不应该成为保证代码风格和编码标准的手段。编码风格和代码规范都属于死的东西，每个程序员在把自己的代码提交团队Review的时候，代码就应该是符合规范的，这是默认值，属于每个人自己的事情，不应该交由团队来完成，否则只会浪费大家本来就不够的时间。我个人认为“meeting”是奢侈的，因为那需要大家在同一时刻都挤出时间，所以应该用在最需要的地方。代码规范比起程序的逻辑和对需求设计的实现来说，太不值得让大家都来了。

今天，在中国的很多公司上面这两件事依然被认为是Code Reivew最重要的事，所以，我能够看到很多开发Team抱怨Code Review就是一个形式，费时费力不说，发现的问题还不如测试，而评审者们除了在代码风格上有些见术，别的也就没什么用了，长而久之，大家都会开始厌烦这个事了。

所以，在今天，请不要把上面的那两件事分散了Code Review的注意力，取而代之的是，对于Bug，程序的作者要在Review前提交自己的单元测试报告（如：XUnit的测试结果），对于代码规范，这是程序作者自己需要保证的，而且，有一些工具是可以帮你来检查代码规范的。

当然，上述这些言论并不是说，你不能在Code Review中报告一个程序的bug或是一个代码规范的问题。我只是说，那并不是Code Review的意图。

下面是我们认为的几个小提示可以让你更好进行Code Review这项最有价值的活动。

1.- 经常进行Code Review

以前经历过几个相当痛苦的Code Review，那几次Code Review都是在程序完成的时候进行的，当你面对那近万行的代码，以前N多掺和在一起的功能，你会发现，整个Code Review变得非常地艰难，用不了一会儿，你就会发现大家都在拼命地打着哈欠，但还是要坚持，有时候，这样的Review会持续3个小时以上，相当的夸张。而且，会议上会出现相当多的问题和争论，因为，这就好像，人家都把整个房子盖好了，大家Review时这挑一点那挑一点，有时候触动地基或是承重墙体，需要大动手术，让人返工，这当然会让盖房的人一下就跳起来极力地维护自己的代码，最后还伤了团队成员的感情。

所以，千万不要等大厦都盖好了再去Reivew，而且当有了地基，有了框架，有了房顶，有了门窗，有了装修，的各个时候循序渐进地进行Review，这样反而会更有效率，也更有帮助。

下面是一些观点，千万要铭记：

• 要Review的代码越多，那么要重构，重写的代码就会越多。而越不被程序作者接受的建议也会越多，唾沫口水战也会越多。
  
• 程序员代码写得时候越长，程序员就会在代码中加入越来越多的个人的东西。 程序员最大的问题就是“自负”，无论什么时候，什么情况下，有太多的机会会让这种“自负”澎涨开来，并开始影响团队影响整个项目，以至于听不见别人的建议，从而让Code Review变成了口水战。
• 越接近软件发布的最终期限，代码也就不能改得太多。

我个人的习惯，并且也是对团队成员的要求是——先Review设计实现思路，然后Review设计模式，接着Review成形的骨干代码，最后Review完成的代码，如果程序复杂的话，需要拆成几个单元或模块分别Review。当然，最佳的practice是，每次Review的代码应该在1000行以内，时间不能超过一部电影的时间——1.5小时（因为据说那个一个正常人的膀胱可以容纳尿液的最长限度）

当然，在敏捷开发中，他们不需要Code Reivew，其实，敏捷开发中使用更为极端的“结对编程”（Pair-Programming）的方法 —— 一种时时刻刻都在进行Code Review的方法，个人感觉在实际过程中，这种方法有点过了。另外，大家可以看看本站的另一篇文章《结对编程的利与弊》来了解一下这种方法的问题。

2.- Code Review不要太正式，而且要短

忘了那个代码评审的Checklist吧，走到你的同事座位跟前，像请师父一样请他坐到你的电脑面前，然后，花5分钟给他讲讲你的代码，给他另外一个5分钟让他给你的代码提提意见，这比什么都好。而如果你用了一个Checklist，让这个事情表现得很正式的话，下面两件事中必有一件事会发生：

1. 只有在Checklist上存在的东西才会被Review。
2. Code Reviews 变成了一种礼节性的东西，你的同事会装做很关心你的代码，但其实他心里想着尽快地离开你。

只有不正式的Code Review才会让你和评审者放轻松，人只有放松了，才会表现得很真实，很真诚。记住Review只不过是一种形式，而只有在相互信任中通过相互的讨论得到了有意义和有建设性的建议和意见，那才是最实在的。不然，作者和评审者的关系就会变成小偷和警察的关系。

3.- 尽可能的让不同的人Reivew你的代码

这是一个好主意，如果可能的话，不要总是只找一个人来Review你的代码，不同的人有不同的思考方式，有不同的见解，所以，不同的人可以全面的从各个方面评论你的代码，有的从实现的角度，有的从需求的角度，有的从用户使用的角度，有的从算法的角度，有的从性能效率的角度，有的从易读的角度，有的从扩展性的角度……，啊，好多啊，还让不让人活了。不管怎么说，多找一些不同的人会对你很有好处。当然，不要太多了，人多嘴杂反而适得其反，基本上来说，不要超过3个人，这是因为，这是一个可以围在一起讨论的最大人员尺寸。

下面是几个优点：

1. 从不同的方向评审代码总是好的。
2. 会有更多的人帮你在日后维护你的代码。
3. 这也是一个增加团队凝聚力的方法。

4.- 保持积极的正面的态度

再说一次，程序最大的问题就是“自负”，尤其当我们Reivew别人的代码的时候，我已经见过无数的场面，程序员在Code Review的时候，开始抨击别人的代码，质疑别人的能力。太可笑了，我分析了一下，这类的程序员其实并没有什么本事，因为他们指责对方的目的是想告诉大家自己有多么的牛，靠这种手段来表现自己的程序员，其实是就是传说中所说的“半瓶水”。

所以，无论是代码作者，还是评审者，都需要一种积极向上的正面的态度，作者需要能够虚心接受别人的建议，因为别人的建议是为了让你做得更好；评审者也需要以一种积极的正面的态度向作者提意见，因为那是和你在一个战壕里的战友。记住，你不是一段代码，你是一个人！

5.- 学会享受Code Reivew

这可能是最重要的一个提示了，如果你到了一个人人都喜欢Code Reivew的团阿，那么，你会进入到一个生机勃勃的地方，在那里，每个人都能写出质量非常好的代码，在那里，你不需要经理的管理，团队会自适应一切变化，他们相互学习，相互帮助，不仅仅是写出好的代码，而且团队和其中的每个人都会自动进化，最关键的是，这个是一个团队。

Code Review需要做什么

以下内容参考了《Software Quality Assurance: Documentation and Reviews》一文中的代码检查部分。

 

1). 完整性检查（Completeness）

·        代码是否完全实现了设计文档中提出的功能需求

·        代码是否已按照设计文档进行了集成和Debug

·        代码是否已创建了需要的数据库,包括正确的初始化数据

·        代码中是否存在任何没有定义或没有引用到的变量、常数或数据类型

 

2). 一致性检查（Consistency）

·        代码的逻辑是否符合设计文档

·        代码中使用的格式、符号、结构等风格是否保持一致

 

3). 正确性检查（Correctness）

·        代码是否符合制定的标准

·        所有的变量都被正确定义和使用

·        所有的注释都是准确的

·        所有的程序调用都使用了正确的参数个数

 

4). 可修改性检查（Modifiability）

·        代码涉及到的常量是否易于修改(如使用配置、定义为类常量、使用专门的常量类等)

·        代码中是否包含了交叉说明或数据字典，以描述程序是如何对变量和常量进行访问的

·        代码是否只有一个出口和一个入口（严重的异常处理除外）

 

5). 可预测性检查（Predictability）

·        代码所用的开发语言是否具有定义良好的语法和语义

·        是否代码避免了依赖于开发语言缺省提供的功能

·        代码是否无意中陷入了死循环

·        代码是否是否避免了无穷递归

 

6). 健壮性检查（Robustness）

代码是否采取措施避免运行时错误（如数组边界溢出、被零除、值越界、堆栈溢出等）

 

7). 结构性检查（Structuredness）

·        程序的每个功能是否都作为一个可辩识的代码块存在

·        循环是否只有一个入口

 

8). 可追溯性检查（Traceability）

·        代码是否对每个程序进行了唯一标识

·        是否有一个交叉引用的框架可以用来在代码和开发文档之间相互对应

·        代码是否包括一个修订历史记录，记录中对代码的修改和原因都有记录

·        是否所有的安全功能都有标识

 

9). 可理解性检查（Understandability）

·        注释是否足够清晰的描述每个子程序

·        是否使用到不明确或不必要的复杂代码，它们是否被清楚的注释

·        使用一些统一的格式化技巧（如缩进、空白等）用来增强代码的清晰度

·        是否在定义命名规则时采用了便于记忆，反映类型等方法

·        每个变量都定义了合法的取值范围

·        代码中的算法是否符合开发文档中描述的数学模型

 

10). 可验证性检查 (Verifiability)

代码中的实现技术是否便于测试

Code Review经验检查项

以下是在实践中建立的检查列表（checklist），通过分类和有针对性的检查项，保证了Code Review可以有的放矢。

 

1. JAVA编码规范方面检查项

检查项参照JAVA编码规范执行，见《JAVA编码规范(Java Code Conventions)》

 

2. 面向对象设计方面检查项

这几点的范围都很大，不可能在本文展开讨论，有专门的书籍介绍这方面问题，当然在Code Review中主要靠经验来判断。

·        类设计和抽象是否合适

·        是否符合面向接口编程的思想

·        是否采用合适的设计范式

 

3. 性能方面检查项

性能检查在大多数代码中都是需要严重关注的方面，也是最容易出现问题的方面，常常有程序员写出了功能和语法没有丝毫问题的代码后，正式运行时却在性能上表现不佳，从而不得不做大量的返工，甚至是推倒重来。

·        在海量数据出现时,队列,表,文件,在传输,upload等方面是否会出现问题,有无控制,如分配的内存块大小,队列长度等控制参数

·        对hashtable,vector等集合类数据结构的选择和设置是否合适，如正确设置capacity,load factor等参数，数据结构的是否是同步的

·        有无滥用String对象的现象

·        是否采用通用的线程池、对象池模块等cache技术以提高性能

·        类的接口是否定义良好，如参数类型等，避免内部转换

·        是否采用内存或硬盘缓冲机制以提高效率

·        并发访问时的应对策略

·        I/O方面是否使用了合适的类或采用良好的方法以提高性能（如减少序列化，使用buffer类封装流等）

·        同步方法的使用是否得当,是否过度使用

·        递归方法中的叠代次数是否合适，应该保证在合理的栈空间范围内

·        如果调用了阻塞方法，是否考虑了保证性能的措施

·        避免过度优化，对性能要求高的代码是否使用profile工具，如Jprobe等

 

4. 资源泄漏处理方面检查项

对于JAVA来说由于存在垃圾收集机制，所以内存泄漏不是太明显，但使用不当，仍然存在内存泄漏的问题。而对于其它的语言，如C++等在这方面就要严重关注了。当然数据库连接资源不释放的问题也是广大程序员最常见的，相信有很多的PM被这个问题折磨的死去活来。

·        分配的内存是否释放，尤其在错误处理路径上（对非JAVA类）

·        错误发生时是否所有的对象被释放，如数据库连接、Socket、文件等

·        是否同一个对象被释放多次（对非JAVA类）

·        代码是否保存准确的对象reference计数（对非JAVA类）

 

5. 线程安全方面检查项

线程安全问题实际涉及两个方面，一个是性能，另一个是资源的一致性，我们需要在这两方面做个权衡，现在就是到了权衡利弊的时候了。

·        代码中所有的全局变量是否是线程安全的

·        需要被多个线程访问的对象是否线程安全，检查有无通过同步方法保护

·        同步对象上的锁是否按相同的顺序获得和释放以避免死锁，注意错误处理代码

·        是否存在可能的死锁或是竞争，当用到多个锁时，避免出现类似情况：线程A获得锁1，然后锁2，线程B获得锁2，然后锁1

·        在保证线程安全的同时，要注意避免过度使用同步，导致性能降低

 

6. 程序流程方面检查项

·        循环结束条件是否准确

·        是否避免了死循环的产生

·        对循环的处理是否合适，如循环变量，局部对象，循环次数等能够考虑到性能方面的影响

 

7. 数据库处理方面

很多Code Review人员在面对代码中涉及到的数据库可移植性和提高数据库性能方面的冲突时表现的无所适从，凡事很难两全其美的啊。

·        数据库设计或SQL语句是否便于移植（注意和性能方面会存在冲突）

·        数据库资源是否正常关闭和释放

·        数据库访问模块是否正确封装，便于管理和提高性能

·        是否采用合适的事务隔离级别

·        是否采用存储过程以提高性能

·        是否采用PreparedStatement以提高性能

 

8. 通讯方面检查项

·        socket通讯是否存在长期阻塞问题

·        发送接收的数据流是否采用缓冲机制

·        socket超时处理，异常处理

·        数据传输的流量控制问题

 

9. JAVA对象处理方面检查项

这个检查项的基础是对JAVA对象有较深的理解，但现实是很多看过《Thinking in Java》的程序员，仍然在程序中无法区分传值和传引用，以及对象和reference的区别。这或许就是理论和实践难以结合的问题啊。正所谓知而不行，非真知也。

·        对象生命周期的处理,是否对象的reference已经失效,能够设置为null,并被回收

·        在对象的传值和传参方面有无问题,对象的clone方法使用是否过度

·        是否大量经常的创建临时对象

·        是否尽量使用局部对象(堆栈对象)

·        在只需要对象reference的地方是否创建了新的对象实例

 

10. 异常处理方面检查项

JAVA中提供了方便的异常处理机制，但普遍存在的是异常被捕获，但并没有得到处理。我们可以打开一段代码，最常见的现象是进入某个方法后，一个大的try/catch将所有代码行括住，然后在catch中将异常打印到控制台，而且该异常是Exception对象。

·        每次当方法返回时是否正确处理了异常，如最简单的处理，记录日志到日志文件中

·        是否对数据的值和范围是否合法进行校验，包括采用断言（assertion）

·        在出错路径上是否所有的资源和内存都已经释放

·        所有抛出的异常都得到正确的处理，特别是对子方法抛出的异常，在整个调用栈中必须能够被捕捉并处理

·        当调用导致错误发生时，方法的调用者应该得到一个通知

·        不要忘了对错误处理部分的代码进行测试，很多代码在正常情况下执行良好，而一旦出错，整个系统就崩溃了

 

11. 方法（函数）方面检查项

·        方法的参数是否都做了校验

·        数组类结构是否做了边界校验

·        变量在使用前是否做了初始化

·        返回堆对象的reference，不要返回栈对象的reference

·        方法API是否被良好定义，即是否尽量面向接口编程，便于维护和重构

 

12. 安全方面检查项

·        对命令行执行的代码，需要详细检查命令行参数

·        WEB类程序检查是否对访问参数进行合法性验证

·        重要信息的保存是否选用合适的加密算法

·        通讯时考虑是否选用安全的通讯方式

 

13. 其他

·        日志是否正常输出和控制

·        配置信息如何获得,是否有硬编码

 

三、总结

通过在项目中实施Code Review将为我们带来多方面的好处，表现在提高代码质量，保证项目或产品的稳定性，开发经验的积累等，具体的实施当然也要看项目的实际情况，因为Code Review也是需要成本的，这方面属于Code Review过程的问题，将在其他文章中进行探讨。

How Code Review：

Code Review的一些阻力：

项目组成员积极性不高

Code Review效果不好

Code Review Meeting成本太高

基于这些阻力Code Review很容易流于形式。

Code Review Method：

1、每个开发人员有自己的CC基线，级别低的开发人员，先checkin到自己的基线，然后让高级别的人员review，review通过后才能合入正式基线。