CodeReview实践与总结

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

CodeReview有效性

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

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

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

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

CodeReview的两个基本问题

两个基本问题：

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

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

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

CodeReview代码问题

常见代码问题

常见的潜在代码问题是当前直接会导致BUG、故障或者产品功能不能正常工作的类别。

空值

空值恐怕是最容易出现的地方之一。常见错误有： a. 值为NULL导致空指针异常； b. 参数字符串含有前导或后缀空格没有Trim导致查询为空。导致以上结果的原因主要有：无此记录、有此记录但由于SQL访问异常而没查到、网络调用失败、记录中有脏数据、参数没传。

原则上，对于任何异常，希望能够打印出具体的错误信息，根据错误信息很快明白是什么原因，而不是一个 null ，还要在代码里去推敲为什么为空。这样我们必须识别出程序中可能的null，并及时检测、捕获和抛出异常。

对于空值，最好的防护是“防御式编程”。当获取到对象之后，使用之前总是判断是否为空，并适当抛出异常、打错误日志或做其它处理。有的人嫌检测为空的 if 语句充斥在代码里会破坏代码的可维护性，对此我的建议是：

空值检测一定要有，有胜于无。
在空值检测总是存在的前提下，可以优化空值检测的方法和存在形式。比如集中于一个类 NullChecker 中管理，并与系统的整体错误处理设计保持一致。集中管理和处理一致性原则可以作为系统设计的一个准则。这样主流程中只要增加一行调用即可，既可以天网恢恢疏而不漏地检测对象为空，也不会让代码显得难看。
```
class NullChecker {
public static void checkNull(Object obj, Error error) {
if (obj == null)  { throw new BizException(error); }
   }
}
```
在参数入口处统一做 trim。如果在业务逻辑里做 trim ，就会导致有的业务逻辑做了 trim ，有的没做，体现在产品上就会有令用户困惑的事情发生。比如搜索和导出业务，搜索能搜索出来，导出却没有。

未捕获潜在的异常

第二个容易出错的地方是未捕获潜在的异常。调用API接口、库函数或系统服务等，只顾着享受便利却不做防护，常导致因为局部失败而影响整体的功能。最好的防护依然是“防御式编程”。要么在当前方法捕获异常并返回合适的空值或空对象，要么抛给高层处理。

切不可默默"吞掉错误和异常"。如果这样做了，出问题了等着加班和耗费大量脑细胞吧！
在CodeReview的时候一定要仔细询问：这里是否可能会抛出异常？如果抛异常会怎么处理？是否会影响整体服务和返回结果？

低性能

低性能会导致产品功能不好用、不可用，甚至导致产品失败。

常见情况有：a. 循环地逐个调用单个接口获取数据或访问数据库; b. 重复创建几乎完全相同的(开销大的)对象；c. 数据库访问、网络调用等服务未处理超时的情况； d. 多重循环对于大数据量处理的算法性能低；e. 大量字符串拼接时使用了String而非StringBuilder.

对于 a，最好提供批量接口或批量并发获取数据；对于 b, 将可复用对象抽离出循环，一次创建多次使用；对于 c，设置合理的超时时间并捕获超时异常处理；对于 d，使用预排序或预处理，构造合适的数据结构，使得算法平均性能在 O(n) 或 O(nlogn) ；对于 e, 记住：少量字符串拼接使用String, 大量字符串拼接使用 StringBuilder，通常不会使用到 StringBuffer.

影响范围过大

对多个模块依赖的公共函数的修改，容易造成影响范围超过当前业务改动，无意识地破坏依赖于该公共函数的其他业务。要特别慎重。可靠的方式是：先查看该公共函数的调用，如果只有自己的业务用，可适当大胆一些；如果有多个地方依赖，抽离一个新的函数，抽离原函数里的可复用部分，然后基于可复用部分构建新的函数。修改原则遵循“开闭”原则，才能尽可能使改动影响降低到最小化。

基类及实例字段和方法也属于公共函数的范畴。尽量不要修改基类的东西。

单测问题

单测是保证工程质量的第一道重要防线。单测问题一般包括： a. 单测未全部通过； b. 重要业务逻辑缺乏单测； c. 缺乏异常单测； d. 代码变更或BUG修复缺乏单测。

单测全部通过应当是提交代码到代码库以及代码Review的前提条件。代码提交者应当保证单测全部通过。没有捷径可走。仅当单测全部通过才提交到代码库，可以通过工具自动化实现。对于 maven 管理的工程，只需一个命令： mvn test && git push origin branch_name 。单测应当更注重质，而非单纯追求覆盖率。

