真实记录一次对项目中代码的重构过程

落日映苍穹つ 2023-07-03 06:08 9阅读 0赞

> 这份记录来自对项目中下单接口重构，详细记录了每一步操作、以及运用到的一些方法。力求能够最大程度将当时的过程展现出来。

## 准备 ##

重构中的每次修改都需要进行测试，用来验证修改是否正确，因此单元测试是一个非常好的选择。

### 单元测试 ###

单元测试中可以进行mock操作，从烦人的token中摆脱出来，做到在任何人的IDE里面都能跑起来，从而任何人都能进行重构。当然前期可能工作量会稍微大一些。

### 接口处理逻辑 ###

1.  下单的入参校验。
2.  乘客校验。
3.  根据订单类型判断是否满足下单条件，例如是否已有在进行中的订单。
4.  根据订单类型塞入想要的值到订单表相应字段。
5.  其他根据订单类型进行的操作。
6.  返回。

### 存在的问题 ###

1.  神秘的命名（Mysterious Name）
2.  重复代码（Duplicated Code）
3.  过长函数（Long Function）
4.  重复的Switch（Repeated Switches）

## 主流程重构 ##

抛开Controller里面的代码，直接从Service代码开始。

### 消除重复代码 ###

最开头有一个判断用户是否存在的代码。首先，这个代码相信只要是与用户相关的接口，肯定是需要做这样的判断；再者，这是一个相对独立的功能，可以提成一个函数，用恰当的命名，可以让代码更容易理解。

![format_png][]

创建新的函数，然后**将带提炼的代码拷贝得到新的函数中**，确保对其逻辑没有实质的改动。提炼之后的代码位置为：`fbb4117030c143d86b0b7f58adff4ea910c39d15`，效果如下：

![format_png 1][]

**运行单元测试，检测是否改出了新的问题**。

### 消除变量的神秘命名 ###

对变量、函数的命名是一门玄学，是个仁者见仁智者见智的问题。*就像一千个人眼中有一千个哈姆雷特，每个人眼中正确的命名可能都有不同*。

但是评判的标准都应该是：是否容易理解，是否能够一眼看出它的含义，诸如`int a,b,c`之类的肯定就不可取了。

JetBrains IDEA有很方便的重命名的方法，基本可以保证要改的地方都改了，不该改的不会动。如果实在不放心IDE的修改结果，可以用`git diff`逐项排查。

![format_png 2][]

还有另外一处函数的局部变量也进行了重命名操作，如下：

![format_png 3][]

重构之后的代码位置为：`6fc2330d0ead0429bb8be683c847f40bc338683c`，效果如下：

![format_png 4][]

**运行单元测试，检测是否改出了新的问题**。一定要运行，虽然我也觉得改个名并不会有什么影响。

### 消除重复的switch语句 ###

在检验当前用户是否可以下单的时候，发现里面有一堆if-else的判断，并且在后续的填充订单数据等也有正对订单类型的if-else判断逻辑。

首先要声明的是，在代码中并没有真实的switch语句，但是有嵌套的if-else语句，这个与switch语句在逻辑上是一样的。重复的switch语句做的判断是与**订单类型**相关，并且这样的判断**出现了很多次**。想象一下出现这种情况的场景：对某些类型的订单，做了特殊要求的时候，按照尿性，我们都会顺手写个if-else，如果两个if还不够，两个不够那就多来几个if就完事了，但是这样显然是不友好、不优雅的、不利于阅读代码。

**用多态取代条件表达式**。

1.  新增了`OrderAction`抽象类，包含一个待子类实现的`verify()`方法。
2.  新增四种分别表示订单类型的类，继承自`OrderAction`，实现`verify()`方法。

因此，对订单的校验变成了下面这种情况：

![format_png 5][]

具体的校验策略均拷贝自原来的校验函数中，暂时不做逻辑上的修改。

