前言，一些感想

有些功能模块比较大，在开发过程中如果功能拆解的不够细致独立，就会导致写出很多相互耦合的代码，进而导致CodeReview的难度提升。试想一下，一次review一个功能和一次review5个功能，哪个更容易理解？一次review 100行代码和一次review 1000行代码，哪个更容易理解？一次 review 1个文件和一次review 20个文件，哪个更容易理解？

按照现在的MVC分层结构模型设计，我们写的功能为了实现高内聚低耦合一定是把相关的功能封装到一个个单独的service层里头，然后service在调用其他的service以实现我们的功能，至少我们有一个数据库表就会有一个对应的mapper，继而有一个对应的service，然后是controller。大部分情况下，我们的一个功能往往涉及1到2个表的操作以及1到2个的RPC操作，我们在开发中最好是一个功能点一个功能点的开发。如果我们非要多个功能点并行开发，而且还写到一个分支上，一会儿写写这个功能点，一会儿写写那个功能定，最后就会导致我们代码拆也不好拆，测也不好测, review 也不好review。最主要的是因为代码review的时候功能点跳来跳去，看了后面忘了前面，功能也比较跳脱，无法在大脑中形成一个连贯的线索。

代码review和单元测试一样，所有的对提高代码质量的措施和制度都是阻塞项目进度的，因为你势必要拿出一定的时间和精力来处理，甚至要占用团队其他同学的时间，review过程中的问题在修复后还需要测试，也是需要耗时间的。但在美团、阿里的经验告诉我，在合适的时候启动代码review是有好处和意义且必要的，不过我们又必须权衡代码review的度，卡的紧了，项目完成不了，卡的松了，其实是在给后人挖坑，安全事故频发。

什么时候不适合启动代码review

有些时候我觉得不适合启动代码review机制。我们团队刚开始人少，只有两个人，但工作经验还算相当(我挺佩服我的搭档的,工作3年就有5年的经验)，双方代码风格相似，而且思想一致，加之项目紧，上线时间卡的死死的，那个时候加班都可能开发不完，再有项目又是刚刚启动，代码从零码起，业务功能不算特别复杂，大多是对几个表的增删查改，此时再花时间进行代码review和单元测试就更上不了线了。基于此种种，我觉得优先保功能上线，代码review可以不做。

什么时候适合启动代码review

随这我们团队人数的增加，我们的

代码风格开始出现差异。比如
1. 有的功能写到service层，有的写到rpc层，有的在controller层写业务逻辑
2. 有的写interface然后写service，有不写interface直接写一个servcie
3. 命名风格开始出现差异，有的定义PO、VO、DTO，有的人定义类不写这些后缀
项目功能开始逐渐出现需要优化的点。比如
1. 原来的表需要加个字段了
2. 原来的功能需要加通知了
3. 原本的逻辑需要推倒重来
安全意识参差不齐。比如
1. 不考虑越权问题了，这个主要跟工作经验有关
2. 不考虑性能问题了，for循环嵌套for循环
3. 不考虑线程安全了，想要用多线程，奈何经验不够，写出了及其晦涩难维护的并发代码
开始初步形成一些制度上的约束了。比如
1. 开始规范代码书写的格式了
2. 发布上线开始卡控业务低峰期了

我断言，后续还会

制度上的约束会加强，甚至会出台一些规章给大家学习
随着领导的要求，团队数据安全意识越来越强
非业务性的各种校验逻辑、数据核对逻辑、后台数据订正逻辑开始出现
服务会接入更多的中间件，比如 redis、分布式锁、分布式事务、ES等
需要开始考虑服务的熔断、降级
部分功能重构开始
业务开始复杂，新旧功能开始交叠了
服务性能出现瓶颈，读写分离、分库分表开始出现
bug数量增多，非紧急bug开始出现积压
…

所有这些问题都会自然而然的推动这个团队开始进行代码review了。

CodeReview的作用

我个人认为代码review的好处

