CodeReview实践与总结

CodeReview 是大型软件工程中公认的必不可少的保证工程质量的重要手段之一。但凡正规软件作战军团都是非常重视 CodeReview 的作用和意义的。那么，如何做好 CodeReview 呢？这里总结下自己的学习笔记和实践心得。

CodeReview有效性###

对于 CodeReview 该做什么和不该做什么，尚存在争议。 个人认为，与其从理论层次去思辨，不妨从现实角度思考：怎样的CodeReview是有效的？

如果一次CodeReview能够检测出代码中的错误或设计的缺陷(即使是低级错误)，阻止上线后导致BUG甚至故障，那么就可以说此次CodeReview 是有效的、成功的。

现实才不管你是怎么区分的，出了导致负面影响的BUG或故障就是铁的事实，没有高级低级之分。

CodeReview的使命就是阻止有负面影响力的 BUG 或故障上线。

CodeReview的两个基本问题###

两个基本问题：

1. 如何 Review 出代码错误和设计缺陷?
2. 如何更高效地Review出代码错误和设计缺陷？

问题二是问题一的递进，先解决问题一。

CodeReview显然不可能面面俱到，在一次提交中程序猿媛们潜在犯错误可能就那么一两个地方，而且很可能隐蔽。这就要更有针对性： 程序猿媛们更容易犯怎样的错误呢？ 一方面，可以通过常见问题来了解， 另一方面， 通过建立 BUG 追踪库，可以得到更为精准甚至出乎意料的结论。

CodeReview代码问题###

CR常见代码问题

高效CodeReview###

高效CodeReview 一方面需要深入熟悉各种代码问题，另一方面需要一定的技巧和工具来快速识别问题。

改动点和改动描述####

代码提交者最好说明改动点和准确的改动描述，让CodeReview的同学有心理预期，明白该 Review 哪些内容和关键点；

问题分类与优先级####

从上一节可知，代码问题的出现种类实在太多，而一次提交中所犯的错误很少很隐蔽。要想更高效的实现有效的CodeReview, 那么必须采用一些技巧。

• 对代码问题进行分类，确定优先级：常见代码问题 > 可维护性问题 > 更难发现的问题 > 较轻微问题。 首先保证代码是正确的，然后确保代码的可维护性。
• 较轻微问题最好由工具使用和代码提交者的自律来保证；
  
  

熟悉相关业务####

CodeReview第一要保证业务的正确性，需要Review者去熟悉相关业务，而不仅仅是从代码层面来推敲。如果改动比较大，还需要推敲设计是否合理，是否划分清晰了系统边界，有哪些遗漏的地方。

影响面评估####

影响面评估非常重要！！！

有时，只改动一行代码，却会影响百万次调用；有时，改动几十几百行，也只影响一个局部功能。 确定代码变更的影响面是CR的首要职责。 如果影响面非常大，需要考虑对所有相关业务的影响，多推敲，保证严格的代码健壮性、性能与超时合理、避免资损等，进行回归测试，确保影响可控。

有针对性####

CodeReview 的同学需要对代码有一定敏感度，熟悉各种代码改动最可能导致的问题，然后确定是否有问题。比如调用 API 接口，则要考虑依赖服务的可靠性，做好捕获异常的操作和日志记录，避免因局部影响整体功能；拉取大量数据，则要考虑采用批量或并发的方式，或者构造合适的数据结构和算法，来避免性能问题；有跨语言交互，要考虑接口之间的边界处理是否正常，在出错的时候是否也衔接良好； 复杂的数据结构，要考虑解析是否正常以及异常捕获； 访问数据库， 要检测是否获取到相应的对象。

做好普通的CodeReview####

幸运的是，绝大多数CodeReview 并不涉及复杂的并发场景和数据一致性问题，只是普通的业务操作。针对这种，可以划分为两种类型的Review: (1) 子模块的CodeReview ; (2) 接口边界的 CodeReview 。

