代码审查规范

 

前言

CodeReview是一种文化、一件有趣的事情，不要把做成一种约束和批判的事懂，本规范的目时是为了建立起适合运配团队的Code Review机制，让所有人都参与进来、融入进来，保持正面的积极的态度，让CR变成我们开发过程的一部分，并在实行的过程中逐步演进，最后形成良好的CR文化，学会享受Code Review过程。

CR的重要性

CodeReview是开发过程不可或缺的重要一环，如果将代码发布比作一个工厂的流水线，那么CodeReview就是流水线接近于重点的质检员，他要担负着对产品质量的保障工作，将“缺陷”从众多的“产品”中挑出，反向推动“生产方”改进生产质量。

目的

评估风险点完整性检查。评估当前需求是否需要加上灰度、需要开关、需要预备方案等。宁肯多写一些代码，也不让线上出现一丁点问题。多考虑系统的风险点，提供解决方案，这样代码上线我们才更加胸有成竹。防御式编程。代码中需要各类防御式编程代码，如: 数组为空的时候，如何处理;关键配置项获取失败，怎么处理等。写代码不能仅考虑“正常”情况下的场景，还需要考虑各种异常场景下，如何去处理、恢复、解决这些异常。
墨菲定理:如果事情有变坏的可能，不管这种可能性有多小，它总会发生。安全。安全这块一直都是很容易忽视的细节，但是这块一但出问题，很容易变成灾难，如:订单信息被扒、0元下单、无限领券等。所以涉及到和前端交互的接口，

比如如果 if 后面就一个语，那花括号是可选的，如果代码Review双方，就这个问题产生分歧，那就按已有约定来 (比如Coding style) 。如果没有既有约定，就按照作者的来
不要相互攻击

需要考虑安全这一块。性能。千里之堤，毁于蚁穴。想把大厦造好，根基一定要牢固说到底核心目的还是:

1.减少P级事故，减少线上事故!!!
2. 提高代码质量:代码可读性、可维护性、易扩展性、合理性、简易原则等。solid (单一原则、开闭原则、里氏代换、接口隔离、依赖反转)。

Code Review 不要做什么

1:不要过于严苛

通常来说，快节奏开发的业务代码的质量会低于慢节奏的核心基础库的代码质量，所以在做CodeReview的时候，标准也要考虑业务等情况做适当的调整(并不是不做Code Review) 

2:不要基于个人喜好吵架

比如如果 if 后面就一个语，那花括号是可选的，如果代码Review双方，就这个问题产生分歧，那就按已有约定来 (比如Coding style) 。如果没有既有约定，就按照作者的来

3:不要相互攻击

Code Review的时候，有可能一方会觉得另一方的某些做法超过自己的接受度，那可以直接飞书友好沟通，仍然无法达成一致的，可以拉上小组PM或者TL一起解决，任何时候都不可以人身攻击。

担心冲突、害怕出错

预防冲突

  • 沟通技巧

    尽量疑向、不要太肯定

    ✅如果采用......是否会更合适？

    ❌这里应该......

    ✅是否考虑过......这样的方案？

    ❌......方案肯定更好

    ✅这个地方似乎会影响滚动性能？

    ❌这样写肯定会影响滚动性能

  • 发现问题，尽量提供建议
    ✅......这样会更简洁
    ❌你这代码复杂度太高了
    ✅根据......项目规范，这里应该这样...
    ❌你这代码不符合项目规范

特别注意

不要吝啬称赞
没必要力求完美

没有时间 CR

浇花很有意义, 但是先把火灭了

首先区分紧急与真正紧急：

CR 时间不够

工作量评估要包含 CR 时间

推荐预留 20% 的 CR 时间

权衡：

关注设计大于具体实现；
保证不出线上问题为底线；

管理好交付里程碑

越大的里程碑越容易产生大型CL，会拖慢CR速度
建议数据：400行/小时

团队研发素养