缺乏单测的重要业务逻辑就像裸露在空气中的电线一样，虽然能跑起来，却是很容易“触电”的。方法：增加覆盖比较全面的单测。

缺乏异常单测也是代码提交者常忽略的问题。异常也是一种实际的业务场景，反映系统的健壮性和友好性。异常应该有相应的单元测试覆盖。创建条件使之抛出异常，并判断异常是否是指定异常；若没有抛出异常或者不是指定异常，则应该 AssertFailed 而不是通过。

对于代码变更和BUG修复，如果当时由于时间紧而没有写，后续应当补上。对于每个代码变更和BUG，都可以抽离出相应的代码部分，并有相应单测覆盖，并注明原因。

与原有业务逻辑不兼容

改动针对当前需求是合理的，却与原有业务逻辑不兼容，也是常见的问题。比如增加一个搜索条件，却不能与原有条件联合查询。

与原有业务不兼容，一般出现在：

一对一与一对多的变化。比如原来的关系是一个订单对应一个物流信息，后来变化为一个订单可能对应多个物流信息；原来的逻辑是一个订单显示多个物流信息可以更改，后来要求一个订单只展示最近一次的物流信息可以修改。
多个业务组合。业务 A 与业务 B 原来是分开发展的，后来开展一种活动，将业务A与业务B进行一种组合营销。此时，多半会出现很多 if-else 语句。

业务逻辑的兼容问题一般体现在系统的复用性和可扩展机制上。良好的系统可复用性和可扩展性可以更容易地做到业务逻辑兼容。主要有如下几种级别：

自动兼容。增加一种类型，只是 biz_type 的值多了一种，系统自动将已有功能适配给新的 biz_type；
一点改动。增加一个分支语句，对 biz_type 的某个特性进行扩展；
一些改动。需要见缝插针地增加一个单独的分支判断和逻辑处理模块，对整体可扩展性没有影响，但会造成局部的复杂化；
一部分功能改动。只需要对其中一个功能模块做个扩展；
多处改动。需要对多个功能模块做相应的改造，不过更多是新增而不是修改；
难以改动。需要深入到功能模块内部做艰难的修改，并要保证原有功能不受影响。

如何应对呢？

针对关联关系，在项目之初，可以询问清楚：将来在产品上是否有可扩展的变化？及早预留空间，或者确定产品上的对策；在代码实现上，兼顾考虑一对一到一对多，或一对多到一对一的关联变化。比如使用列表来表达单个信息，使用索引从列表中获取单个信息。
针对业务组合，明确各业务的核心部分，抽离出业务的可复用的部分，形成 API ；考虑组合模式和装饰器模式来进行扩展。

核心不变，外围定制化。

缺乏必要日志

对于重要而关键的实例状态、代码路径及API调用，应当添加适当的INFO日志；对于异常，应当捕获并添加Error日志。缺乏日志并不会影响业务功能，但出现问题排查时，就会非常不方便，甚至错失极宝贵的机会(不易重现的情况尤其如此)。此外，缺乏日志也会导致可控性差，难以做数据统计和分析。

错误码不符合规范

错误码本身不算是代码问题，不过基于整个组织和工程的可维护性来说，可以将错误码不符合规范作为一种错误加以避免。方法：对错误码进行可控的管理和遵循规范使用。可以使用公共文档维护，也可以开发错误码管理系统来避免相同的错误码。

参数检测缺乏或不足

参数检测是对业务处理的第一层重要过滤。如果参数检测不足够，就会导致脏数据进入服务处理，轻则导致异常，重则插入脏数据到数据库，对后续维护都会造成很多维护成本。方法：采用“契约式编程”，规定前置条件，并使用单测进行覆盖。

对于复杂的业务应用，优雅的参数检测处理尤为重要。根据 “集中管理和处理一致性原则”，可以建立一个 paramchecker 包，设计一个可复用的微框架来对应用中所有的参数进行统一集中化检测。参数检测主要包括： (1) 参数的值类型，可以根据不同值类型做基础的检测； (2) 参数的业务类型，有基础非业务参数，基础业务参数和具体业务参数。不同的参数业务类型有不同的处理。将参数值类型与参数业务类型结合起来，结合一致性的异常捕获处理，就可以实现一个可复用的参数检测框架。参数检测既可以采用普通的分支语句，也可以采用注解方式。采用注解方式更可读，不过单测编写更具技巧。

