Code Review 规范

写在前面

• Code Review并不是一个可有可无的过程，很多技术团队都将该过程视为必要的工作环节，甚至一些技术团队会设置轮值的CR组，进行阶段的CR工作，长期的坚持对团队整体的质量和工作能力提升，都是非常可观的，应被重视。
• Code Review一定会占用时间，无论何种形式的Review都是，如果决定进行代码审查，那么这部分内容需要被等同于开发过程一样，安排在工作计划内，不能因时间紧张的问题，就粗糙了CR过程。
• 尽管CodeReview是重要的，但并不意味着在一个项目周期内的任何时间，都适合把这个过程作为必要流程，如果处于项目的起步或排期相对紧张的阶段，CR过程可以酌情追加，以事后的CR来代替开发过程中的。
• 作为一个开发人员，经常会有这样的感觉，自己写代码容易，看别人代码却无从下手，但事实上难免会需要接手别人的工作与代码。学会阅读不熟悉的代码并找出重点，是一个必须的学习的过程，所以，从认识上需要具备一定的认知：会看代码与会写代码一样重要。

关于内容

本文针对CodeReview过程介绍，通俗点讲，主要回答了四个问题：

• CR目的 - 为啥看
• CR目标 - 看什么
• CR人选 - 找谁看
• CR过程 - 怎么看

内容上，结合团队在开发过程中实际发生的问题及出现的状况，找到对应CR过程中的解决点，予以给出方案。

流程上，目前只细化了一部分流程，其他流程仍需补充，欢迎大家积极讨论，提出意见，不断完善。

希望本文能对实际的CR过程给出一定帮助，也希望每个技术人员能在CR过程中获得成长。

内容

CR目的

CR虽然最直接的目的是为了发现问题，但这里的问题不是代码错误，而是缺陷，隐患等内容。需要明确一点的是，有两点内容不应由CR保证：

• 不承担发现代码错误的职责。CR目的为审查代码质量，包含可读性、可维护性、程序需求逻辑设计实现。BUG错误应由开发人员自身单元测试保证，开发人员未经自测的内容审查无意义。
• 不作为保证代码风格和标准的手段。目前有Java开发指南等很多成熟的标准，按统一的标准进行开发是开发的基本能力，如果存在指南，则不需要由CR过程指出。

CR方法论最终目标是，如何提高CR效果并缩短时间，如果所有的问题都交由CR来做，那么时间会不可控的增长，费时费力，某些问题发现还不如测试，长而久之，难免厌烦。只关注CR该做的事，而不要想着将所有流程纳到CR中来。CR目的主要包含：

• 功能确认：在代码诞生早期就可以确认完整的需求的关键点，发现代码中的功能缺陷。
• 隐患挖掘：很多功能不存在明显BUG，但随着用户量增长，访问量激增，可能存在隐患，开发过程可能忽略了细节。
• 素质约束：开发人把自己的作品赤裸裸的呈现在别人面前，由别人评价，不再只是呈现外在功能而是呈现内在细节，每个人都是要面子的，为了CR会不由自主的尽可能提升自己的代码质量，约束自身的开发素质。
• 知识共享：每一个人都有自己的设计思路与开发习惯，帮助开发人员互相学习彼此代码中的经验，看到更多的形式，学习别人做的好的地方。
• 错误共享：人在错误中成长，技术亦然，多看看别人可能会犯的错误，看看踩过的坑，以提醒自己。
• 内容维护：保证同一个业务至少有两个人知道，同一个业务实现至少有两人熟悉，当出现线上事故或需要紧急业务调整时，不受制于人（同于结对编程）。
• 优化实现：增强团队成员间的沟通及凝聚力，产生思维碰撞，优化实现形式。
• 产品维护：CR的结果是长期的代码质量把控与维护成员的共同经验，尽管随着产品的迭代周期不断增长，但产品的维护成本会控制在有效的范围内，不会产生激增。

CR目标