子模块的CodeReview尤其要注意健壮性。能够推断出各种可能的主要错误场景，并在错误或异常的情况下正确捕获异常、打印日志并返回可读的错误信息，通常就做好了一大半工作。

接口边界的CodeReview通常要注意返回结构是否衔接良好。 正常和异常情况的数据结构返回都要测试一下。

CodeReview工具####

工具始终是提高效率的利器。

提高CodeReview效率的工具分两类： (1) 静态代码检测工具，比如 PMD, FindBugs, etc. (2) CodeReview 工具：比如 Upsource 。

CodeReview的一个实际困难是，在缺失整体代码氛围的情况下审视代码片段很难保证CodeReview的有效性。也就是说，代码片段可以“看上去是毫无问题的”，然而与现有的某个设计或细节关联起来就变成了BUG。此外，在浏览器中做CodeReview要求暂时离开IDE，会破坏开发者的工作状态。Upsource 可以与 IntellJ 很好的集成，下载 IntellJ 的 upsource 插件，就可以在 IntellJ 里面 Review 相关改动，添加 Review 意见， 回复 Review 意见，这些都会自动同步给 Upsource ，然后发送通知给代码提交者。

CodeReview的另一个实际困难是， 很难在短时间内对代码可能出现的各种问题都判断一遍，只能专注于最可能出现的问题，而部分可维护性问题、轻微问题、编程风格、单元测试、正常功能测试则必须由代码提交者保证。 静态代码检测工具一般在代码提交前使用，保证在CodeReview前就消除大部分低级错误； 这可以形成团队代码规范，提高CodeReview效率。

CodeReview小贴士###

一些小的技巧和习惯可以让 CodeReview 更加容易和轻松。

• Review时机： 对于普通bugfix或优化，CodeReview最迟要在发布前一天或发布当天早上； 对于项目，CodeReview 最迟应该在QA提测前一天；
• 持续增强原则：每次提交变更尽可能小一点， 但是建议每次的变更都是正确和完整的， 合并到代码库中持续增强系统，尽量保证随时可交付。
• 去除无用改动： 仔细斟酌每一行改动，去掉无用的注释，reset 掉没有逻辑改动（空行改动）的文件；改动越少越好； 合并提交： 如果仅有一个分支，且全部是自己的改动，合并成一次提交，避免漏掉某些重要Review；
• 分支与提交： 使用 teamname_改动简述_年月日 作为分支名，认真编写含义明显的 commit 注释。
• 相关性：组内业务的相关改动放在单独分支里方便Review ; 避免在一个大而全的项目分支里Review 少许改动造成遗漏；
• 改动最小化： 改动文件数代码数尽量越小越好，减少调用方兼容代码更改； 不改“陌生”代码： 不要自己更改前端代码(涉及不熟悉的打包流程)；不要更改自己不熟悉的代码；
  
  

代码提交建议标准###

不涉及复杂并发或一致性的业务变更，要求能够达到第十二级； 复杂的业务，需要达到第十六级。

1. 第一级： 无语法错误，消除无用的代码、注释；解决了代码检测工具的警告与错误。
2. 第二级： 简洁清爽的排版，合理的命名， 遵循代码规范， 适宜必要的注释。
3. 第三级： 适当的方法 Javadoc 及注释， 注明用途及作者、日期; 文档化边界点、例外情况、极端情况以及特殊处理。
4. 第四级： 编译通过，成功启动应用。
5. 第五级： 能处理正常情况下的功能实现，保证正常场景下的准确。
6. 第六级： 能处理不合法输入，给出友好错误提示；能处理可能出现的错误和异常，输出合理级别和合适detail容易排错的日志。
7. 第七级： 使用相互协作良好的短小方法； 消除了重复代码； 没有硬编码； 没有未实现的 TODO 桩。
8. 第八级： 编写的代码有比较完善的单元测试用例；对错误和异常进行了测试；变更有相应单元测试且通过； 全量单元测试本地通过。
9. 第九级： 避免常见的错误，比如 +-1、越界、空指针、 传值或引用错误、 变量未初始化为合适值、多重条件下的逻辑与或非错误、死循环、修改全局变量。
10. 第十级： 考虑基本的安全问题，比如SQL注入攻击、访问或操作权限限制、屏蔽敏感信息。
11. 第十一级： 如果涉及到性能，比如大量数据处理，选择合适的容器及算法，使性能保持在 O(nlogn) 或 O(n)。
12. 第十二级： 使用合理的设计模式组织代码结构，消除重复，实现可扩展性。
13. 第十三级：确保打开的资源（http, db, file）被正确释放 。
14. 第十四级： 对于关键的数据一致性业务，使用合适的数据库事务和锁同步。
15. 第十五级： 考虑性能、安全、线程安全及死锁；对共享可变的状态使用同步访问，使用并发类、线程池而不是原始的线程对象。
16. 第十六级： 开发时考虑后期维护和可运维性，使错误情况的排查更加高效，从错误中恢复更加容易。
    
    