重构之后的代码位置为：`2329017ba0f420306eae4b443139785c05bc525b`。新增实时拼车单、预约不拼车单的单元测试，**运行单元测试，检测是否改出了新的问题**。

### 重复运用提炼代码 ###

对于刚消除了switch之后的代码，继续对其进行重构。梳理其业务逻辑如下：

1.  查询是否有进行中的订单。
2.  根据需求判断是否可继续下单。

首先从**实时单下单前校验**开始：

![format_png 6][]

虽然这个if没有bug，但是总是感觉很难受，直接删掉了后面那个判断条件。接着看获取进行中订单列表的函数。

![format_png 7][]

这里又进行了一次判断该用户是否存在的逻辑，然而在下单最开始的时候已经做了该判断，所以，这个if判断是不需要的。但是

![format_png 8][]

调用的地方只有两处，并且另外一处就是**预约单下单前校验**的时候调用了该函数。虽然后面代码中，还用到了`memberInfo`的手机号码，去订单表里查询该用户的订单，但是订单表里的每一笔订单后面都加上了user\_id（也就是`memberId`）。因此后面也不需要通过`memberInfo`去拿手机号码，所以这一次数据库查询完全可以去掉，可以节约一点接口处理时间。

在处理完上面的SQL查询后，对于这种局部变量，可以直接用IDEA的提示进行`Inline variable`操作，去掉不必要的局部变量。

![format_png 9][]

所以重构后的代码如下：

![format_png 10][]

另外，在Java中**基本类型**(primitive)优先于**装箱基本类型**(boxed primitive)。

对变量重命名、并删掉局部变量：

![format_png 11][]

运行单元测试，检测是否改出了新的问题，**发现单元测测试过不了**。单元测试终于派上用场了，报错是因为`StringUtils.join()`得到的字符串并不符合SQL语法，如下：

![format_png 12][]

修改成`base-support`下面的`StringUtil.join2SqlInStr`即可修改好改问题。

![format_png 13][]

可见，单元测试在保证每步重构的正确性上面，是有一定的帮助的。

**实时单下单前校验**重构后，代码的位置为：`4dcd17826ddd67f711151e28d569970e00ff0327`。

预约单下单前校验与实时单类似，这里不再展示。

### 重构数据填充 ###

#### 先去掉重复的switch ####

将原有的`dealWithByOrderType()`函数放到`OrderAction`的`fillOrderData()`中，

![format_png 14][]

并在该类中新建`fillOrderDetail()`抽象函数，用来分别处理不同订单类型的数据填充方式，在4个子类中分别实现该函数，将相应的填充代码拷贝进相应的函数中，先不做逻辑修改，暂时先对所需的函数、变量进行调整，让它能正常运行即可。

![format_png 15][]

**运行单元测试，检测是否改出了新的问题**。代码位置为：`b0fc69718184ffa543a57df0210e2086225172dd`。

#### 对各子类中的`fillOrderDetail()`进行重构 ####

后续发现，在填充数据的函数里面，充斥了大量的校验，这个时候函数做的事情与函数名称是不相符合的，这时候就应该使用`提炼函数`方法，将不同的工作提到另外一个函数中。因为是拷贝过来的，所以这就是之前代码中的问题。

但是考虑到后面的流程中还有一个填充数据的流程，所以对这部分的重构，等将整体流程重构完成之后，再一起重构。

### 发送消息 ###

完成初步填充数据后，后面还有一个填充数据的流程，这个if-else里面都有`unifiedCreateOrderProcess()`这个函数，所以可以直接提出来，放到`OrderAction`里面。这个if-else实质上只做了一件事情，那就是根据不同的订单发送不同的消息。

![format_png 16][]

这个if-else真的是有点辣眼睛啊，最起码也得放到`orderList`初始化那一行。但是下单接口改的人确实是多，也可以理解。

![format_png 17][]