引用错误

对于动态语言，由于缺乏强大的静态代码检测，修改了类引用的地方尤其要注意，很可能导致依赖的其他业务出错；尤其是修改重名引用时。有线上故障教训。PHP工程中含有两个 Format 类，一个基础的一个业务相关的，被改动的类文件里开始没有指明引用，默认采用了基础 Format 类的实现，然后提交者在改动文件头增加了对业务 Format 的引用，导致依赖于基础Format类的其他业务不能正常工作。避免引用错误的方法：当要在文件里增加新的类引用时，先在文件里搜索是否有重名类的引用。如果有，就要格外小心了。

细节错误

比如数组越界、JSON解析出错、函数参数传递出错、API 版本不对、使用网上拷贝的未经测试的代码、不成熟的算法、传值与传引用、相等性比较等。

对于数组越界错误，通常要对空数组、针对数组大小的边界值+1和-1写单测来避免；使用网上拷贝的代码，诚然可节省时间，也一定要加工一下并用单测覆盖；传值和传引用可通过单测来避免错误；对象的相等性比较切忌使用等号=。

多重条件

类似 if ((!A || !B) && C || (D && E)) 的多重条件要仔细推敲。方法：最好拆分成多个有含义变量。 isNotDelay = !A || !B ; isNormal = C ; isAllow = D && E ; cond = isNotDelay && isNormal || isAllow 。

文不符实

文不符实是一种可能导致线上故障的错误。比如一个 getXXX 的函数，结果里面还做了 add, update 的操作。对问题排查、产品运维等都有非常大的杀伤力。因此命名一定要用实质内容相符，除非是故意搞破坏。

跨语言或跨系统交互

稍具规模的互联网创业公司通常会采用多语言开发，比如PHP作为前端，Java作为后台服务。当动态类型语言与静态类型语言交互时，会有一些问题产生。比如PHP的对象通常是一个Map, 如果是空对象就会写成 [], 然而 [] 会被 Java 解析成列表。这样，如果数据库的值是通过 PHP 写入，那么这个值既有可能是JSON对象字符串，也可能是空数组字符串， Java 来解析就有点尴尬了。同样，当 Java 调用 PHP 接口时，不规范的PHP接口既可能返回列表，也可能返回 true or false ， Java 解析返回结果也会比较尴尬。因此，在跨语言交互的边界处，要特别注意这些类型转换的差异。

跨系统交互则主要是接口设计与约定的问题。同一个项目里不同业务团队之间的业务接口设计与约定，不同企业里开放接口的设计与约定，要在最初深思熟虑，一旦开放，在后期很少有接口设计改动的空间。开放接口设计要符合小而美、正交的特性，命名要贴切一致，参数取值要指明约束，枚举参数要给出列表，结果返回要规范一致，可以采用通用的 {"code":200, "msg": "success", "data": xxx} 。跨系统交互也要统一对术语和接口的理解的一致。

可维护性问题

可维护性问题是“在当前业务变更的范围内通常不会导致BUG、故障，却会在日后埋下地雷，引发BUG、故障、维护成本大幅增加”的类别。

硬编码

硬编码主要有三种情况： a. “魔数”； b. 写死的配置； c. 临时加的逻辑和文案。

“魔数”与重复代码类似，当前或许不会引发问题，时间一长，为了弄清楚其代表的含义，增加很多沟通维护成本，且分散在各处很容易导致修改的时候遗漏不一致。务必清清除。方法也比较简单：定义含义明显的枚举或常量，代表这个魔数在代码中发言。

“写死的配置”不会影响业务功能，不过在环境变更或系统调优的时候，就显得很不方便了。方法：尽量将配置抽离出来做成配置项放到配置文件里。

“临时加的逻辑和文案”也是一种破坏系统可维护性的做法。方法：抽离出来放在单独的函数或方法里，并特别加以注释。

重复代码

重复代码在当前可能不会造成 BUG，但上线后，需要维护多处的事实一致性；时间一长，后续修改的时候就特别容易遗漏或处理不一致导致 BUG；重复代码是公认的“代码坏味”，必当尽力清除。方法：抽离通用的部分，定制差异。重复代码还有一种情况出现，即创造新函数时，先看看是否既有方法已经实现过。

通用逻辑与定制业务逻辑耦合