CR的过程不能覆盖到整个代码的周期，不能代替开发人员自己的单元测试以及规范的测试流程，基本功能的通畅保证仍需开发人员提前保证。同时，为保证CR的顺利进行，也需要团队中使用统一的Code Formatter和规范插件保证代码可读性，当然也要善用sonar分析报告的结果。CR要看的内容与目标，大致可以归纳为看与不看两方面：

看

• 功能要点：该做的功能是不是都在，某需求的关键回路是不是覆盖完全，是否存在业务特殊场景未包含，关键要点的实现是否合理。
• 资源消耗：以单一功能接口为例，某接口访问其他资源，诸如DB、Cache、HTTP请求的次数是否合理。
• 结构形态：对于某一功能，包含多个系统的子功能，拆分上是否合理；各服务之间是否职责明确，结构分层清晰；单一接口上，在同步处理时，是否存在部分处理过程可修改为异步。
• 设计模式：在具体功能实现上，是否使用了确切的设计模式，是否存在冗余部分，是否存在可抽象部分，是否存在扩展性有待提升部分。

不看

• 代码格式：通过统一的IDE Formatter（推荐alibaba-p3c-codestyle）保证。
• 使用规范：通过统一的的规范检查插件（Alibaba Java规范）保证。

CR人选

CR的形式多种多样，目前比较通用的形式有一下几种：

• 会议式CR：组织相关人员坐到一起，按照功能，一块一块的过。
• 答辩式CR：开发人结合自己的代码，给相关人员介绍设计思路与实现流程，当场提问与答辩指出问题。
• 讲解式CR：开发人与CR人（多为1对1），讲解关键代码变更，CR人负责审查。
• 离线式CR：开发人提交代码变更，选定CR人，CR人收到审查请求，对相关代码进行审查。

其实就上述形式而言，前两种形式有团队在用，但其实在CR推行初期仍不建议这样做，这种形式会占用很多人的大部分时间，较难坚持难以推行。在CR成为习惯之前，仍建议以讲解式CR与离线式CR相结合，人员参与少，执行迅速，形式灵活，可行性较高。

首先，CR不一定要找前辈，只要适合即可，可以参考：

• mentor或者对此功能有了解的人
• 本周有时间review
• 你评估可以完成对此功能review的人（跟入职年限无关）

几点心得：

• 闻道有先后，术业有专攻，每个人都需要被CR，也有CR他人的机会，团队内都是平等的，互相学习，共同进步。
• 每次CR，尽可能多让不同的人做，每一个人有不同理解和认知，不同的人会从不同的角度审视和评论自己的代码，有的根据实现、有的根据需求、有的根据性能、有的根据扩展性，这样对自身代码的提升是全面的。

CR过程（重点）

这里主要以讲解式CR与离线式CR进行说明。

事先准备

开发人

沟通

• 需求沟通。开发人与审查人事先沟通需求内容
• 设计沟通。如果条件允许，可以一并交代下设计，至少包含是否有异步流程，涉及哪些系统。

代码

• 写好注释。开发人如果希望CR时，尽量减少审查人事后询问，关键实现过程请写明逻辑注释，养成好习惯，提升代码可读性。
• 格式统一。代码编写时，遵循通用formatter与编码规范检查（Alibaba插件），提升代码可读性。
• 自测完备。开发人需要至少完成单元测试级别自测，确保运行通畅。
• 体量分割。如果条件允许，不要一次性提交特别多的代码，需求间能拆分的，尽量拆分成多个分支进行提交。
• 文档配套。对于一些复杂的功能，如果在开发时做了相关设计，有设计文档或设计图，则一并交付给审查人。

审查人

浏览资料

• 对于需求文档一定要做到了解，方便在CR过程中功能覆盖的确认。
• 对于设计文档也要一定的理解，方便CR过程对实际代码逻辑的梳理。

构思设想

• 拿到需求时，也要自己先构思一个设计，要是自己来搞会怎么做，带着问题去看设计文档，有什么异同，并在之后的CR过程中逐个的去验证，比较下自身的设计思路与开发人的设计思路的优劣之处，想想哪些是开发人忽略的，哪些却是自己缺少的。