阶段性小结###

CodeReview的检查事项可以涵盖非常广泛的范围，从不起眼的“变量未初始化”到“并发事务”，乃至设计性问题。这要求代码提交者有足够高的自律和编程能力，能够尽量自行Hold住这些Review事项。而当一个人能够做到这些时，他就具备了Review其他伙伴代码的能力。

参考内容###

CodeReview重点检查什么####

• 业务逻辑的实现是否未考虑到全局的设计或现有的某些业务细节（对业务不够熟悉的同学往往因为没有考虑到更大的业务范围或细节而犯错）
• 是否有隐藏的细微错误或潜在的隐患（经验判断）
• 代码的质量属性，性能、可维护性、可扩展性等，对需求和设计的代码实现方式
• CodeReview 不应该检测编程风格和低级错误；代码提交者应当足够自律保证这两点
• CodeReview 不应该承担发现编译与运行错误的职责；代码中的编译与运行错误应该由单元测试，接口测试，回归测试来消除
  
  

CodeReview检查类型及优先级####

• Functionality 功能问题 = High
• General Unit Testing 单元测试 = High
• Error Handling 错误处理 = High
• Resource Leaks 资源泄露 = High
• Performance 性能问题 = High
• Thread Safety 并发安全 = High
• Security 安全问题 = High
• Redundant Code 重复或无用代码 = Medium or High
• Control Structures and Logical issues 控制结构和逻辑错误 Medium or High
• Comment and Coding Conventions 注释与代码风格 = Low

一些老外建议做以上事项的CodeReview。事实上，对于 90% 的CodeReview 来说，通常涉及到的是 Functionality 、General Unit Testing 和 Error Handling。 其他大部分很少会触及到。因此说， 与其沉迷于思辨， 不如从实际角度来探讨和行动。

检查点详细清单####

【以下内容来自于《Java Code Review清单》】

信息安全#####

• 注释出安全相关的信息 文档化
• 系统的输入必须检查是否有效和在允许范围内 拒绝服务(Denial of Service)
• 检验输入是否含有非法或恶意字符， 防止注入性攻击 拒绝服务(Denial of Service)
• 避免对于一些不寻常行为的过分日志 拒绝服务(Denial of Service)
• 把从不可信对象得到的输出作为输入来检验 输入检验(Input Validation)
• 避免服务器暴露应用系统的目录结构的配置 访问限制
• 定义合理的角色权限分级， 并授予合适的人员 访问限制
• 从异常中清除敏感信息(暴露文件路径，系统内部相关，配置) 私密信息(Confidential Information)
• 不要把高度敏感的信息写到日志 私密信息(Confidential Information)
• 考虑把高度敏感的信息在使用后从内存中清除 私密信息(Confidential Information)
• 重要信息如密码的保存是否选用难以破解的不可逆加密算法 私密信息(Confidential Information)
• 通讯时考虑是否使用了安全的通讯方式 私密信息(Confidential Information)
• 避免暴露敏感类的构造函数 对象构造
• 避免安全敏感类的序列化 序列化反序列化(Serialization Deserialization)
• 通过序列化来保护敏感数据 序列化反序列化(Serialization Deserialization)
• 小心地缓存潜在的特权操作结果 序列化反序列化(Serialization Deserialization)
• 只有在需要的时候才使用JNI 访问限制
  
  