意识上的审慎：有代码review机制，我们在书写代码的时候就会站在一个第三方视角审视我们的代码，时刻保持着审慎的思维在书写。
改善代码质量：有代码review机制，代码reviewer们会指出我们代码中的缺陷，并给出建议，从一定程度上帮助作者改善代码质量。
形成统一共识：有代码review机制，代码reviewer们提出的一些建议被整个团队接纳后会形成一种规约，从而逐渐形成统的一共识。
灌输安全意识：有代码review机制，代码reviewer们会指出代码中的安全问题、性能问题。诸如SQL注入、方案安全漏洞、性能问题等。
增进业务学习：有代码review机制，代码reviewer们也是在学习作者实现功能的业务的过程。有助于全员了解整体业务流程，在一定程度上避免仅仅只有某个人熟悉某个业务的情况。

发起评审前

CR说到底是一件协同的事情，那么一定要有人遵守这项基本的游戏规则。不要听不进去别人的建议，欣然接受其他人给出的合理建议，因为这本就是一个很好的学习机会。

【强制】保证代码的逻辑正确，是设计者的责任。

我在此首先申明一点：保证代码的逻辑正确，是设计者的责任，不要指望别人帮你找出问题。代码中出现的bug，肯定是设计者制造的，不要一股脑的怪到reviewer身上，reviewer只是尽可能的帮你检查，并不能帮你去实现。千万不要在出现事故之后说：你当时review的时候怎么不说？你当时review的时候怎么没发现？

我个人认为在小团队里面不适合也不推荐搞代码review连带追责这种制度来强迫reviewer高度认真的review提交者的代码，这会进一步降低团队的合作效率，更加阻塞项目进度，有可能还会打击人们review代码的积极性。因为review不出问题还要问责与我，那我多一事不如少一事，直接就不参与了。

【强制】git分支名字不能以开发者的名字、无意义的名称、时间等直接命名，需要尽可能的体现开发功能的意图

正例：feature/case-recovery-time、hotfix/fix-time-nullpoint
反例：feature/zhangsan、feature/123、feature/bugfix

你的本地开发分支爱怎么叫怎么叫，但不规范的命名千万不要推到远程给别人看到。
如果在创建分支的时候你是根据功能起的名字，那么你在开发的过程中就会倾向专注于某一个功能。例如你命名为 feature/case-recovery-time 的分支你大概率就会只开发和案件回收时间有关的功能，此时如果来了另一不相干的需求：在案件回收之后发送一个mq消息。此时你就不会倾向于把这个功能加到名为feature/case-recovery-time的分支，而是另起一个分支比如叫feature/case-dispatch-mq。但是如果你命名成为 feature/case 或者 feature/zhangsan 如果经验不足或者不是时刻惦记着，你很有可能就把本来可以独立开发、独立上线的两个功能耦合到了一个分支了。耦合到一起之后就出现我在文章开头 《前言，一些感想》 中说的的代码review的种种弊端了。

【强制】进行充分自测，保证代码可运行。代码必须合并到测试分支并发布一遍测试环境后才可以提起CR

如果代码往测试分支合并过程中与其他人有代码冲突，则会提前暴露出来，后续往master分支合并的时候需要留意代码冲突。
reviewer直接去pull测试分支的代码即可在本地idea中看到你的代码，方便在idea中查阅。
证明你的代码至少编译没有问题、单测没有问题。如果有问题，流水线一定通过不了，发布是不会成功的。如果连本地编译都没有过就找人review是极其不负责任的表现。

【强制】代码不能太多:每次审查的代码行数不超过 400 行

SmartBear对思科系统编程团队的研究表明，开发人员一次应该审查不超过200到400行的代码（LOC）。大脑一次只能有效地处理这么多信息；超过400LOC，发现缺陷的能力就会减弱。一次推给别人 1000+ 行代码，还想得到有价值建议的可能性微乎其微。在实际操作中，60到90分钟内对200-400LOC的审查应该能够发现70-90%的缺陷。因此，如果代码中存在10个缺陷，正确进行的审查将发现其中7到9个。

初次制定代码review可以先从1000行开始，然后逐渐缩小到800行，然后缩小到500行，最后到400。超过这个阈值说明一个功能中涉及的功能点太多了，需要拆分后再提CR。

【推荐】速度不能太快: review 速度应该低于每小时500行代码（LOC）

在合理数量下，以较慢的速度，在有限的时间内进行代码审查，会获得最有效的代码审查结果。

【推荐】时间不能太久:一次 Code Review 不要超过60分钟，尽量控制在30-60分钟