执行流程

1、代码完成时，第一时间考虑CR人选，与其进行沟通时间安排后选择合适人选，在git发起Merge Request（只会比对文件，不会真Merge，确认后才Merge）。

• a. 开发人事先将master向自己的开发分支进行合并并解决冲突，确保自身分支之后不会与master产生冲突。
• b. 选做：将master向CR镜像分支进行合并，以保证CR镜像分支版本不低于当前master。正常情况下，CR镜像分支永远都同于或高于master版本，如果开发分支经由release合并至master后不再修改，则CR镜像分支无需每次都合并master，此合并主要应对临时修改或遗漏。
• c. 提出Merge Request，从开发分支合并至CR镜像分支

2、功能正常提测，排期允许时，在CR完成后提测上线，做到预防风险，在排期非常紧急时可以在提测后，同时进行CR工作，做到预防下一次风险。但所有开发分支都必须提出Merge Request。能及时则及时，因为开发人当时的记忆是最深刻的，会降低CR成本。

3、CR人自己可以安排时间做本阶段的CR工作

• a.通过Merge Request中的开发分支与CR镜像分支变动，如果不便也可经由本地git工具，进行内容确认。
• b.对于代码问题或者疑问可以直接写在评论内，方便回溯，会有邮件通知相关的开发。
• c.CR完成后，如果无需修改变更，则点击完成，执行开发分支到CR镜像分支的合并。

特别说明：由于Merge Request的顺序有先后，可能会在Merge RequestCR镜像分支出现冲突的情况，处理方法如下:

• 后发现冲突的开发人，将自身的开发分支B与CR镜像分支合并成一个CR提审分支解决相关冲突，再发起CR提审分支到CR镜像分支的Merge Request。
• 如果两分支的冲突无业务耦合，只是工具代码冲突，则开发分支A与CR提审分支选定两分支各自的CR人进行审查。
• 如果两分支本身存在业务耦合，建议两开发人互为CR人，执行讲解式CR，对冲突进行确认。

流程图

审查要点

根据CR目标内容，参考目前在代码审查中的一些理论关键点，结合Java后台开发的实际过程，细化CR的要点大致包含：

完整性检查（Completeness）

• 代码是否完全实现了设计文档中提出的功能需求，流程正确，且无丢失。
• 数据源（数据库、Redis）部分，是否形态合理（表设计（列项选取，冗余等内容）、缓存结构设计），涵盖所有功能需求的数据。

一致性检查（Consistency）

• 代码逻辑符合设计预期，即便是小修改，至少跟设计文档方向一致
• 代码格式、风格一致（如果使用了统一的formatter，由格式化插件保证）

正确性检查（Correctness）

• 如果有统一变量命名要求，代码是否满足要求，不引起歧义
• 注释和实现相符（因为看代码时，会先看注释来了解逻辑，再看实现，如果存在不一致，很容易发现）
• 如果功能会导致旧代码发生变动，那么变动位置的选择是否合理（同一功能若干层实现，动哪都能实现，关键是好不好）

可修改性检查（Modifiability）

• 如果使用到了常量字典，所用的常量是否是可被枚举的，是否使用了枚举
• 所用到的常量，是否可能需要可配，如果需要可配，是否使用了可配置的实现（ini等）
• 封闭原则，代码是否只暴露了应该暴露的接口，对其他不应暴露的内容是否良好封闭，使其他使用者使用时尽量简单
• 关注业务的扩展可能，代码在实现上，能够提供扩展能力的地方是否预留了扩展路径

可预测性检查（Predictability）

• 参数验证：某语法段或功能节点存在前值或上层依赖时，是否对前值依赖进行过参数验证。（习惯性验空，大量依赖内存的场景，内存bean没刷到，为空是否考虑到）。
• 循环与递归：在带有条件的循环与递归过程中，是否存在某种形态导致死循环或无穷递归
• 资源使用：在目前的实现逻辑下，DB、Cache等资源使用上是否合理，如果访问量上升，是否有优化性能的可能。
• Corner Case：是否存在边界情况，代码没有覆盖完全。一表现为逻辑上的逻辑回路完全，二表现在数据内容上，是否有未做判定的异常形态数据。