在`OrderAction`里面，新建一个发送消息的方法，并将上面if中的代码拷贝进去。根据原代码，得出的结论是只有孩子单没有发送消息的必要，所以只需要在表示孩子单的那个子类中覆盖这个方法，然后函数里面什么都不做，就可以了。**但是这里好像把预约单忘了**，不过也没关系，目前并没有与预约单相关的业务。

所以修改后的代码如下：

下单流程代码：

![format_png 18][]

`OrderAction`代码：

![format_png 19][]

其函数中的具体重构事项，暂时不管，留着后续再次做重构。此次重构后的代码位置为：`30b45bdef6dd69f1737b7d6bde747030a69da5cd`。

**运行单元测试，检测是否改出了新的问题**。

### 存储上下车站点位置 ###

存储上下车站点位置的这段代码看着好像没有啥问题，理解起来也没什么问题，但是始终还是觉得有点辣眼睛。

![format_png 20][]

因为这个`needRecordSites()`函数还是依据订单类型来做判断，返回是true或者false。

![format_png 21][]

这里可以考虑让每个子类返回一个参数，表示是否需要进行上下车站点的记录。形式可以是将主要的代码移到`OrderAction`中，构建一个模板方法；或者直接用运行时的多态即可。这里我选择了后面这种，主代码不移走，仍然放在下单的主流程里面。

![format_png 22][]

![format_png 23][]

删掉之前的`neeadRecordSites(short orderType)`，**运行单元测试，检测是否改出了新的问题**。代码位置为：`2f05795039824d0ddd1d4cfd912069a8bbdf4c79`。

### 主流程扫尾重构 ###

①变量命名：`OrderIds`。改名。

![format_png 24][]

②这段小代码里面明显存在公用的代码。去掉公用的代码。

③是网约车平台监控推送代码提炼成一个函数。

④去掉`orderIds`这个变量。

![format_png 25][]

当前代码位置为：`5b6bac8da`。**运行单元测试，检测是否改出了新的问题**。

## 各函数重构 ##

目前的主流程中如下：

![format_png 26][]

①有一个非常刺眼的`orderType`在其中

②并且记录上下车站点的函数名也很是奇怪

③然后就是校验这一块可以细分成**入参校验**\+**用户状态是否可下单校验**。因为多种类型的订单，公用一个接口进行下单，在Form表单里面不能一概而论的使用框架提供的参数校验。

④在`createOrder()`中调用`insertTravellingOrder()`函数看起来很多余，因为前者只是做一个转发作用，这是可以将后者内联回前者中。

修改后的主流程代码如下，整体比较清楚明白，但是仍然有点瑕疵，那就是`fillOrderData()`与`unifiedCreateOrderProcess()`这个函数是不是可以合并？这个可以待后续观察。

![format_png 27][]

当前代码位置为：`5b6bac8da`。**运行单元测试，检测是否改出了新的问题**。

当前的下单逻辑已经分成几块明显的模块，其中`verifyMemberStatus()`是由之前的`verify()`改名而来，之前已做过重构。接下来就对`fillOrderData()`模块进行重构。

### `fillOrderData()`模块 ###

这部分的修改主要集中在孩子单的处理上，孩子单的填写参数模块中夹杂着校验参数和填写订单信息、校验乘客状态着三部分的代码，使得每个函数的分工不再明确，因此需要将相应的逻辑，移到相应的函数中去。

![format_png 28][]

在调用这个方法之前有一个查询用户是否存在的函数，如果用户存在，但是在这里参数校验过不了，那么在查询用户是否存在的代码中进行的SQL查询就白费了。因此将参数判断放在最开始的地方是有好处的。

![format_png 29][]

当前代码位置为：`698cb047d`。**运行单元测试，检测是否改出了新的问题**。

### `unifiedCreateOrderProcess()` ###

①这个函数参数很奇怪。

![format_png 30][]

这个`appointmentTimeList`到底来自哪里？

![format_png 31][]

到头来还是来自`orderCreateForm`，可是这个参数已经被传进来了。所以这个`appointmentTimeList`可以直接被干掉。

