Code Review Guide

代码审查者应该关注哪些方面?

代码审查时应该关注以下方面:

  • 设计:设计是否合理?
  • 功能:是否满足prd需求?是否满足用户流程交互合理性?
  • 复杂度:代码能更简单吗?将来其他开发人员能轻松理解并使用此代码吗?
  • 测试:代码是否具有正确且设计良好的单元测试?
  • 命名:开发人员是否为变量、类、方法、包等选择了明确的名称?
  • 注释:注释是否清晰有用?
  • 风格:代码是否遵守了代码规范
  • 文档:开发人员是否同时更新了相关文档?

设计

审查中最重要的是 CL 的整体设计。CL 中各种代码的交互是否有意义?现在是添加此功能的好时机吗?

单一职责原则 (Single Responsibility Principle)

  • 类中的代码行数、函数或属性过多,会影响代码的可读性和可维护性,我们就需要考虑对类进行拆分;
  • 类依赖的其他类过多,或者依赖类的其他类过多,不符合高内聚、低耦合的设计思想,我们就需要考虑对类进行拆分;
  • 私有方法过多,我们就要考虑能否将私有方法独立到新的类中,设置为 public 方法,供更多的类使用,从而提高代码的复用性;
  • 比较难给类起一个合适名字,很难用一个业务名词概括,或者只能用一些笼统的 Manager、Context 之类的词语来命名,这就说明类的职责定义得可能不够清晰;
  • 类中大量的方法都是集中操作类中的某几个属性,那就可以考虑将这几个属性和对应的方法拆分出来。
违背SRP原则的案例

1.CreditSchemaAbilityImpl中函数、依赖过多,充斥着类转换逻辑、访问facade逻辑、工厂逻辑、私有方法过多、大量私有方法最终只是得到一个字段值等。

2.User类中大量方法操作的都是UserAddress成员变量对象,而这些方法散落在User类而不是委托UserAddress类。

开闭原则 (Open-Closed Principle)

  • 添加一个新的功能,应该是通过在已有代码基础上扩展代码(新增模块、类、方法、属性等),而非修改已有代码(修改模块、类、方法、属性等)的方式来完成。关于定义,我们有两点要注意。第一点是,开闭原则并不是说完全杜绝修改,而是以最小的修改代码的代价来完成新功能的开发。第二点是,同样的代码改动,在粗代码粒度下,可能被认定为“修改”;在细代码粒度下,可能又被认定为“扩展”。

  • 我们要时刻具备扩展意识、抽象意识、封装意识。在写代码的时候,我们要多花点时间思考一下,这段代码未来可能有哪些需求变更,如何设计代码结构,事先留好扩展点,以便在未来需求变更的时候,在不改动代码整体结构、做到最小代码改动的情况下,将新的代码灵活地插入到扩展点上。

最常用来提高代码扩展性的方法有:多态、依赖注入、基于接口而非实现编程,以及大部分的设计模式(比如,装饰、策略、模板、职责链、状态)。

违背OCP原则的案例

public class Alert {
  private AlertRule rule;
  private Notification notification;

  public Alert(AlertRule rule, Notification notification) {
    this.rule = rule;
    this.notification = notification;
  }

  public void check(String api, long requestCount, long errorCount, long durationOfSeconds) {
    long tps = requestCount / durationOfSeconds;
    if (tps > rule.getMatchedRule(api).getMaxTps()) {
      notification.notify(NotificationEmergencyLevel.URGENCY, "...");
    }
    if (errorCount > rule.getMatchedRule(api).getMaxErrorCount()) {
      notification.notify(NotificationEmergencyLevel.SEVERE, "...");
    }
  }
}

上面这段代码非常简单,业务逻辑主要集中在 check() 函数中。当接口的 TPS 超过某个预先设置的最大值时,以及当接口请求出错数大于某个最大允许值时,就会触发告警,通知接口的相关负责人或者团队。现在,如果我们需要添加一个功能,当每秒钟接口超时请求个数,超过某个预先设置的最大阈值时,我们也要触发告警发送通知。这个时候,我们该如何改动代码呢?主要的改动有两处:第一处是修改 check() 函数的入参,添加一个新的统计数据 timeoutCount,表示超时接口请求数;第二处是在 check() 函数中添加新的告警逻辑。具体的代码改动如下所示:


public class Alert {
  // ...省略AlertRule/Notification属性和构造函数...
  