健壮性检查（Robustness）

• 语法段上，常见数组越界，0除，堆栈溢出等问题，是否有所规避（部分内容，插件会有所提示）
• 对于可能发生异常的语法段，在必要条件下，是否做了单独的try catch，以防影响至上层依赖
• 代码最外层提供服务的接口处，是否有保底的异常捕获处理，防止某层异常捕获遗漏
• 对于可捕获的异常状态，是否在需用事务的地方给出了事务。

结构性检查（Structuredness）

代码结构

• 在某一功能实现上，是否对各个原子功能进行合理分层
• 在同层的方法内，是否存在方法体过长的方法，是否进行了合理拆分
• 关键业务方法上，是否职责独立且完全，“如果一个函数，别人需要看你的长篇注释才能明白，那这个函数就一定有重构的空间”。
• 关键业务方法上，如果出现了非常复杂难以阅读的逻辑，考虑是否存在一个相对简单的逻辑进行优化。
• 每一个独立功能，是否可分割为独立代码块，耦合尽量低
• 某些业务功能的实现方法，是否使用了设计模式，以提高代码整体的效果，设计模式选用是否合理
• 数据结构设计，对于业务的细节代码，所选用的数据结构是否合理（List or Set? HashMap or LinkedMap?）

设计结构

• 分布式结构下，多职能系统所分管的功能部分是否合理
• 多系统间的交互顺序是否合理，是否出现了交互顺序矛盾

可理解性检查（Understandability）

• 关键逻辑部分，注释是否能够清晰描述内容
• （可选）类及方法命名上，是否满足命名规则，是否能概述内容，是否过长
• 在可预估值范围的变量上，是否初始了合法的取值范围（想想线程池）

可追溯性检查（Traceability）

• 关键节点上该有的日志是否给到位了，想想这如果有bug，前后是否有注释能够反馈出问题
• 注释内容是否合理，该打印的关键参数是否是都有，但也不要一股脑的什么都打出来，日志太长一是占资源，二是实际也便查找问题

审查流程（重点）

此处以包含客户端的接口功能为例，CR审查流程具体介绍（结合目前项目常用思路整理，可能对其他项目组建形式，不能直接套用，仅供参考）。

宏观把控

• 流程合理

• 同步流程上，是否有适合改为异步处理的部分，是否合适使用了线程或消息队列改为异步处理。
• 异步流程上，如果有异步流程会受同步流程中的事务影响，是否能合并至同步流程中，如果不能，在面临事务回滚时，是否做到合适处理。

• 性能评估（接口素质三问）

• 访问预估。开发者是否对该接口的访问量、访问频次等并发因素有过预估。
• 资源使用。该接口除当前服务器内存外，使用了哪些其他服务，使用了哪些其他资源消耗，诸如Db、Redis、Memcache、HTTP请求等。
• 资源梳理。该接口在不利状况下，单次访问内使用其他各个资源的频次是多少，是否存在隐患；该接口在一般状态下，单次访问内使用各个资源的频次是多少，资源依赖的消耗与功能是否匹配。

细节把控 建议从最外层接口逐层审查。

• 对外服务接口Controller

• 接口功能分割合理性

• 功能拆分。功能能够拆分且访问允许的，尽量不把功能耦合至一个接口，防止某接口返回无用数据
• 功能合并。如果客户端希望降低请求个数，则做出合理评估，将一些生命周期相同的，需要同时访问到的功能尽量合并在一起，不要拆成若干接口，增加使用成本。

• 接口参数

• 功能支持。接口要求的参数，能够完全满足功能需要，没有缺失。
• 扩展支持。如果对未来该接口的变化有一定预估，是否留有预留参数做扩展支持，协商后，先请客户端传递，但是后台暂不处理，将来可避免因此发版。
• 参数验证。所有参数的判空、格式、取值是否经过有效性验证，是否满足后续流程可在验证后安全使用。

• 返回参数