②这个函数里面大部分是执行set操作。只有后面有一个对孩子单生成多日订单的代码，不然整个代码使用于所有的订单类型。因此只需要将公共的代码放入`fillOrderDataCommon()`中即可，生成订单列表的部分，可以交给不同的子类来实现。

![format_png 32][]

当前的代码位置：`f30ec83f7a427bd299b40ed7a0cfe4816faef15d`。**运行单元测试，检测是否改出了新的问题**。

### 发送消息 ###

在发送消息模块，有一个VIP通道，意思大致是：*后端自己去找合适的车辆，找到了就不用传给算法进行计算，没找到再传给算法计算*。这段代码可以**提炼**。是否需要发送给算法，可以**交给子类**。

![format_png 33][]

当前的代码位置：`3ea9c14331d260a6de8fd7966ec8b84af43744bf`。**运行单元测试，检测是否改出了新的问题**。

到目前为止，下单接口的代码已经重构完成了。可以等发布开发环境后，再次用司机端、客户端再次进行测试。

## 后记 ##

**最终还是翻车了，出错的代码在孩子单生成多日订单的时候**。

![format_png 34][]

这个是因为for循环里面的order原来也叫`orderCreateForm`，然后函数的入参变量名也被我改成了`orderCreateForm`，所以这两个一开始的没开始搬家之前，就已经相同了，然后通过IDEA的Refactor-rename修改时，两个都被改成了一样的名称，但是主流程还是没有报错。

