【工程专题】我在 Code Review 中对代码提出的问题

以下内容归纳自过去两年(2018.4-2020.10),我做 Code Review, 所遇到的时机项目中的代码问题
都为真实问题, 经验或猜想的推理.

截图约 230 条(在下面链接中), 对涉及其他组员, 代码等不便公开的内容做了裁删打码处理
指出的问题, 可归为以下几类:

利用以上数据, 要弄清楚:

  • 我纠结哪些 “坏” 代码?
  • 我对代码判断的依据,能成文或者跟进异步体系化?
  • 花很长时间做的 Code Review, 正向效应吗. 有什么办法能将代码问题规避与发生前

问题代码 进一步细分

代码错误(直接导致程序异常):

代码错误

  • 直接的逻辑上错误
  • 对使用的第三方框架, 函数, 用法等理解不足导致用错的
  • 对团队的代码, 类, 文件等分工层级理解不足,导致混乱的
  • 对要使用代码做的实现认知不足,导致用了错误的方式
  • 低性能代码
  • 代码中使用”经验值”(固定时间的延迟等)
  • 低级错误: 无用代码, 用了不释放, 对 0 的处理等

代码设计能更优化:

代码设计优化

  • 对重复或独立模块未做单独封装
  • 代码中的实体(类,文件,函数,命名空间等)实现了超出其定义的逻辑 / 影响到不归他管的内容
  • 实体(类,文件,函数,命名空间等)之间的关系没理清 — 层级混乱, 关系混乱,封装混乱
  • 对于已设计好的各部分, 不理解其职责, 错误的增加代码
  • 逻辑不统一(一样的内容用了多种不同实现方式, 一个”实体”的功能散落在不同”实体”中)
  • 对主要的问题点没抓清楚, 用了间接手段解决
  • 代码中包含只有自己知道的潜规则
  • 多层 if-else 嵌套
  • ”语言” 所提供的类型未能正确使用

写法还能再优化:

写法优化

  • 用官方提倡的方式简化代码
  • 用 key-value 替代 switch-case
  • 使用链式调用避免多层嵌套
  • 不留无用代码

兼顾整体团队的代码统一性:

提高代码可维护性, 代码统一性

  • 代码布局方面(对类内部函数归类, 同类代码写一块)
  • 命名规范方面(同类型东西,统一命名)
  • 项目结构方面(各不同模块存放,文件目录)
  • 日志统一
  • 兼顾维护性

码不达意:

码不达意
变量,类,函数等命名和用途对不上

需求实现错误:

需求纠错
代码没能实现需求


问题代码 具体问题

逻辑错误:

对第三方 设计,函数, 用法 等理解不足导致的错误:

  • react 在 Component 内部改 props
  • Object.assign(), 目的是复制, 却把自己改了
  • promise 使用错误
  • 使用错误的第三方函数
  • 调用 realm 时, 遍历执行锁等耗性能操作. 而不是采取在遍历外层的操作
  • 在应该使用纯函数避免副作用的地方, 调用了业务函数
  • 用了可以实现类似功能接口, 却不了解原理引起的错误
  • 使用 rxjs observer 不实现 complete 和 error (会 crash)
  • 不取消订阅
  • 对 state 异步执行的机制了解不足
  • ScrollView 嵌套 SectionList 为了实现滑动
  • promise 流程不熟悉

对代码中各实体 (文件,类,结构等) 的理解导致的

  • 父视图与子视图的布局合在一块才能给子视图做布局 (子视图依赖父视图/父视图不完全控制子视图)
  • 在应该使用纯函数避免副作用的工具类, 调用了业务函数
  • 在对外的头文件中, 加入模块内部定义, 导致循环引用
  • 子视图直接用屏幕宽度
  • 图层关系弄错
  • 只改了 APP 状态, 没改 APP 连接的设备状态

逻辑错误 或 不完善, 没考虑可能情况

  • 一段明显执行异常的代码
  • 写了个互相影响,然后死循环的逻辑
  • 为了解决一些问题写了条件代码, 但是这个条件引起了更多问题
  • 改了某些行为没改相关状态
  • 没考虑外界调用次数, 导致反复创建或者调用多次
  • 判断条件考虑不周
  • 隐藏弹窗, 只考虑自己的弹窗要隐藏, 没考虑会不归自己管的也隐藏掉

想达成某个目的, 但问题和实现手段理解不足, 用了错误的实现

  • 要拿具备某特性的元素, 这个元素存放于数组中. 代码里直接写序号获取 : 一旦数组变动, 代码就错误
  • 为了方便将一个类型改为几个类型的结合体, 即使不需要 : 可能导致很多 switch-case
  • 为了对比对象, 先序列化再反序列化