• 功能满足。返回内容满足功能需要，保证接口功能完整。
• 体量合理。该分页的要分页，返回的数据太多，就做个体量相对较小的Vo封装下返回值，给的太多就少给点。
• 满足协定。返回码，所有的return的形态必须满足协定，包含返回码与描述等通用内容。
• 返回码。返回码，使用上，如参数错误、未登录、处理异常等通用内容使用通用返回码，非通用内容，应根据具体需要细化返回码内容，不可都以参数错误与处理异常进行返回。

• 用户验证

• 登录验证。根据功能要求，对必须进行登录的接口验证登录；但对无须登陆的公用接口，酌情验证登录（不验证登录，是为了谁都能访问；而验证登录，这是考虑到接口本身的负载问题，会不会被刷）。
• 权限验证。对一些特别的功能，在通用的登录及权限过滤器过滤基础上，是否做到按需权限细化，即结合具体功能甄别用户。

• 安全保证

• 内容加密。对于参数的解密过程，一般由过滤器统一保证，但在返回值上，需要结合具体需要，单独进行加密，对于涉及到用户个人数据的部分，返回时是否做到按需加密。

• 容错保证

• 异常捕获。每一个接口至少要包含一个兜底的异常捕获，防止将异常直接抛出。但除此之外，按照异常捕获范围最小原则，应对能够甄别异常类型的部分做出单独捕获单独处理。

• 日志内容。除默认有的access日志外，需健全日志内容。

• 参数日志。描述 谁 何时 以何参数 访问该接口，方便基于内容的追溯。
• 流程日志。如果接口包含多段的服务调用（想想是否适合将多段服务封装为外观或门面方法），多段服务之间是否有必要日志来记录进行到的流程。
• 异常日志。对于各个异常捕获点，是否都配置了单独的日志，打印了异常类型，且包含了引起异常的参数内容。
• 日志级别。日志级别选用上，请严格遵守日志级别约定（详见 优秀日志实践准则）。该打info的不要打error，会造成不必要的维护成本；反过来，该打error的也不要打info，出问题感知不到。

• 服务逻辑Service Controller审查内容中关于参数、日志等方面内容较通用，这一部分不再赘述。

• 层次拆分

• 基本层面。必要功能分层，是否自顶向下能满足Facade（外观）、Business（业务）、Common（通用）的层理念。分层有利于事务及服务治理。
• 设计模式。对于该功能，是否使用了设计模式，以及使用的设计模式是否合理。很复杂的功能需要利用设计模式来进行分层，以方便维护；很清晰的功能，也不要硬套设计模式，维护结构是要成本的，这样会带来额外的开发成本，并降低代码的可读性。
• 角色单一。不同层面的Service都有自身的角色，明确每一个角色都要做什么内容，角色和角色之间应做到有耦合，无交叉。

• 逻辑拆分

• 功能单一。在同一个Service的方法集合内，每一个方法的功能相对比较单一，通过方法名即简单注释即可理解公用，但不排除相互嵌套，再包含的逻辑。
• 体量合理。同一个方法体，体量不宜过长，如果某方法体量过长，则尽量进行再拆分，提高代码可维护性。
• 封闭性。在同一个Service的方法集合内，明确方法的适用范围，永远只对其他服务暴露该暴露的内容，对于只使用于当前的方法请进行封闭。要知道，给的越多，使用方越不易用，用的时候不知道选哪个。

• 容错处理

• 熔断。在依赖其他资源时，对资源响应超时，是否做出必要熔断策略（通过组件，以及请求设超时时间等）。

• 事务。在必要的流程上，增加事务。

• 事务必要。只在必要的地方，如需回滚的地方，增加事务，不要什么都加事务，这玩意开销很大。
• 事务最小。对于一个需要事务的流程，要让一个事务作用的范围足够小，事务太长，时间就会长。如果一个事务不够，就拆成两段，再进行组建。

• 异常处理。该捕获的异常就捕获，该抛出的就抛出。不能一味都捕获，如果上层有事务，就打断不了了；如果全抛，对层次职责规划又是不利的，上层使用成本就会变高。