  // 改动一:添加参数timeoutCount
  public void check(String api, long requestCount, long errorCount, long timeoutCount, long durationOfSeconds) {
    long tps = requestCount / durationOfSeconds;
    if (tps > rule.getMatchedRule(api).getMaxTps()) {
      notification.notify(NotificationEmergencyLevel.URGENCY, "...");
    }
    if (errorCount > rule.getMatchedRule(api).getMaxErrorCount()) {
      notification.notify(NotificationEmergencyLevel.SEVERE, "...");
    }
    // 改动二:添加接口超时处理逻辑
    long timeoutTps = timeoutCount / durationOfSeconds;
    if (timeoutTps > rule.getMatchedRule(api).getMaxTimeoutTps()) {
      notification.notify(NotificationEmergencyLevel.URGENCY, "...");
    }
  }
}

这样的代码修改实际上存在挺多问题的。一方面,我们对接口进行了修改,这就意味着调用这个接口的代码都要做相应的修改。另一方面,修改了 check() 函数,相应的单元测试都需要修改(关于单元测试的内容我们在重构那部分会详细介绍)。

里氏替换原则 (Liskov Substitution Principle)

  • 子类对象(object of subtype/derived class)能够替换程序(program)中父类对象(object of base/parent class)出现的任何地方,并且保证原来程序的逻辑行为(behavior)不变及正确性不被破坏。

  • 判断子类的设计实现是否违背里式替换原则,一个小窍门,那就是拿父类的单元测试去验证子类的代码。如果某些单元测试运行失败,就有可能说明,子类的设计实现没有完全地遵守父类的约定,子类有可能违背了里式替换原则。

违背LSP原则的案例
  1. 子类违背父类声明要实现的功能父类中提供的 sortOrdersByAmount() 订单排序函数,是按照金额从小到大来给订单排序的,而子类重写这个 sortOrdersByAmount() 订单排序函数之后,是按照创建日期来给订单排序的。那子类的设计就违背里式替换原则。

  2. 子类违背父类对输入、输出、异常的约定在父类中,某个函数约定:运行出错的时候返回 null;获取数据为空的时候返回空集合(empty collection)。而子类重载函数之后,实现变了,运行出错返回异常(exception),获取不到数据返回 null。那子类的设计就违背里式替换原则。在父类中,某个函数约定,输入数据可以是任意整数,但子类实现的时候,只允许输入数据是正整数,负数就抛出,也就是说,子类对输入的数据的校验比父类更加严格,那子类的设计就违背了里式替换原则。在父类中,某个函数约定,只会抛出 ArgumentNullException 异常,那子类的设计实现中只允许抛出 ArgumentNullException 异常,任何其他异常的抛出,都会导致子类违背里式替换原则。

  3. 子类违背父类注释中所罗列的任何特殊说明父类中定义的 withdraw() 提现函数的注释是这么写的:“用户的提现金额不得超过账户余额……”,而子类重写 withdraw() 函数之后,针对 VIP 账号实现了透支提现的功能,也就是提现金额可以大于账户余额,那这个子类的设计也是不符合里式替换原则的。

依赖倒转原则 (Dependence Inversion Principle)

  • 高层模块(high-level modules)不要依赖低层模块(low-level)。高层模块和低层模块应该通过抽象(abstractions)来互相依赖。除此之外,抽象(abstractions)不要依赖具体实现细节(details),具体实现细节(details)依赖抽象(abstractions)。
违背DIP原则的案例

1.在sop流程中提供通用功能,拓展点却依赖了具体实现环节中的类,关心了具体实现。

2.在某个类中依赖了某个接口具体实现类,而不是依赖接口。

3.没有按照DI方式分层,单测困难。

4.没有通过spring ioc容器控制反转,直接在类中new成员变量,单测困难 , 依赖细节。

接口隔离原则 (Interface Segregation Principle)

客户端不应该被强迫依赖它不需要的接口。其中的“客户端”,可以理解为接口的调用者或者使用者。

违背ISP原则的案例

1.为了程序分层方便api将对数据库的逻辑DELETE方法也暴露对外了,造成风险。

2.由于接口设计不佳,授信领域间接被强迫依赖了支用领域接口。

3.一个接口方法功能太多,只能兼容一些入参白跑了很多多余的逻辑。

迪米特法则(Law Of Demeter)

