代码质量分享 2016_06_24_舒琴_代码质量.key
For 代码提交人
基本原则
- Review时机： 对于普通bugfix或优化，CodeReview最迟要在发布前一天或发布当天早上； 对于项目，CodeReview 最迟应该在QA提测前一天；
- 持续增强： 每次提交变更尽可能小一点， 但是保证每次的变更都是正确和完整的， 合并到代码库中持续增强系统。
- 去除无用改动： 仔细斟酌每一行改动，去掉无用的注释，reset 掉没有逻辑改动（空行改动）的文件；改动越少越好；
- 合并提交： 如果仅有一个分支，且全部是自己的改动，合并成一次提交，避免漏掉某些重要Review；
- 分支名： 使用 trade_改动简述_年月日 作为分支名，比如 hotfix/trade_express_service_20160818 ;
- 相关性： 组内业务的相关改动放在单独分支里方便Review ; 避免在一个大而全的项目分支里Review 少许改动造成遗漏；
- 改动最小化： 改动文件数代码数尽量越小越好，减少调用方兼容代码更改；
- 不改“陌生”代码： 不要自己更改前端代码(涉及不熟悉的打包流程)；不要更改自己不熟悉的代码；
自检基本要求
第一级
| 类别 | 建议要求 |
|---|---|
| 变量 | 使用枚举或常量替代魔数； 变量声明时初始化赋默认值； 仅在变量使用的时候声明和初始化（缩小变量的作用范围）；避免使用全局变量； |
| 命名 | 变量与函数的命名做到通俗易懂见名知义，长短适宜； 可以参考 jdk 的命名、通用词汇和行业词汇； 作用域小的采用短命名，作用域大的采用长命名； |
| 规范 |
符合Java编程规范，参见附注的 《Google Java 规范》 ； 风格与工程的整体保持一致； |
| 语言细节 | 注意语言细节的影响，比如 isset 与 empty 的区别； |
| 类型转换 | 尽可能避免使用类型转换，容易滋生运行时异常 ClassCastException ； |
| 直白表达 | 不可对非布尔变量进行取反运算（烧脑）；尽可能使用肯定句而不是否定句； 尽早 Return ； |
| 多重条件 | 将多重逻辑条件中的逻辑拆分为多个布尔变量，而不是写一个大的逻辑表达式； 多重条件注意运算符的优先级； |
| 容器遍历 | 尽可能使用 foreach 或 迭代器模式来遍历，少用 for(int i=0;i<N;i++) 方式； 避免数组越界错误 |
| API 调用 |
调用多个参数类型相同的函数或方法时，检查参数顺序是否正确； 对返回的对象进行空判断，避免空指针异常； 调用外部服务接口必须进行异常捕获或调用成功判断； |
| 括号 | 切忌在一行里使用过多的括号； 使用变量存储函数返回结果，减少括号的使用；括号使用过多可能导致无意识的函数调用的参数错误； |
| 测试 | 每一行改动应当有单元测试覆盖保证； |
| 新增逻辑 | 创建一个新方法来包含这些逻辑，并在原方法里调用；避免直接在原方法里添加大段的新增逻辑； |
| 方法签名 | 修改原有方法的签名时，a. 禁止修改原有参数顺序； b. 新增参数一定要放在最后面，并且添加默认值，以兼容之前的处理； c. 检查和确定修改该方法的影响范围； |
第二级
| 类别 | 建议要求 |
|---|---|
| 错误码 | 错误码符合公司和业务组制定的规范，参见 ErrorCode规则； |
| 健壮性 | 必须捕获抛出异常并进行处理（日志记录或业务逻辑）；宁可返回空值，也不要因为未捕获异常导致霸王龙页面； |
| 日志 | 打印适宜而关键的 info 和 error 日志（关键路径和关键状态）； debug 日志只在开始调试的时候使用，生产环境禁止debug日志； |
| 配置 | 可调节的运行时配置，尽量在配置文件中配置； |
| 异常测试 | 异常捕获也要有单元测试。创建条件使之抛出异常，并判断异常是否是指定异常；若没有抛出异常，则应该 AssertFailed 而不是通过； |
| 容器变更 | 使用迭代器模式在遍历的时候删除； 使用新的容器来容纳容器新增元素； |
| 临时逻辑 | 使用新增方法包含临时逻辑，并在主流程中进行调用和隔离； |
| 多级结构 |
使用多级数据结构时，要确定父级数据一定有值，或者进行检测。 比如 $order['baole']['ump']['money']，必须确保 $order['baole'], $order['baole']['money'] 一定有值或做非空检测； 在函数中使用多级数据结构时，要进行非空检测，比如 doSomeWith($order['baole']['ump']['money']) 确保 $order['baole']['ump']['money'] 为空时不出问题。 |
For 代码审查人:
设计与代码组织
- 影响评估： 变更代码是否覆盖了需求的范围，没有遗漏（比如覆盖多个终端，多个页面，多个业务等）；
- 代码组织： 变更代码是否放置在合适的分层或模块， Controller, Service, Dao , Api , Components, Utils 等 ； 是否放在合适的命名空间、包组织下；
- 接口设计： 接口设计的参数与返回值是否正交，足够灵活；
- 变化分离： 变更代码是否将特定业务逻辑耦合写在通用逻辑里面； 应当将变量与不变量分离；
- 公共逻辑： 变更代码是否更改了公共逻辑，影响了其他的业务或更大的业务范围；
公共逻辑原则上不建议改动；若迫不得已改动一定要慎之又慎，因为影响范围可能波及大量业务，稍微不慎会导致P1级及以上故障！
可以全局搜一下方法名，确定影响范围； - 引用变更： 修改了类引用的地方尤其要注意（有线上故障教训）；
- 事实单一： 变更代码是否重复了多段相似代码；是否同一个业务点采用了多个不同的实现方式；
封装新函数时候可以看下之前接口是否已经有，有则无需新函数，避免维护多套; 每一个业务点或通用逻辑封装成单独的方法或函数； - 基类修改： 尽量避免直接修改基类，因为会影响所有的子类； 使用适配器、委托模式来扩展基类的功能。
进阶检查点
- 检查业务逻辑时， 一定要向代码提交者请教清楚相关的业务才可以；
API调用： 是否是关键API，需要对返回数据添加日志，方便问题排查；
- 重复创建： 在循环里重复创建新的可复用对象或开销很大的对象；
- 超时设置： API 调用、数据库访问等是否设置了超时机制；
- 测试： 检查单元测试是否充分，异常是否有单测覆盖，是否可以全部运行通过；
- 边界： 边界情况是BUG事故多发地，检查是否使用了单元测试覆盖；
- 日志： 业务的关键状态和关键路径是否有日志覆盖，关键业务是否有日志埋点；
- 性能： 循环里对每一个实例调用接口，而不是使用批量接口；是否使用了多重循环，算法性能是否可接受；减少重复调用；
- 并发： Controller, Service 类中的实例变量只允许 Service, DAO 的单例，不允许业务变量实例；是否使用了线程池或连接池而不是单个的线程连接；
- 事务： 多个 SQL 语句提交时是否需要使用事务；
- SQL: 添加sql语句时候尤其分库分表量很大的时候一定看下表的ddl ，能否用上索引，从源头避免慢查 ;
- 安全： 敏感信息是否使用了加密；是否可能存在 SQL 注入攻击的可能；
附注
Code Review 检查什么
1. Code Review 重点在于检测：
a. 业务逻辑的实现是否未考虑到全局的设计或现有的某些业务细节（对业务不够熟悉的同学往往因为没有考虑到更大的业务范围或细节而返错）；
b. 是否有隐藏的细微错误或潜在的隐患（经验判断）;
c. 代码的质量属性，性能、可维护性、可扩展性等，对需求和设计的代码实现方式；
2. Code Review 不应该检测编程风格和低级错误。 自检基本要求两次不过关的同学要暂时剥夺其提交代码的资格；
3. Code Review 不应该承担发现编译与运行错误的职责；代码中的编译与运行错误应该由单元测试，接口测试，回归测试来消除。
Code Review 检查类型及优先级
General Unit Testing 单元测试 = High
Error Handling 错误处理 = High
Resource Leaks 资源泄露 = High
Thread Safety 并发安全 = High
Performance 性能问题 = High
Functionality 功能问题 = High
Security 安全问题 = High
Redundant Code 重复或无用代码 = High
Control Structures and Logical issues 控制结构和逻辑错误 Medium or High
Comment and Coding Conventions 注释与代码风格 = Low
检查点详细清单参考
| 检查项 | 清单项目 | 分类 |
| 信息安全 或权限访问 |
如果不用于继承，使类为final | 可访问性的扩展(Accessibility Extensibility) |
| 最小化包，类，接口，方法和域的可访问性 | 可访问性的扩展(Accessibility Extensibility) | |
| 为native方法定义包装类(而不是定义native方法为public) | 可访问性的扩展(Accessibility Extensibility) | |
| 注释出安全相关的信息 | 文档化 | |
| 系统的输入必须检查是否有效和在允许范围内 | 拒绝服务(Denial of Service) | |
| 检验输入是否含有非法或恶意字符， 防止注入性攻击 | 拒绝服务(Denial of Service) | |
| 避免对于一些不寻常行为的过分日志 | 拒绝服务(Denial of Service) | |
| 把从不可信对象得到的输出作为输入来检验 | 输入检验(Input Validation) | |
| 对命令行执行的代码，需要详细检查命令行参数 | 输入检验(Input Validation) | |
| WEB类程序检查是否对访问参数进行合法性验证 | 输入检验(Input Validation) | |
| 使public static域为final(避免调用方(caller)修改它的值) | 访问限制 | |
| 避免服务器暴露应用系统的目录结构的配置 | 访问限制 | |
| 定义合理的角色权限分级， 并授予合适的人员 | 访问限制 | |
| 从异常中清除敏感信息(暴露文件路径，系统内部相关，配置) | 私密信息(Confidential Information) | |
| 不要把高度敏感的信息写到日志 | 私密信息(Confidential Information) | |
| 考虑把高度敏感的信息在使用后从内存中清除 | 私密信息(Confidential Information) | |
| 重要信息如密码的保存是否选用难以破解的不可逆加密算法 | 私密信息(Confidential Information) | |
| 通讯时考虑是否使用了安全的通讯方式 | 私密信息(Confidential Information) | |
| 避免暴露敏感类的构造函数 | 对象构造 | |
| 避免安全敏感类的序列化 | 序列化反序列化(Serialization Deserialization) | |
| 通过序列化来保护敏感数据 | 序列化反序列化(Serialization Deserialization) | |
| 小心地缓存潜在的特权操作结果 | 序列化反序列化(Serialization Deserialization) | |
| 只有在需要的时候才使用JNI | 访问限制 | |
| 并发安全 | 代码中所有的全局可见可变变量是否是线程安全的 | 并发 |
| 需要被多个线程访问的对象是否线程安全，有无通过同步方法保护 | 并发 | |
| 同步对象上的锁是否按相同的顺序获得和释放以避免死锁，注意错误处理代码 | 并发 | |
| 是否存在可能的死锁或是竞争 | 并发 | |
| 对共享可变的数据使用同步访问 | 并发 | |
| 使用executors而不是task和thread ； 使用并发库、框架而不是线程对象 | 并发 | |
| 性能 | 同步方法是否过度使用， 同步区域是否过大 | 并发 |
| 拼接大量字符串时是否使用 StringBuilder 而不是 String | 综合编程 | |
| 处理大量数据时，是否选取了合适的数据结构和高效的算法 | 综合编程 | |
| 对hashtable,vector等集合类数据结构的选择和设置是否合适，如正确设置capacity,load factor等参数，数据结构的是否是同步的 | 综合编程 | |
| 有无滥用String对象， 不停地创建 String 对象 | 综合编程 | |
| 是否采用通用的线程池、对象池、连接池等cache技术以提高性能 | 综合编程 | |
| 是否采用内存或硬盘缓冲机制以提高效率 | 综合编程 | |
| I/O方面是否使用了合适的类或采用良好的方法以提高性能（如减少序列化，使用buffer类封装流等） | 综合编程 | |
| 递归方法中的叠代次数是否合适，应该保证在合理的栈空间范围内 | 综合编程 | |
| 如果调用了阻塞方法，是否考虑了保证性能的措施 | 综合编程 | |
| 避免过度优化，对性能要求高的代码是否使用profile工具，如Jprobe等 | 工具使用 | |
| 通用 | 避免重复代码 | 综合编程 |
| 在public类中，使用访问器方法(getter/setter方法)而不是public域 | 类和接口 | |
| 最小化局部变量的范围， 需要使用时才定义 | 基础 | |
| 遵循广泛接受的命名规则， 排版、缩进、空白行、格式化等 | 基础 | |
| 使用枚举来代替 int, string 常量 | 基础 | |
| 使用标记接口(marker interface)来定义类型 | 基础 | |
| 数组类结构是否做了边界校验 | 基础 | |
| 变量在使用前是否做了初始化 | 基础 | |
| 注释中适宜地描述了方法的用途、业务逻辑、作者及日期 | 基础 | |
| 静态代码检查 | 查看静态代码分析器的报告来进行类的添加和修改 | 工具使用 |
| 资源回收与泄露 | 是否集合中的失效对象的reference 已经设置为 null 可以被回收 | 综合编程 |
| 是否所有的资源对象被正确释放，如数据库连接、Socket、文件等 | 综合编程 | |
| 资源是否被释放多次 | 综合编程 | |
| 避免使用finalizer | 综合编程 | |
| 控制流与逻辑 | 循环初始值、结束条件以及循环变量递进是否正确 | 基础 |
| 是否避免了死循环的产生（for, 递归， 对象嵌套） | 基础 | |
| 循环次数是否正确 | 基础 | |
| 是否在 for 循环的过程中删除元素 | 基础 | |
| switch case 是否缺少 break 和 default; | 基础 | |
| if-else 嵌套是否正确 | 基础 | |
| &&， ||， ！ 逻辑运算符是否使用正确 | 基础 | |
| 数据库访问 | 数据库设计或SQL语句是否便于移植（注意和性能方面会存在冲突） | 综合编程 |
| 数据库资源是否正常关闭和释放 | 综合编程 | |
| 数据库访问模块是否正确封装，便于管理和提高性能 | 综合编程 | |
| 是否采用合适的事务隔离级别 | 综合编程 | |
| 是否采用存储过程以提高性能 | 综合编程 | |
| 是否采用PreparedStatement以提高性能 | 综合编程 | |
| 网络通信 | socket通讯是否存在长期阻塞问题 | 综合编程 |
| 发送接收的数据流是否采用缓冲机制 | 综合编程 | |
| socket超时处理，异常处理 | 综合编程 | |
| 数据传输的流量控制问题 | 综合编程 | |
| 错误处理 | 每次当方法返回时是否正确处理了异常，记录日志到日志文件中 | 日志 |
| 是否对数据的值和范围的合法进行校验 | 输入检验(Input Validation) | |
| 在出错路径上是否所有的资源和内存都已经释放 | 综合编程 | |
| 所有抛出的异常都得到正确的处理，特别是对子方法抛出的异常，在整个调用栈中必须能够被捕捉并处理 | 异常 | |
| 当调用导致错误发生时，方法的调用者应该得到一个通知 | 异常 | |
| 对可以恢复的情况使用已受检异常(checked exceptions)，对于程序错误使用运行时异常(runtime exceptions) | 异常 | |
| 是否更多地使用标准异常 | 异常 | |
| 是否定义了具有合理名称的自定义异常 | 异常 | |
| 是否“默默地吞掉了”异常 | 异常 | |
| 面向对象编程 | 通过接口而不是实现类来引用对象， 是否符合面向接口编程的思想 | 设计与重构 |
| 方法API是否被良好定义， 便于维护和重构 | 设计与重构 | |
| 重写对象的equals时， 总是重写hashCode | 基础 | |
| 总是重写对象的 toString | 基础 | |
| 对象的传值或传引用方面有无问题 | 基础 | |
| 需要 update 的对象的值是否正确地设置 | 基础 | |
| 是否大量或频繁地创建临时对象 | 基础 | |
| 是否尽量使用局部对象(堆栈对象) | 基础 | |
| 是否使用了全局可变对象且在某处代码里进行了修改 | 基础 | |
| 是否修改了全局可见 final Reference 的内容 | 基础 | |
| 在只需要对象reference的地方是否创建了不必要的对象实例 | 基础 | |
| 类的接口是否定义良好，如参数类型等，避免内部转换 | 基础 | |
| 是否有丑陋的强制类型转换 | 基础 | |
| 是否存在不必要的使用反射来获取私密信息 | 基础 | |
| 返回堆对象的reference，不要返回栈对象的reference | 基础 | |
| 测试 | 代码变更存在有效的单元测试用例 | 基础 |
| 是否对错误处理部分的代码进行了测试 | 基础 | |
| 代码规范 | 是否符合JAVA编码规范(Java Code Conventions) | 标准规范 |
| 其它 | 配置信息如何获得， 是否有硬编码 | 配置 |