正如你不应该过快地审查代码一样，你也不应该一次性审查过长时间。当人们从事任何需要集中一段时间精力的活动时，在大约60分钟后性能开始下降。我个人觉得看代码超过半小时，注意力基本已经开始涣散，就好比一节课40分钟，最后10分钟基本都在想着下课后怎么玩了。研究表明，在一段时间内从任务中休息可以大大提高工作质量。进行更频繁的评审应该减少需要进行如此长时间评审的需求。设计者应该自己估好时间，如果代码太复杂就拆分后再提CR。

【强制】只要是合并过程中与别人的代码有冲突，如果自己不是特别笃定，解决冲突必须双方在场。

冲突是因为针对同一个文件中的同一行代码双方进行了修改，一个要把他变成这样，一个要把他变成那样，最后 git 也不知道该听谁的了，所以就报冲突了。发生冲突了，如果不涉及业务逻还好，只要涉及业务逻辑，就必须双发在场商讨该如何取舍。

【强制】利用自动化工具进行前置检查

推荐以下几种工具或插件

Alibaba Java Coding Guidelines(XenoAmess TPM): 专注于Java代码检查，如果你的团队期望遵循阿里规约那么这个插件必装
CheckStyle-IDEA : 侧重检查编码格式和代码风格规范，如命名规范、Javadoc注释规范、空格规范、size度量（如过长的方法）、重复代码、多余Imports等，Checkstyle主要是文法层面的代码编写规范的分析，对bug几乎没什么发现能力。

拆分CR的原则和技巧

上面说了一次CR的代码不宜太多，但是如果真的就是写了很多，那么只能拆分后分批review，而且要保证每一批都应该是一个独立完整可以运行的功能。

【推荐】与业务不相关的代码可以优先单独提交一次review

正例：仅仅更改类名、把一个类挪动到另一个包、替换一个工具类等等。这种即使改动行数特别多也不用占用特别多的时间，因为这种改动业务逻辑并没有改变或者改变很少，因此风险最小。

【推荐】底层的、独立的功能可以优先单独提交一次review

正例：添加个枚举、添加一个工具类里面全是静态方法、约定一个接口(没有实现)等。这种虽然涉及业务逻辑，但是对历史业务基本没有改变或者改变很少，因此风险较小。

【推荐】相关业务的不同业务逻辑分别单独提交一次review

正例：一个大功能中需要实现先查询数据并校验，然后执行某项业务逻辑最后更新数据库，然后发送mq消息通知并消费mq执行某个业务逻辑。测试可以把查询数据并校验和更新数据库做成一个分支，然后把发送mq消息通知并消费mq执行某个业务逻辑做成一个分支。

【强制】不相关的业务的逻辑分别单独提交review

正例：更新案件还款信息和案件调增调减bugfix，本身就是两个不相关的业务逻辑，那么就一定要单独分支开发。耦合在一起，一个有问题，另一个也没有办法发布。

拆分业务逻辑仁者见仁智者见智，有些业务逻辑你硬说他是相关的或者不相关的纯属于抬杠，没有意义。一个简单的评判标准是：这些功能可不可以单独上线？不可以！那基本就是相关的功能，无法解耦；可以！那基本就是不相关的功能，那理论上就支持拆分。如果严格评判，请参考领域驱动设计DDD中的领域服务的拆分标准，后面展开文章再研究。

对评审者的要求

【强制】重点review的是设计方案和业务逻辑，其次是数据安全，最后才是代码样式。

【强制】帮别人review代码切忌三心二意。

反例：一边处理手头上的问题一边reivew代码，与其这样还不如等我们处理完手头上的事情再开始进行代码reivew。如果是代码review过程中发生的问题，问题不紧急我们可以先review完代码在处理，问题紧急我们可以向提出review的同学解释先处理完紧急问题后再继续reivew。我们要认真对待别人的代码，这样别人才会善待我们的代码。

【强制】如果变更达到可以提升系统整体代码质量的程度，就可以让它们通过，即使它们可能还不完美。这是所有代码评审准则的最高原则。

世界上没有“完美”的代码，评审者不应该要求代码提交者在每个细节都写得很完美。评审者应该做好修改时间与修改重要性之间的权衡。