性能#####

• 同步方法是否过度使用， 同步区域是否过大 并发
• 处理大量数据时，是否选取了合适的数据结构和高效的算法 综合编程
• 对hashtable,vector等集合类数据结构的选择和设置是否合适，如正确设置capacity,load factor等参数，数据结构的是否是同步的 综合编程
• 是否采用通用的线程池、对象池、连接池等cache技术以提高性能 综合编程
• 是否采用内存或硬盘缓冲机制以提高效率 综合编程
• I/O方面是否使用了合适的类或采用良好的方法以提高性能（如减少序列化，使用buffer类封装流等） 综合编程
• 递归方法中的叠代次数是否合适，应该保证在合理的栈空间范围内 综合编程
• 如果调用了阻塞方法，是否考虑了保证性能的措施 综合编程
• 避免过度优化，对性能要求高的代码是否使用profile工具，如Jprobe等 工具使用
  
  

资源泄露检查#####

• 资源回收与泄露 是否集合中的失效对象的reference 已经设置为 null 可以被回收 综合编程
• 是否所有的资源对象被正确释放，如数据库连接、Socket、文件等 综合编程
• 资源是否被释放多次 综合编程
• 避免使用finalizer 综合编程
  
  

数据库访问#####

• 数据库设计或SQL语句是否便于移植（注意和性能方面会存在冲突） 综合编程
• 数据库资源是否正常关闭和释放 综合编程
• 数据库访问模块是否正确封装，便于管理和提高性能 综合编程
• 是否采用合适的事务隔离级别 综合编程
• 是否采用存储过程以提高性能 综合编程
• 是否采用PreparedStatement以提高性能 综合编程
  
  

网络通信#####

• socket通讯是否存在长期阻塞问题 综合编程
• 发送接收的数据流是否采用缓冲机制 综合编程
• socket超时处理，异常处理 综合编程
• 数据传输的流量控制问题 综合编程
  
  

错误处理#####

• 每次当方法返回时是否正确处理了异常，记录日志到日志文件中 日志
• 是否对数据的值和范围的合法进行校验 输入检验(Input Validation)
• 在出错路径上是否所有的资源和内存都已经释放 综合编程
• 所有抛出的异常都得到正确的处理，特别是对子方法抛出的异常，在整个调用栈中必须能够被捕捉并处理 异常
• 当调用导致错误发生时，方法的调用者应该得到一个通知 异常
• 对可以恢复的情况使用已受检异常(checked exceptions)，对于程序错误使用运行时异常(runtime exceptions) 异常
• 是否更多地使用标准异常 异常
• 是否定义了具有合理名称的自定义异常 异常
• 是否“默默地吞掉了”异常 异常
  
  

面向对象编程#####

• 通过接口而不是实现类来引用对象，是否符合面向接口编程的思想 设计与重构
• 方法API是否被良好定义， 便于维护和重构 设计与重构
• 重写对象的equals时， 总是重写hashCode 基础
• 总是重写对象的 toString 基础
• 需要 update 的对象的值是否正确地设置 基础
• 是否大量或频繁地创建临时对象 基础
• 是否尽量使用局部对象(堆栈对象) 基础
• 是否使用了全局可变对象且在某处代码里进行了修改 基础
• 是否修改了全局可见 final Reference 的内容 基础
• 在只需要对象reference的地方是否创建了不必要的对象实例 基础
• 类的接口是否定义良好，如参数类型等，避免内部转换 基础
• 是否有丑陋的强制类型转换 基础
• 是否存在不必要的使用反射来获取私密信息 基础
• 返回堆对象的reference，不要返回栈对象的reference 基础