• 实现细节

• 注释健全。关键部分的注释一定要有，逻辑久了谁都记不住，自己也不例外，一定要写明白注释，留出一条后路。注释要有针对，着重复杂算法、复杂逻辑、流程，不要什么注释都写，浪费时间。
• 数据类型。数据类型选用是否合理，比如涉及到精度时，是否用了高精度类型；涉及集合时，是否用恰当了容器；需要考虑到线程安全时，是否考虑到了对象的线程安全问题。
• 循环递归。看好循环及递归条件，看是否有死循环及无穷递归的可能性，尤其是循环内涉及到线程、资源使用的地方，很容易将其他资源吃满。
• 边界。Corner Case，数据边界问题，0除，异常值等。
• 其他。关于命名、格式之类的就不再赘述了，谨遵规范。

• 底层Dao、Mapper

• 表设计。大多数情况下，在开发前，这一部分就已经确认过，CR过程不再确认。

• 语句合理。

• 慎用关联。在大表间慎用关联，在目前Mysql资源（Cetus）提供上，大表关联，再进行分页经常会报错。需要做拆分。
• 合理嵌套。在非DBA开发过程中，通常的服务器开发，在语句嵌套上使用较少，如果出现此类使用，需衡量合理性。
• 善用索引。高频查询语句中，如果能够通过索引对某些字段进行查询优化，尽量配置。

其他

心态

CR过程涉及到的开发人与审查人，要注意自身心态。

开发人

• 不要羞于将自己的代码呈现给别人，这本身很是一个提升过程，代码由粗糙逐渐变得经得起考验是好事。
• 坚持原则与虚心理解。程序员工作越久，就会在代码中包含越多的自己的东西，干技术的，都有些恃才傲物，难免“自负”，但是不要情绪影响了工作。要学着听听别人的理解，想想道理，如果有意义那么就接受，如果建议本身存在一定问题，就多与审查人交流，一定程度上坚持原则。
• 被询问与指点，摆正心态，一个团队的兄弟愿意花时间给自己看代码，还给自己提出意见，是件幸运的事。

审查人

• 摆正态度
  • 不要怯懦于提出问题，大家都是为了工作，这跟伤不伤及面子没关系，你的建议会让开发人的得到更好的提升，不要把CR留于礼节。
  • 不要带着批判眼光或者找茬的态度，去做CR，容易纠结于小问题而忽略真正的隐患。
  • 尊重风格，不要带着自己的编程风格去看待别人的分格，CR的目的在于确认正确性。
• 正视时间。有的时候CR内容比较多，但是不要想着时间的问题。
  • 其一，CR本身对审查人也是不断提升读码能力的过程，要看到受益；
  • 其二，审查人要有责任心，审查后会关系到线上运行的，CR过程慎重对待；
  • 其三，缩短CR时间的方式不是粗糙CR过程，而是提升CR能力。
• 不要厌烦。CR这事值得享受。
  • 有时间多做做CR，多看看不同的实现，总能发现乐趣。
  • CR时不要太正式，莫要太拘束，遇到问题多与开发人交流，轻松环境更容易沟通出结果。
  • CR本就是个教学相长的过程，付出及反馈。
• 不吝称赞。如果开发人的代码让自己眼前一亮，有所感悟，表达赞扬是一种良性的做法，这样既可以鼓励开发人坚持好的地方，也同时表达了自己的感谢，消除开发人与审查人之间的隔阂。
• 想好再说。CR是一个需要审查人自身负起责任的过程，形如对代码行医，对任何异议点，需要斟酌，确定不是误诊。

心得

CR流程，乍一想大家平时都在做，但是却缺乏方法论的总结，很多东西只依赖口耳相传或经验养成，难以形成规范，进而推行下去。CR总是需要时间的，如果每次CR都是劳苦那几位经验丰富的前辈，时间久了，总容易疲惫难以坚持，最好的解决方案，就是团队中的所有人都具备CR能力，都能够成为审查人，共同完成这个工作。