每个模块(unit)只应该了解那些与它关系密切的模块(units: only units “closely” related to the current unit)的有限知识(knowledge)。或者说,每个模块只和自己的朋友“说话”(talk),不和陌生人“说话”(talk)。

不该有直接依赖关系的类之间,不要有依赖;有依赖关系的类之间,尽量只依赖必要的接口(也就是定义中的“有限知识”)。

违背LOD原则的案例

1.文件上传接口FileUploadInterface依赖了OSS的Bucket对象,而不是定义抽象的文件上传参数,文件上传不一定要用OSS实现的。

2.还款实体为了得到某个字段,依赖了征信实体,只因为征信实体里面有这个字段,获取方便,但是征信不属于还款领域概念。

功能

背景了解

Code Reviewer 要知道改动的背景,本次改动代码是为了做什么?不能做什么?会影响什么?

逻辑分析

Code Reviewer 根据需求背景,对代码逻辑进行分析,是否满足需求,是否有业务逻辑漏洞。

边界检查

Code Reviewer 检查改动影响的功能模块,是否有边界问题,如果是refactor change,应当检查兼容性,如果是feature change,需要检查领域边界是否合理。

异常监控

Code Reviewer 检查日志处理、异常处理、监控埋点方式等代码,是否存在缺陷、丢失。

非业务功能性检查

Code Reviewer 检查多线程、并发、代码安全、性能等角度,是否存在漏洞,是否可以优化。

复杂度

圈复杂度

Code Reviewer 检查代码圈复杂度情况(if while分支过多、方法分支太多),圈复杂度在1-10为合理范围。

改进方法
  • 简化、合并条件表达式
  • 将条件判定提炼出独立函数
  • 将大函数拆成小函数
  • 以明确函数取代参数
  • 替换算法、策略模式

方法长度

代码长短可以直接说明它复不复杂、阅读成本高不高,长度度量往往是复杂度分析里最简单直接的手段。此处,有效代码长度 = 代码行 - 空白行 - 注释行。当前,集团95%的方法的有效代码行数在42以内。

方法参数长度

方法参数过多会降低容错性、模糊表达意图,参数个数应当在3个以内,可以通过静态创建、参数合并为Model等方式降低长度。

最高结构控制层数

花括号 { } 的最大嵌套层数。深度嵌套的代码总是像俄罗斯套娃一样晦涩难懂,可读性和可维护性大打折扣。当前,集团95%的方法的最大嵌套层数在3以内。

设计复杂度

是否有over设计的情况,简单的两个分支,拓展性低的case,做成了策略模式之类的。

测试

case覆盖率

Code Reviewer 检查单测case覆盖情况,是否覆盖了主要业务流程,严格来说每个逻辑分支都需要覆盖。

可测性

Code Reviewer 检查代码中是否有环境、外部、静态类等依赖,如有,会影响单测隔离性,单测应该可以通过打桩隔绝任何依赖。

规范

单测代码也需要规范,清晰易读,重复性低为准。

命名、注释、风格

代码规范

文档

文档更新

Code Reviewer 提醒 developer 在二分库、应用等升级之后更新文档。

如何撰写 Code Review 评论

总结

  • 保持友善。
  • 解释你的推理。
  • 在给出明确的指示与只指出问题并让开发人员自己决定间做好平衡。
  • 鼓励开发人员简化代码或添加代码注释,而不仅仅是向你解释复杂性。

糟糕的示例:“为什么这里使用了线程,显然并发并没有带来什么好处?”

好的示例:“这里的并发模型增加了系统的复杂性,但没有任何实际的性能优势,因为没有性能优势,最好是将这些代码作为单线程处理而不是使用多线程。”

引用

  • <设计模式之美> 王争
  • <Code Review Developer Guide> Google