设计模式: 掌握24种设计模式
 设计原则: 掌握SOLID原则（单一职责、开闭原则、里氏替换、接口隔离、依赖反转）
 方法学: 了解Devops，极限编程，Scrum，精益，看板，瀑布，结构化分析，结构化设计
 实践: 实践测试驱动开发，面向对象设计，结构化编程，函数式编程，持续集成，结对编程

Code Review 过程

准备阶段


1.提前通知，预约会议的形式，线上、线下同时参与;
2.参与代码评审人数:至少2人以上，
3.提前发出CR的内容，附带相关的需求PRD、技术设计方案
4.CR工具选择: IDE的代码比对工具、gitlab的MR;

实施阶段

Code Review是个持续进行的过程:
a. 开发人日大于6人日的需求，要进行两次以上的CR;
b.CR的时间点: 开发中、提测前、上线前;
2.每次Code Review需要有记录，作为跟进解决的依据
3.做好时间控制，不超过45min。

事后跟进

1.代码复查完成才能提测和上线

Check List

编码规范
代码注释完整度
业务逻辑
影响范围
实现复杂度
可复用性
可扩展性
代码性能
安全

Review范围

Review到底做什么，哪些事必须做，哪些事不该做? 其实CR主要检查代码中是否存在以下方面问题: 代码的一致性、编码风格、代码的安全问题、代码冗余、是否正确设计以满足需求(约束、性能、功能)，下边逐一列举:

完整性检查

检查prd文档中涉及的功能需求、性能和约束需求是否全部得到了满足，是否存在遗漏的项。

逻辑正确，逻辑合理，避免晦涩难懂的逻辑

一致性检查

检查代码是否和架构文档、详细设计文档保持一致，是否存在违背设计的地方;检查代码风格等是否遵循开发规范

代码一致性，函数名和实现一致，注释和代码一致，命名方式一致，异步写法一致（promise， async await，callback混用），抽象层级一致，不建议混用 import 和 require

正确性检查

检查代码中注释是否正确，变量是否正确定义并使用等

可修改性检查

代码涉及到的常量是否易于修改(如使用配置、定义为类常量、使用专门的常量类等)代码中是否包含了交叉说明或数据字典，以描述程序是如何对变量和常量进行访问的

可预测性检查

代码是否无意中陷入了死循环、无穷递归

复杂性

  • 需要添加“黑客代码（hack）”来保证功能的正常运行。
  • 总是有其他开发者询问代码的某部分是如何工作的。
  • 总是有其他开发者因为误用了你的代码而导致出现bug。
  • 即使是有经验的开发者也无法立即读懂某行代码。
  • 你害怕修改这一部分代码。
  • 管理层认真考虑雇用一个以上的开发人员来处理一个类或文件。
  • 很难搞清楚应该如何增加新功能。
  • 如何在这部分代码中实现某些东西常常会引起开发者之间的争论。
  • 人们常常对这部分代码做完全没有必要的修改，这通常在代码评审时，或者在变更被合并进入主干分支后才被发现。

健壮性检查

代码是否采取措施避免运行时错误(如数组边界溢出、被零除、值越界、堆栈溢出等)

结构性检查

程序的每个功能是否都作为一个可辩识的代码块存在循环是否只有一个入口

可追溯性检查

是否有一个交叉引用的框架可以用来在代码和开发文档之间相互对应代码是否包括一个修订历史记录，记录中对代码的修改和原因都有记录

可理解性检查

注释是否足够清晰的描述每个子程序，代码是否易于理解是否使用到不明确或不必要的复杂代码，它们是否被清楚的注释

性能方面检查项