这大概是每个媛猿们在开发生涯中遇到的最恶心的事情之一了。通用逻辑与具体的各种业务逻辑混杂交错，想插根针都难。遇到这种情况，只能先祈福，然后抽离一个新的函数，严格判断相应条件满足后去调用它。

如果是新创建逻辑，可以使用函数式编程或基于接口的编程，将通用处理流程抽离出来，而将具体业务逻辑以回调函数的形式传入处理。

不要让不同的业务共用相同的函数，然后在函数里一堆 if-else plus switch ，而是每个业务都有各自的函数，并可复用相同的通用逻辑和流程处理；或者各个业务可以覆写同样命名的函数。

复用，而非混杂。

直接在原方法里加逻辑

有业务改动时，猿媛们图方便倾向于直接在原方法里加判断和逻辑。这样做是很不好的习惯。一方面，增加了原方法的长度，破坏了其可维护性；另一方面，有可能对原方法的既有逻辑造成破坏。可靠的方式是：新增一个函数，然后在原方法中调用并说明原因。

多业务耦合

在业务边界未仔细划分清晰的情况下出现，一个业务过多深入和掺杂另一个非相关业务的实现细节。在项目和系统设计之初，特别要注意先划分业务边界，定义好接口设计和服务依赖关系，再着手开发；否则，延迟到后期做这些工作，很可能会导致重复的工作量，含糊复杂的交互、增加后期系统维护和问题排查的许多成本。磨刀不误砍柴工。划分清晰的业务、服务、接口边界就属于磨刀的功夫。

代码层次不合理

代码改动逻辑是正确的，然而代码的放置位置不符合当前架构设计约定，导致后续维护成本增加。

代码层次不合理可能导致重复代码。比如获取操作人和操作记录，如果写在类 XController 里，那么类 YController 就面临尴尬局面：如果写在 YController ，就会导致重复代码；如果跨层去调用 XController 方法，又是非常不推荐的做法。因此，获取操作人和操作记录，最好写在 Service 层， Controller 层只负责参数传入、检测和结果转译、返回。

不用多余的代码

工程中常常会有一些不用的代码。或者是一些暂时未用到的Util工具或库函数，或者是由于业务变更导致已经废弃不用的代码，或者是由于一时写出后来又重写的代码。尽量清除掉不用多余的代码，对系统可维护性是一种很好的改善，同时也有利于CodeReview。

使用全局变量

使用全局变量并没有“错”，错的是，一旦出现问题，排查和调试问题起来，真的会让人“一夜之间白了头”，耗费数个小时是轻微惩罚。此外，全局变量还能“顺手牵羊”地破坏函数的通用性，导致可维护性变差。务必消除全局变量的使用。当然，全局常量是可以的。

缺乏必要的注释

对重要和关键点的代码缺乏必要的注释，使用到的重要算法缺乏必要的引用出处，对特别的处理缺乏必要的说明。

原则上，每个方法至少要用一个简短的单行注释，适宜地描述了方法的用途、业务逻辑、作者及日期。对于特殊甚至奇葩的需求的特别实现，要加一些注释。这样后续维护时有个基础。

更难发现的错误

更难发现的错误是指“复杂并发场景下的有一定技术难度的、需要丰富开发与设计经验才能看出来的错误”。

并发

并发的问题更难检测、复现和调试。常见的问题有：a. 在可能由多线程并发访问的对象中含有共享变量却没有同步保护；b. 在代码中手动创建缺乏控制的线程或线程池；c. 并发访问数据库时没有做任何同步措施；d. 多个线程对同一对象的互斥操作没有同步保护。

对于 a, 在大部分Java应用中，通常由Spring框架来控制和创建请求和服务实例，因此，保证“Controller, Service 类中的实例变量只允许 Service, DAO 的单例，不允许业务变量实例”基本确保没有并发不正确更新的问题；不过，包含缓存策略的对象要特别注意多线程并发访问的问题，出于性能考量，尽量只对共享实例部分加锁。

对于 b, 禁止在应用中手动创建线程或线程池，失控的线程池很容易导致应用崩溃(有线上应用崩溃的教训)。

对于 c, 并发访问数据库时，要特别注意时序和状态同步。如果时序控制不对，会导致状态同步和更新出错。

对于 d, 对同一对象的互斥操作需要加分布式锁同步。

使用线程池、并发库、并发类、同步工具而不是线程对象、并发原语。在复杂并发场景下，还需注意多个同步对象上的锁是否按合适的顺序获得和释放以避免死锁，相应的错误处理代码是否合理。