©著作权归作者所有,转载或内容合作请联系作者
  • 序言:七十年代末,一起剥皮案震惊了整个滨河市,随后出现的几起案子,更是在滨河造成了极大的恐慌,老刑警刘岩,带你破解...
    沈念sama阅读 226,828评论 6 526
  • 序言:滨河连续发生了三起死亡事件,死亡现场离奇诡异,居然都是意外死亡,警方通过查阅死者的电脑和手机,发现死者居然都...
    沈念sama阅读 97,669评论 3 412
  • 文/潘晓璐 我一进店门,熙熙楼的掌柜王于贵愁眉苦脸地迎上来,“玉大人,你说我怎么就摊上这事。” “怎么了?”我有些...
    开封第一讲书人阅读 174,467评论 0 373
  • 文/不坏的土叔 我叫张陵,是天一观的道长。 经常有香客问我,道长,这世上最难降的妖魔是什么? 我笑而不...
    开封第一讲书人阅读 62,238评论 1 306
  • 正文 为了忘掉前任,我火速办了婚礼,结果婚礼上,老公的妹妹穿的比我还像新娘。我一直安慰自己,他们只是感情好,可当我...
    茶点故事阅读 71,039评论 6 405
  • 文/花漫 我一把揭开白布。 她就那样静静地躺着,像睡着了一般。 火红的嫁衣衬着肌肤如雪。 梳的纹丝不乱的头发上,一...
    开封第一讲书人阅读 54,561评论 1 319
  • 那天,我揣着相机与录音,去河边找鬼。 笑死,一个胖子当着我的面吹牛,可吹牛的内容都是我干的。 我是一名探鬼主播,决...
    沈念sama阅读 42,658评论 3 433
  • 文/苍兰香墨 我猛地睁开眼,长吁一口气:“原来是场噩梦啊……” “哼!你这毒妇竟也来了?” 一声冷哼从身侧响起,我...
    开封第一讲书人阅读 41,806评论 0 285
  • 序言:老挝万荣一对情侣失踪,失踪者是张志新(化名)和其女友刘颖,没想到半个月后,有当地人在树林里发现了一具尸体,经...
    沈念sama阅读 48,316评论 1 329
  • 正文 独居荒郊野岭守林人离奇死亡,尸身上长有42处带血的脓包…… 初始之章·张勋 以下内容为张勋视角 年9月15日...
    茶点故事阅读 40,293评论 3 353
  • 正文 我和宋清朗相恋三年,在试婚纱的时候发现自己被绿了。 大学时的朋友给我发了我未婚夫和他白月光在一起吃饭的照片。...
    茶点故事阅读 42,443评论 1 364
  • 序言:一个原本活蹦乱跳的男人离奇死亡,死状恐怖,灵堂内的尸体忽然破棺而出,到底是诈尸还是另有隐情,我是刑警宁泽,带...
    沈念sama阅读 37,998评论 5 355
  • 正文 年R本政府宣布,位于F岛的核电站,受9级特大地震影响,放射性物质发生泄漏。R本人自食恶果不足惜,却给世界环境...
    茶点故事阅读 43,698评论 3 342
  • 文/蒙蒙 一、第九天 我趴在偏房一处隐蔽的房顶上张望。 院中可真热闹,春花似锦、人声如沸。这庄子的主人今日做“春日...
    开封第一讲书人阅读 34,097评论 0 25
  • 文/苍兰香墨 我抬头看了看天上的太阳。三九已至,却和暖如春,着一层夹袄步出监牢的瞬间,已是汗流浃背。 一阵脚步声响...
    开封第一讲书人阅读 35,333评论 1 281
  • 我被黑心中介骗来泰国打工, 没想到刚下飞机就差点儿被人妖公主榨干…… 1. 我叫王不留,地道东北人。 一个月前我还...
    沈念sama阅读 51,012评论 3 385
  • 正文 我出身青楼,却偏偏与公主长得像,于是被迫代替她去往敌国和亲。 传闻我的和亲对象是个残疾皇子,可洞房花烛夜当晚...
    茶点故事阅读 47,434评论 2 370

推荐阅读更多精彩内容

  • 转载自Jim's blog 关于Code Review的重要性,我相信好的工程师都能认识到。 参考 让Code R...
    Vinc阅读 1,547评论 0 14
  • 通常在企业开发中,会定期面对面(face to face)对代码进行评审 1.Code Review的意识 作为一...
    IIronMan阅读 1,039评论 1 6
  • 架构/设计 单一职责原则这是经常被违背的原则。一个类只能干一个事情,一个方法最好也只干一件事情。比较常见的违背是一...
    苦笑男神阅读 284评论 0 1
  • CodeReview iOS App 稳定性指标及监测(转载) 代码规范及CodeReview要点 iOS-程序员...
    不吃鸡爪阅读 1,382评论 0 1
  • 通常多人协作开发App,为了提高主库代码的质量,保证良好软件架构,代码审核这个环节就显得尤为重要。 代码审核方式 ...
    雨田_Toping阅读 949评论 0 1