性能检查在大多数代码中都是需要严重关注的方面，也是最容易出现问题的方面，常常有些功能没有丝毫问题的代码后，正式上线运行时却在性能上表现不佳，从而不得不做大量的返工，甚至是推倒重来。下面是一些常见的检查点:

  • 数据库索引检查
  • 在大量数据出现时,队列,表,文件,在传输,upload等方面是否会出现问题,有无控制,如分配的内存块大小,队列长度等控制参数
  • 对hashmap、list等集合类数据结构的选择和设置是否合适，如正确设置capacity,load factor等参数，数据结构的是否是同步的
  • 有无滥用String对象的现象
  • 是否采用通用的线程池、对象池模块等cache技术以提高性能
  • 类的接口是否定义良好，如参数类型等，避免内部转换
  • 否采用内存或硬盘缓冲机制以提高效率
  • 并发访问时的应对策略
  • I/0方面是否使用了合适的类或采用良好的方法以提高性能(如减少序列化，使用buffer类封装流等)
  • 同步方法的使用是否得当,是否存在粒度过大等过度使用
  • 递归方法中的迭代次数是否合适，应该保证在合理的栈空间范围内

资源泄漏处理方面检查项

 

线程安全方面检查项

 

没事别乱写注释

good case：
为什么接下来的代码要这么做
bad case：
接下来的代码做了什么

安全性

代码中应注意，不要存储敏感内容

数据库处理方面检查项

CR人员在面对代码中涉及到的数据库可移植性和提高数据库性能方面的冲突时，需要做出抉择，强烈建议必须有稳定性人员or DBA参与。

  • 数据库设计或SQL语句是否便于移植(注意和性能方面会存在冲突)，是否存在大量过于复杂或利用数据库特性的SQL语句
  • SQL语句需要DBA审核通过

异常外理检查项

JAVA中提供了方便的异常处理机制，但普遍存在的是异常被捕获，但并没有得到处理。日常代码中最常见的现象是进入某个方法后，一个大的try/catch将所有代码行括住，然后在catch中将异常打印到控制台，而且该异常是Exception对象。

  • 每次当方法返回时是否正确处理了异常，如最简单的处理，记录日志到日志文件中
  • 在出错路径上是否所有的资源和内存都已经释放
  • 所有抛出的异常都得到正确的处理，特别是对子方法抛出的异常，在整个调用栈中必须能够被捕捉并处理

方法方面检查项

  • 方法的参数是否都做了校验
  • 数组类结构是否做了边界校验
  • 变量在使用前是否做了初始化
  • 返回堆对象的reference，不要返回栈对象的reference方法 eg：https://blog.csdn.net/weixin_34402408/article/details/93453710
  • API是否被良好定义，即是否尽量面向接口编程，便于维护和重构

命名（世上问题千千万，问题命名占一半）

  • 不用宽泛的模块或文件名

  • 没有拼写错误，单复数也应该正确

  • 符合规范：

    • 文件名kebab-case
    • 类名PascalCase
    • 文件作用域内 常量、变量、函数 camelCase
    • private 是否采用下划线，应保持一致

 

Bad Case

缺乏必要参数

/**
 *订单关的Api接口提供给前端使用
*/
public class OrderApi [
 
/**
*获取订单信息
*@param orderNumber 订单编号
*@return 订单详情
*/
public Order getOrderInfo(String orderNumber) [
    return ...;
 }
 
 
}

问题点: 参数没有加上用户id，这样会导致数据被扒，别人直接调用接口，按照顺序生成orderNumber，会将全部数据库中的订单信息报漏出去。

必要性的校验

样例1:返回值

public BigDecimal getExchangeRate(String sourceCurrency, String targetCurre){
       final BigDecimal rate = currencyRpc.getExchangeRate(sourceCurrency， targetCurre)
       return rate == null ? BigDecimal.Zero : rate;
}

问题点:此处拉取汇率失败，应该报错，抛异常出去，而不是返回一个“有误导性的默认值”。因为方法没有报错，上游会假定这个汇率是有效的，因而将0作为标准汇率进行后续计算。

样例2:NPE

public Long getLastBiddingPrice(long uid){
       Bidding data = biddingDao.queryLastBidding(uid);
       return data.getPrice();
}

问题点: 此处应该加上data==null的判断，如果db没有此数据，data.getPrice()会抛出NPE.

未处理边界

 

吞噬异常

样例1

public Long getMinPrice(long skuId){
 try {
      return inventoryRpc.queryMinPrice(skuId);
    } catch (Exception e) {
      return 0L;
     }
}