事务

事务方面常出现的问题是：多个紧密关联的业务操作和 SQL 语句没有事务保证。在资金业务操作或数据强一致性要求的业务操作中，要注意使用事务，保证数据更新的一致性和完整性。

SQL问题

SQL的正确性通常可以通过 DAO 测试来保证。 SQL问题主要是指潜在的性能问题和安全问题。

要避免SQL性能问题，在表设计的时候就要做好索引工作。在表数据量非常大的情况下，SQL语句编写要非常小心。查询SQL需要添加必要索引，添加合适的查询条件和查询顺序，加快查询效率，避免慢查；尽量避免使用 Join, 子查询；避免SQL注入。

SQL优秀书籍推荐： SQL语言艺术

安全问题

安全问题一向是互联网产品研发中极容易被忽视、而在爆发后又极引发热议的议题。安全和隐私是用户的心理红线之一。应用、数据、资金的安全性应当仅次于产品功能的准确性和使用体验。

安全问题的CodeReview可参见检查点清单：信息安全。主要是如下措施： a. 严格检查和屏蔽非法输入； b. 对含敏感信息的请求加密通信； c. 业务处理后消除任何敏感私密信息的任何痕迹； d. 结果返回前在反序列化中清除敏感私密信息； e. 敏感私密信息在数据存储设备中应当加密存储； f. 应用有严格的角色、权限、操作、数据访问分级和控制； g. 切忌暴露服务器的重要的安全性信息，防止服务器被攻击影响正常服务运行。

设计问题

设计问题通常体现在： a. 是否有潜在的性能问题； b. 是否有安全问题； c. 业务变化时是否容易扩展； d. 是否有遗漏的点。

较轻微的问题

较轻微问题是指“没有技术难度、通过良好习惯即可避免的问题”。

较轻微问题一般不会造成负面影响的BUG或故障，不过建立一些好的习惯，主动使用代码检测工具，消除这些较轻微错误，也是一种修行。

命名不贴切

命名不贴切不会影响功能实现，却会误导理解或增加理解难度。

方法：先查查字典，找个通俗易懂而且比较贴近的名字。可以参考 jdk 的命名、通用词汇和行业词汇；作用域小的采用短命名，作用域大的采用长命名。取名字是一种重要技能，—— 多少父母为此愁灰了头！

声明时未初始化

声明时未初始化通常情况下都不会是问题，因为后面会进行赋值。不过，如果赋值的过程中出现异常，那么可能会返回空值，从而导致空值异常。通常，变量声明时赋予默认初始值是个好习惯。

风格与整体有不一致

工程通常求稳，一致性能更好地维护。在工程项目中，最好能够遵循工程约定的风格，在个人项目中可以凸显个性风格。Java编程一般要遵循《Java编程规范》，有追求的程序猿媛还会追求更高层次的，比如《Google Java 规范》等。

类型转换错误

编程语言的类型系统是非常重要的。如何在不同类型之间可靠地互转，尤其是在父子类型之间相互赋值，也是一个微技能。滥用类型转换，也会导致BUG 。

Java 中容易出现的错误是：a. 字符串转数值，字符串含有非数字部分；b. JSON字符串转对象，某个字段含有不兼容的值类型导致解析出错；c. 子类型转不兼容的父类型，滋生运行时异常 ClassCastException；d. 相同特质的类型不兼容。比如 Long 与 Integer 都是数值型，却不能互转。

类型转换中最容易出BUG的地方是非布尔类型取反。受C语言的影响，很多高级语言支持各种数据类型转布尔类型，比如 PHP 字符串、数组、数字等都可以转布尔类型，相应的就喜欢写 if (!notBoolVar) 这种表达式，容易隐藏看不出的BUG甚至错误。

否定式风格

变量含义、表达式语句倾向于使用否定式风格，可能不知不觉耗费大量脑细胞，因为每次理解的时候都要绕个弯子。比如 isNoExpress 是否无需物流，就有点绕。为什么呢？无需物流是针对快递发货的，如果快递发货占发货的90%，无需物流只占10%，那么, isNoExpress = false 几乎总为真。涉及到判断的时候，可能不得不写 if (!isNoExpress) ，双重否定足够弄晕你。

容器遍历的结构变更