【强制】在代码样式上，请遵从团队约定的代码样式指南，所有代码都应与其保持一致，任何与代码样式指南不一致的观点都是个人偏好。

千万不要在自己违反了团队已有的规范的情况下对reviewer说:我的个人习惯就是这样，我觉得这样写比较顺眼。

【强制】如果没有可用的规范，那么reviwer应该让作者与当前代码库保持一致，至少不会恶化代码系统的质量。（一旦恶化代码质量，就会带来破窗效应，导致系统的代码质量逐渐下降）

【推荐】如果没有可用的规范，可以对问题进行研讨后制定并补充到规范当中，需要参考网络或者ChatGPT的建议并遵循少数服从多数的原则进行制定。

【推荐】但如果某项代码样式在指南中未提及也未约定，那就接受作者的样式。

【推荐】规范不是不可以变，不合理的规约团队经过实践后重新约定可以进行修改升级，但已经写完的代码同样需要依照新的约定进行重新修正。

【推荐】Java规范参开阿里系的规约，其内容大部分都是合理的，小部分可以打破并形成自己团队的约束。

例如：对于一些有争议的方面，例如接口命名是否采用大写“I”开头，集合类型的变量是否采用复数。团队采用一个统一的标准就好，不需太过纠结。

【强制】任何涉及软件设计的问题，都应取决于基本设计原则，而不应由个人喜好来决定。当同时有多种可行方案时，如果作者能证明（以数据或公认的软件工程原理为依据）这些方案基本差不多，那就接受作者的选项；否则，应以标准的软件设计原则为准。

通常有不止一种方法来完成事情。在阅读一段代码时，您可能会想到其他方法，并且会倾向于在评论中提出建议，因为它是更好的解决方案。有时您需要问自己：这真的是更好的解决方案吗？还是作者的方式也不错？除非代码复杂性或可读性有明显优势，否则在建议重写之前请三思而后行。如果作者的方式没问题，但您仍然认为自己的方式更好，请将其写为建议而不是强制，并愿意与作者讨论您建议更改的原因和细节。

【推荐】如果reviewer的建议实现起来并不麻烦而且不会让你的程序变得更差反而可以提高代码的质量，那么就接受reviewer的建议。

正例: for 循环中发起了rpc请求。 reviewer建议：改造成批量查询。这个建议通常是合理的。
正例: for 循环嵌套，内层for循环先转换成map，时间复杂度由o(n^2)->o(n)。这个建议通常是合理的。

【推荐】提问式review，通过听取作者对问题的回答可以判断作者是否考虑问题全面。如果作者能果断的回答，说明其思考过。如果作者回答含糊其辞，需要一步步引导才能输出答案，请重点review下这段代码。

正例：

请问这个地方如果数据量变大了会怎样（我们在写代码的时候永远要考虑数据量可能变大的场景）？这个地方会不会抛出异常？是不是该加事务？这个地方的 try catch 是不是多余？
这个地方有没有可能为空？为什么不能为空呢？既然你笃定不为空，为什么还要判空呢？

【强制】礼貌用语，避免夸大，不要人身攻击

在评审过程中，要特别注意不能把代码问题升级成为人的问题，组织者要特别注意引导和及时地提醒。不是每个人的经历都跟你一样：有些东西对你来说是常识，但有些人可能并不知道，即使他们的能力并不差。你可以说这些东西是常识，但不要显露出嘲笑或高人一等的姿态。

大项目要求编码前设计评审，小需求可以事先Review设计思路，避免最后的惊喜

review的一些基本原则

【推荐】尽快完成评审
【推荐】避免过度追求完美，浇花很有意义，但是先把火灭了
【强制】明确评论是否要解决
【推荐】基于技术事实和数据的沟通
【推荐】否决个人偏好和意见
【推荐】不要吝啬称赞

评审什么

Commit Message

Commit Message在CR时的作用也很重要，一个好的Git Commit Message应该能够清晰点明主题，是bug还是feature还是doc的完善，应该能让Reviewers一目了然。目前团队还没有对此进行约束，暂不展开描述，后续再补充。

Self Check

让别人评审前先把自己当作 Reviewer 来对自己的代码进行 CR，问一下自己下面的问题。reviewer 在 review 过程中也需要有意识的注意下面的问题。当然重点是review业务逻辑！！