问题点:

  • 异常信息被莫名吞噬，异常中携带重要的错误信息，以便我们排查和跟踪调用失败的原因，将异常吞噬，然后假装它不存在，不仅会让代码更不健壮，也会让我们更加束手无策。
  • 异常被吞噬，我们无法知道到底出了什么错误，无法排查出错的原因。这个返回值0，表达的意义不明确，当返回值为0的时候，不知道是正常调用，然后返回0;还是错误调用，无法区分。

样例2

public Long getMinPrice(long skuId) {
try {
    return inventoryRpc.queryMinPrice(skuId);
    } catch (Exception e) {
    throw new RuntimeException("获取最低价格失败");
}

问题点: 将原本的异常转换为一个其他的异常，会导致原始异常信息丢失，仅做到出错了，还是无法知道到底是什么原因导致的错误，而且这种写法，会导致异常的调用堆栈丢失。

有风险代码

样例1

获取身份证号码，吐给前端，然后让前端加掩码。

样例2

用户评论等可以手动输入的文本，原封不动的存入数据库，不进行任何html等转义。问题点: 这么做，很容易出现xss和csrf，导致网站被攻击

样例3

太信任前端传入的参数，如:前端计算出来的订单价格等直接使用，而不在服务器重新计算问题点: 1元下单，就是这么来的

样例4

某些页面的权限用是否显示按钮控制，而且不在服务器做double-check是否有权限问题点: 很明显，用户按下F12，修改下源码，就可以越权操作了

样例5

这边开发了一个新需求，需要同步1W+的用户信息到营销，每个用户的改动点，涉及用户、订单、金额、支付等各个方面
问题点: 此处建议采用灰度的方式上线，修改面广，涉及东西多，如果一口气开放1W+的商品，如果有隐藏的问题没有发现，很容易出现大面积数据问题

样例6

有个接口的qps很高，达到了1k+以上，调查后发现，里面有个基于乐观锁的重试机制，重试间隔为1s,最多3次，也就是说此接口最多时间为3s+
问题点:服务很容易发生雪崩。高并发接口，不建议使用乐观锁，因为失败概率太高。一条线程挂起3s+，在高qps情况下，系统线程很快耗尽，导致服务不可用。

资金场景

样例1

public float getPrice(float price, float rate) {
   return price * rate;
}

问题点: 4.015*100的结果是多少呢?401.5，不，答案是: 401.49999999999994。java中的浮点数存储的是一个近似值，会和真实的值有一定的偏差。可以考虑使用long(保留数值后几位为小数位置，如:将1元存储为10000000)，或者使用BigDecimal类型。

无法精确存储的原因: 大部分的十进制的小数无法用二进制方式精确存储，如0.7 表示二进制等于0.10110011001100110011001100...从第三位开始1100开始循环了

样例2

@Transactional
public void cancelOrder(String orderNum){
   orderDao.cancelOrder(orderNum);
   // 退款
   String messageId = rocketMessage.sendToRefund();
   logDao. insert(messageId,orderNum);
   .. 一些其他业务逻辑
}

问题点:事务没有提交，但是退款的消息先发出去了，有可能导致，钱退掉了，但是事务回滚了，订单还在。

优先使用异常而不是错误码

public Result<Long> getProductPrice(long spuId) {
       ProductInfo info = commodityService.get(spuId);
       if (info == null) {
          return Result.fail(100，"商品不存在");
        }
      return info.getPrice();
}

问题点: 此处应该返回Long，而不是Result这种携带错误码的返回值。而且return Result.fail(100"商品不存在”);这行代码应该抛出异常，而不是返回一个错误对象。

  • 使用异常能中断当前调用
  • 使用异常能回滚当前事务(配合spring的注解)
  • 上游不需要频繁的check返回值Result的code是否正常(调用链路没有出异常就是OK的)。
  • 使用异常，当前异常所在的调用堆栈会被记录下来，方便后续排查问题

 

posted @ 2025-03-03 15:20  无味之水  阅读(73)  评论(0)    收藏  举报