低性能代码实现

  • 在渲染时调用遍历或者复杂操作
  • 在遍历中刷新界面
  • state 改变时重新刷整个列表

逻辑不连贯

  • 一块的逻辑被拆分在不同地方分开实现

使用经验值(不该用超时时用超时,并随便给个时间)

  • 有明确状态切换的地方, 用超时处理

低级错误

  • 代码中有无用内容
  • 复制代码未检查
  • 没处理 0
  • 没处理失败情况 (只是打 log,而不是抛出)
  • 用了不销毁
  • 用两个斜杠注释 html
  • if 语句删除了 if 还留着 (重构导致的)
  • 提交 debug 代码
  • 调试时注释的正式代码没还原
  • 编译错误
  • 在异步中的 return 当做了外层函数的 return

代码设计优化

对重复或独立的内容做封装

  • 某部分代码块, 函数, 或者业务逻辑等, 在开发中逐渐多次出现
  • …不细述

做的抽象不合适

  • 入参可以被归纳为 enum, 然后由内部控制具体值
  • 可以用 enum 的值用 key-value 来解决的内容, 变成了 switch
  • 表示同一 Tab, 多种不同值的. 确使用了多了 boolean 而不是 enum 来表示
  • 应该是归父节点管辖的状态, 变成了两个子节点分别实现
  • 对函数做封装, 但是绑定错对象
  • 为了在一个类中加个 id, 增加了一个两个字段的类
  • 相册 list 的封装中, 包含了太多不归 list 处理的东西
  • 管什么都叫 manager

”实体”(类/文件/函数等)实现了超出其定义的逻辑/影响到不归他控制的逻辑

  • 对于 list 和 item 的封装, list 里包含太多细分逻辑
  • UpdateList 时, 把三个列表业务全都包了
  • 在管平台的控件中加入管 ui 的恭喜
  • 在包含三个细分页面的业务中,由一个页面管完所有事情
  • 在某个生命周期函数中实现了太多功能
  • 在纯数据转换函数中, 加入了业务相关的排序
  • 将不必要的东西作为全局状态
  • 在影响到全局的基础封装中, 加了业务相关的内容
  • 在一个定义为存工具类的类中,加入业务功能
  • 不同的状态放在一起

实体间的关系没理清(层级关系, 封装混乱)

  • 在最外层, 最基础的控件中, 引用细分的业务控件
  • 最最基础的控件中, 加入业务功能
  • 内部包含了应该由外部处理的内容
  • 在最底层有专门处理手势控件的情况下, 还在上面相互冲突的内容
  • View 自己,定义了在父控件的坐标

对设计好的各部分职责不理解, 错误使用其职能

  • 代码中使用了 redux 作为数据流, 但是未正确使用, 各类中有很多种不同实现方式
  • 对于代码中划分的层级理解不清, 不清楚各层级应该包含什么内容
  • 对于 redux 的 reduce 应该做什么理解不清
  • 在应该只做纯 UI 的内容中, 管了业务
  • 使用原生 module 没有封装一层而是直接调用
  • 对于纯功能无关业务的类, 做了外部引用
  • 实现逻辑不统一

  • 代码中多处出现不同的 log 库
  • View 对于继承基类不统一
  • 对页面的处理逻辑, 走了两套不同的体系
  • 对于类内部的子视图封装, 用了不同的两套逻辑
  • 一个类中一部分代码自己实现, 一部分功能给了其他部分实现

对主要的问题点没抓清楚,用了间接手段解决问题

  • 需求是不允许手动模式的曝光修改, 但是实现方式确实按钮还在,但是点了不响应

代码里包含潜规则

  • 对于一个 cell, 因为默认了, style 改变 data 肯定改变, 所以将这两个值绑定在一块
  • 在界面刷新的回调中, 做了自适应当前的判断, 一旦入参修改, 整个函数都会出问题
  • 添加了不明所以的倒计时, 而不是用明确的状态实现
  • 给定一个自己估算的大小
  • 延迟执行

    代码只考虑了当前可行性

  • 实现了一个数据转换方法, 但是这个方法一旦多一段就傻逼

多层 if-else 嵌套

没啥好说的, 这是很常见, 但是又比较难处理,拆分的问题

未正确使用语言提供的类型

  • 不该定义可选值的时候使用可选值,导致用的时候麻烦

对问题分析 & 引起问题的原因

… 此处施工🚧

避免问题的方式

… 此处施工🚧