设计：代码是否设计良好？这种设计是否适合当前系统？
功能：代码实现的行为与作者的期望是否相符？代码实现的交互界面是否对用户友好？
复杂性：代码可以更简单吗？如果将来有其他开发者使用这段代码，他能很快理解吗？
测试：这段代码是否有正确的、设计良好的自动化测试？
命名：在为变量、类名、方法等命名时，开发者使用的名称是否清晰易懂？
注释：所有的注释是否都一目了然？
代码样式：所有的代码是否都遵循代码样式？
文档：开发者是否同时更新了相关文档？

代码样式大体是否符合团队间约定的代码规范？有瑕疵没有关系，保证大约90%符合约定就行，不追求完美。
方法或接口的声明、表达式使用是否恰当？
是否有重复代码可以抽取出来？
注释是否清晰明了？
异常处理、日志记录是否全面？
是否有判空和异常传参校验?
逻辑边界是否完整？
是否存在并发调用线程安全问题？
是否需要支持幂等？
是否需要加事务？
是否存在内存泄露风险？
是否存在数据一致性问题？
是否需要增加限流、熔断、降级等保护机制？
是否需要兼容旧逻辑、旧版本？
是否存在循环调用（接口、数据库），能否改成批量处理？
调用外部接口是否设置合理的超时时间？
对外开放的接口，是否预估调用量？是否有保护机制（限流、熔断、降级）？
打印日志是否过多？
单元测试检查是否执行？
新增单元测试检查是否覆盖？
圈复杂度过高？
…

并发场景

异常是否应该捕获？
异常堆栈信息是否打印？
线程安全?
事务是否能生效

数据安全和数据一致性

是否需要报警提示
如死锁、死循环、FullGC、慢SQL、缓存数据热点等
是否需要加事务
是否越权
是否需要脱敏展示

避免过渡设计

测试用例：单元测试用例的验证逻辑是否有效，测试用例的代码行覆盖率和分支覆盖率；

代码质量
编码规范：命名、注释、领域术语、架构分层、日志打印、代码样式等是否符合规范
可读性：是否逻辑清晰、易理解，避免使用奇淫巧技，避免过度拆分
简洁性：是否有重复可简化的复杂逻辑，代码复杂度是否过高，符合KISS和DRY原则
可维护性：在可读性和简洁性基础上，是否分层清晰、模块化合理、高内聚低耦合、遵从基本设计原则
可扩展性：是否仅仅是满足一次性需求的代码，是否有必要的前瞻性扩展设计
可测试性：代码是否方便写单元测试及分支覆盖，是否便于自动化测试

代码性能

是否可以批量操作？
是否存在for循环嵌套？
事务的范围是否精准？是否存在大事务？
是否可以并发执行？
是否需要增加本地缓存、分布式缓存、多线程、消息队列？
缓存是否存在大key问题？
是否需要分页查询？没有分页的列表查询对DB性能影响非常大，特别是在项目初期，因为数据量非常小问题不明显，而导致没有及时发现，会给未来留坑。

性能和可读性之间的权衡

个人觉得可读性在90%的场景下要优先于性能进行考量。好的代码，应该让大家一眼能看出来，即必须有良好可读性，可读性决定了可维护性。下列是一个简单代码的例子：

x = x ^ y;
y = x ^ y;
x = x ^ y;

和

temp =  x;
x = y;
y = temp;

上面的代码都是做值交换操作，前者高效，后者明了。但前者加上一行注释是不是会更好呢：

// 整型值互换
x = x ^ y;
y = x ^ y;
x = x ^ y;

不该评审什么或者什么情况不用评审

理论上都应该评审

书籍推荐

《编程原则( understanding software)》
《重构：改善既有代码的设计》
《编写可读代码的艺术》
《代码大全》
《敏捷软件开发》
《架构整洁之道》
《代码整洁之道》

关于 DRY

DRY是Don’t Repeat Yourself的缩写，DRY是Andy Hunt 和 Dave Thomas’s 在《 The Pragmatic Programmer 》一书中提出的核心原则。DRY 原则描述的重复是知识和意图的重复，包含代码重复、文档重复、数据重复、表征重复，我们这里重点讲讲代码重复。