[format_png]: https://imgconvert.csdnimg.cn/aHR0cHM6Ly9naXRlZS5jb20vc2FzdXJhaS9waWNzL3Jhdy9tYXN0ZXIvMjAxOTEyMjMxNzExMTIucG5n?x-oss-process=image/format,png
[format_png 1]: https://imgconvert.csdnimg.cn/aHR0cHM6Ly9naXRlZS5jb20vc2FzdXJhaS9waWNzL3Jhdy9tYXN0ZXIvMjAxOTEyMjMxNzM5MjMucG5n?x-oss-process=image/format,png
[format_png 2]: https://imgconvert.csdnimg.cn/aHR0cHM6Ly9naXRlZS5jb20vc2FzdXJhaS9waWNzL3Jhdy9tYXN0ZXIvMjAxOTEyMjMxNzM3NDIucG5n?x-oss-process=image/format,png
[format_png 3]: https://imgconvert.csdnimg.cn/aHR0cHM6Ly9naXRlZS5jb20vc2FzdXJhaS9waWNzL3Jhdy9tYXN0ZXIvMjAxOTEyMjMxNzQ2MDkucG5n?x-oss-process=image/format,png
[format_png 4]: https://imgconvert.csdnimg.cn/aHR0cHM6Ly9naXRlZS5jb20vc2FzdXJhaS9waWNzL3Jhdy9tYXN0ZXIvMjAxOTEyMjMxNzUwMzIucG5n?x-oss-process=image/format,png
[format_png 5]: https://imgconvert.csdnimg.cn/aHR0cHM6Ly9naXRlZS5jb20vc2FzdXJhaS9waWNzL3Jhdy9tYXN0ZXIvMjAxOTEyMjQxNTI0NTYucG5n?x-oss-process=image/format,png
[format_png 6]: https://imgconvert.csdnimg.cn/aHR0cHM6Ly9naXRlZS5jb20vc2FzdXJhaS9waWNzL3Jhdy9tYXN0ZXIvMjAxOTEyMjQxNjA1NTkucG5n?x-oss-process=image/format,png
[format_png 7]: https://imgconvert.csdnimg.cn/aHR0cHM6Ly9naXRlZS5jb20vc2FzdXJhaS9waWNzL3Jhdy9tYXN0ZXIvMjAxOTEyMjQxNjA4NDgucG5n?x-oss-process=image/format,png
[format_png 8]: https://imgconvert.csdnimg.cn/aHR0cHM6Ly9naXRlZS5jb20vc2FzdXJhaS9waWNzL3Jhdy9tYXN0ZXIvMjAxOTEyMjQxNjExNDgucG5n?x-oss-process=image/format,png
[format_png 9]: https://imgconvert.csdnimg.cn/aHR0cHM6Ly9naXRlZS5jb20vc2FzdXJhaS9waWNzL3Jhdy9tYXN0ZXIvMjAxOTEyMjQxNjMyMjkucG5n?x-oss-process=image/format,png
[format_png 10]: https://imgconvert.csdnimg.cn/aHR0cHM6Ly9naXRlZS5jb20vc2FzdXJhaS9waWNzL3Jhdy9tYXN0ZXIvMjAxOTEyMjQxNjM5MzcucG5n?x-oss-process=image/format,png
[format_png 11]: https://imgconvert.csdnimg.cn/aHR0cHM6Ly9naXRlZS5jb20vc2FzdXJhaS9waWNzL3Jhdy9tYXN0ZXIvMjAxOTEyMjQxNzExMjIucG5n?x-oss-process=image/format,png
[format_png 12]: https://imgconvert.csdnimg.cn/aHR0cHM6Ly9naXRlZS5jb20vc2FzdXJhaS9waWNzL3Jhdy9tYXN0ZXIvMjAxOTEyMjQxNjUzMTAucG5n?x-oss-process=image/format,png
[format_png 13]: https://imgconvert.csdnimg.cn/aHR0cHM6Ly9naXRlZS5jb20vc2FzdXJhaS9waWNzL3Jhdy9tYXN0ZXIvMjAxOTEyMjQxNjU2MDkucG5n?x-oss-process=image/format,png
[format_png 14]: https://imgconvert.csdnimg.cn/aHR0cHM6Ly9naXRlZS5jb20vc2FzdXJhaS9waWNzL3Jhdy9tYXN0ZXIvMjAxOTEyMjUxMDE3MzMucG5n?x-oss-process=image/format,png
[format_png 15]: https://imgconvert.csdnimg.cn/aHR0cHM6Ly9naXRlZS5jb20vc2FzdXJhaS9waWNzL3Jhdy9tYXN0ZXIvMjAxOTEyMjUxMDE4MTYucG5n?x-oss-process=image/format,png
[format_png 16]: https://imgconvert.csdnimg.cn/aHR0cHM6Ly9naXRlZS5jb20vc2FzdXJhaS9waWNzL3Jhdy9tYXN0ZXIvMjAxOTEyMjUxMDM3NDAucG5n?x-oss-process=image/format,png
[format_png 17]: https://imgconvert.csdnimg.cn/aHR0cHM6Ly9naXRlZS5jb20vc2FzdXJhaS9waWNzL3Jhdy9tYXN0ZXIvMjAxOTEyMjUxMDQzNTUucG5n?x-oss-process=image/format,png
[format_png 18]: https://imgconvert.csdnimg.cn/aHR0cHM6Ly9naXRlZS5jb20vc2FzdXJhaS9waWNzL3Jhdy9tYXN0ZXIvMjAxOTEyMjUxMTA2MjkucG5n?x-oss-process=image/format,png
[format_png 19]: https://imgconvert.csdnimg.cn/aHR0cHM6Ly9naXRlZS5jb20vc2FzdXJhaS9waWNzL3Jhdy9tYXN0ZXIvMjAxOTEyMjUxMTA3MDAucG5n?x-oss-process=image/format,png
[format_png 20]: https://imgconvert.csdnimg.cn/aHR0cHM6Ly9naXRlZS5jb20vc2FzdXJhaS9waWNzL3Jhdy9tYXN0ZXIvMjAxOTEyMjUxMTEyNDQucG5n?x-oss-process=image/format,png
[format_png 21]: https://imgconvert.csdnimg.cn/aHR0cHM6Ly9naXRlZS5jb20vc2FzdXJhaS9waWNzL3Jhdy9tYXN0ZXIvMjAxOTEyMjUxMTE0MDUucG5n?x-oss-process=image/format,png
[format_png 22]: https://imgconvert.csdnimg.cn/aHR0cHM6Ly9naXRlZS5jb20vc2FzdXJhaS9waWNzL3Jhdy9tYXN0ZXIvMjAxOTEyMjUxMTIyMDUucG5n?x-oss-process=image/format,png
[format_png 23]: https://imgconvert.csdnimg.cn/aHR0cHM6Ly9naXRlZS5jb20vc2FzdXJhaS9waWNzL3Jhdy9tYXN0ZXIvMjAxOTEyMjUxMTIyMzAucG5n?x-oss-process=image/format,png
[format_png 24]: https://imgconvert.csdnimg.cn/aHR0cHM6Ly9naXRlZS5jb20vc2FzdXJhaS9waWNzL3Jhdy9tYXN0ZXIvMjAxOTEyMjUxMTI1MjUucG5n?x-oss-process=image/format,png
[format_png 25]: https://imgconvert.csdnimg.cn/aHR0cHM6Ly9naXRlZS5jb20vc2FzdXJhaS9waWNzL3Jhdy9tYXN0ZXIvMjAxOTEyMjUxMTM5NDAucG5n?x-oss-process=image/format,png
[format_png 26]: https://imgconvert.csdnimg.cn/aHR0cHM6Ly9naXRlZS5jb20vc2FzdXJhaS9waWNzL3Jhdy9tYXN0ZXIvMjAxOTEyMjUxMTU2MTIucG5n?x-oss-process=image/format,png
[format_png 27]: https://imgconvert.csdnimg.cn/aHR0cHM6Ly9naXRlZS5jb20vc2FzdXJhaS9waWNzL3Jhdy9tYXN0ZXIvMjAxOTEyMjUxNDExMzUucG5n?x-oss-process=image/format,png
[format_png 28]: https://imgconvert.csdnimg.cn/aHR0cHM6Ly9naXRlZS5jb20vc2FzdXJhaS9waWNzL3Jhdy9tYXN0ZXIvMjAxOTEyMjUxNTQwMDIucG5n?x-oss-process=image/format,png
[format_png 29]: https://imgconvert.csdnimg.cn/aHR0cHM6Ly9naXRlZS5jb20vc2FzdXJhaS9waWNzL3Jhdy9tYXN0ZXIvMjAxOTEyMjUxNTQ0MDQucG5n?x-oss-process=image/format,png
[format_png 30]: https://imgconvert.csdnimg.cn/aHR0cHM6Ly9naXRlZS5jb20vc2FzdXJhaS9waWNzL3Jhdy9tYXN0ZXIvMjAxOTEyMjUxNTU0MTUucG5n?x-oss-process=image/format,png
[format_png 31]: https://imgconvert.csdnimg.cn/aHR0cHM6Ly9naXRlZS5jb20vc2FzdXJhaS9waWNzL3Jhdy9tYXN0ZXIvMjAxOTEyMjUxNTU1MjcucG5n?x-oss-process=image/format,png
[format_png 32]: https://imgconvert.csdnimg.cn/aHR0cHM6Ly9naXRlZS5jb20vc2FzdXJhaS9waWNzL3Jhdy9tYXN0ZXIvMjAxOTEyMjUxNjMxMzYucG5n?x-oss-process=image/format,png
[format_png 33]: https://imgconvert.csdnimg.cn/aHR0cHM6Ly9naXRlZS5jb20vc2FzdXJhaS9waWNzL3Jhdy9tYXN0ZXIvMjAxOTEyMjUxNjQ4MjgucG5n?x-oss-process=image/format,png
[format_png 34]: https://imgconvert.csdnimg.cn/aHR0cHM6Ly9naXRlZS5jb20vc2FzdXJhaS9waWNzL3Jhdy9tYXN0ZXIvMjAxOTEyMjUxNzMyNDQucG5n?x-oss-process=image/format,png