绝大多数语言都承袭了 C 语言的 for(int i=0;i<N;i++) 循环形式。不过，现代编程语言通常都提供了迭代器遍历、或 foreach 遍历。 foreach 遍历通常基于迭代器遍历实现。只要对容器结构不做变更，推荐使用 foreach ；若要遍历的同时做修改或更新，推荐迭代器模式。遍历容器的时候同时做删除元素操作，要特别留意，很可能导致越界错误。更可靠的方式时，直接生成新的容器，如果不涉及空间效率的话。

API参数传递错误

如果API参数有多个，而且相邻参数的类型相同，那么要特别留意是否参数顺序是正确的，而不会张冠李戴。

当然，在设计API参数的时候，就可以仔细用更精准类型进行区分，并将相同类型的参数错开。比如 calc(int accountNo, int pay, int timestamp) ，就容易传错，比较可靠的是 calc(int accountNo, Currency pay, Timestamp now) ，这样是不可能将参数传递错误的。

单行调用括号过多

为了简便，常常会写出 wapper(calc(now, String.format("%s\n", new BufferedFileReader(filename, "UTF-8").readLines() ))) 的语句 , 嗯，你得好好瞧瞧和算算右边的括号数量是否正确了。更糟糕的时候，结合API参数传递错误，IDE 可能没有报错，而你很可能没有意识到自己的参数传递错误了。可靠的方式是，拆出一部分变量，并将调用之间的括号用空格隔开，显示出层次感。

String fileContent = new BufferedFileReader(filename, "UTF-8").readLines();
wapper( calc( now,  String.format("%s\n", fileContent) ) )

修改方法签名

对某个方法有业务改动时，程序猿媛们倾向直接修改原方法的签名。这时，要特别注意：a. 不要修改原方法的参数顺序； b. 在最后面增加可选参数。从另一个角度来看，复杂的业务方法应当分两层：最外层负责调度，方法参数具有包容性，里面包含的字段比较多；内层方法负责特定业务逻辑的实现，方法参数少而精。

修改原方法签名本身就是容易产生问题的习惯，篡改原方法的参数顺序更是大忌。最好的方法是新建一个方法去复用原方法，然后调用新的方法。代码变更始终铭记“开闭”原则。

打印日志太多

打印过多的日志并不好。一方面遮掩真正需要的信息，导致排查耗费时间，另一方面造成服务器空间浪费、影响性能。生产环境日志一般只开放 INFO及以上级别的日志； Debug 日志只在调试或排错的时候使用，生产环境可以禁止debug日志。

多级数据结构

使用多级数据结构时，要确定父级数据一定有值，或者进行检测。比如 $order['baole']['ump']['money']，必须确保 $order['baole'], $order['baole']['money'] 一定有值或做非空检测。

作用域过大

由于C语言的影响，猿媛们会在开头就定义好一些变量或要返回的对象，在很靠后的地方才使用到。不必要的过大的作用域对变量和对象的变化产生不可测的影响，并增大理解的成本。可靠的方法是，仅当在使用时才定义，并尽快返回结果。

另一种情况是，暴露的访问域过大，比如 public 字段。尽可能地缩小可访问的范围，可以增大变更和重构的空间；减少可变性，则可以自然地获得并发安全性，降低CodeReview的理解成本。

比如，不可变的类和字段定义成 final , 最小化包，类，接口，方法和域的可访问性，默认为 private , 若需要继承，可定义为 protected , 仅当需要作为 API 服务暴露出去时，使用 public.

分支与循环

条件与循环偶尔也会导致错误，不过通常错误可以在发布前解决掉。

对于 if-else 嵌套条件，需要仔细检查是否符合业务逻辑；如果嵌套太深，是否可以使用另一种方式“解结” ；对于 switch 语句，大多数语言的 case 有 fall through 问题，要注意加上 break ；最好加上 default 的处理。

对于 for 循环，编写合理的结束条件避免死循环；对于循环变量的控制，避免出现 -1或 +1 错误，消除越界错误； for 循环也要特别注意对空值和空容器的处理，避免抛出空值异常。可以通过单测来确保 for 循环的准确性。

高效CodeReview

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

改动点和改动描述

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

问题分类与优先级

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

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

有针对性

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 少许改动造成遗漏；
改动最小化：改动文件数代码数尽量越小越好，减少调用方兼容代码更改；不改“陌生”代码：不要自己更改前端代码(涉及不熟悉的打包流程)；不要更改自己不熟悉的代码；

代码提交建议标准

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

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

阶段性小结

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 基础