前言
接手不规范的代码是一件痛苦的事，各种code smell会让你有推翻重写的冲动，下面记录一次代码重构练习
代码仓库：https://github.com/Dengqlbq/GildedRose
一言难尽的代码
如果你看到下面这样的代码会作何感想？
public class Rose {
Item[] items;
public Rose(Item[] items) {
this.items = items;
}
public void updateQuality() {
for (int i = 0; i < items.length; i++) {
if (!items[i].name.equals("Aged Brie")
&& !items[i].name.equals("Backstage passes to a TAFKAL80ETC concert")) {
if (items[i].quality > 0) {
if (!items[i].name.equals("Sulfuras, Hand of Ragnaros")) {
items[i].quality = items[i].quality - 1;
}
}
} else {
if (items[i].quality < 50) {
items[i].quality = items[i].quality + 1;
if (items[i].name.equals("Backstage passes to a TAFKAL80ETC concert")) {
if (items[i].sellIn < 11) {
if (items[i].quality < 50) {
items[i].quality = items[i].quality + 1;
}
}
if (items[i].sellIn < 6) {
if (items[i].quality < 50) {
items[i].quality = items[i].quality + 1;
}
}
}
}
}
if (!items[i].name.equals("Sulfuras, Hand of Ragnaros")) {
items[i].sellIn = items[i].sellIn - 1;
}
if (items[i].sellIn < 0) {
if (!items[i].name.equals("Aged Brie")) {
if (!items[i].name.equals("Backstage passes to a TAFKAL80ETC concert")) {
if (items[i].quality > 0) {
if (!items[i].name.equals("Sulfuras, Hand of Ragnaros")) {
items[i].quality = items[i].quality - 1;
}
}
} else {
items[i].quality = items[i].quality - items[i].quality;
}
} else {
if (items[i].quality < 50) {
items[i].quality = items[i].quality + 1;
}
}
}
}
}
}
public class Item {
public String name;
public int sellIn;
public int quality;
public Item(String name, int sellIn, int quality) {
this.name = name;
this.sellIn = sellIn;
this.quality = quality;
}
@Override
public String toString() {
return this.name + ", " + this.sellIn + ", " + this.quality;
}
}
添加测试
重构代码的第一步是看懂代码的逻辑，这里我也只是看了个大概懂。。
然后是写测试，保证你的重构没有改变代码本身的逻辑
看着上面代码那一大堆的 if else嵌套简直奔溃，所以这里采用的方法是每次覆盖一小段逻辑，
简答讲就是一个 if 判断是否成立为一小段
public class RoseTest {
@Test
public void should_update_item_quality_from_1_to_0_and_sell_in_from_2_to_1_when_name_is_any_and_sell_in_is_2_and_quality_is_1() {
// given
Rose rose = new Rose(new Item[]{new Item("any", 2, 1)});
// when
rose.updateQuality();
// then
assertEquals(0, rose.items[0].quality);
assertEquals(1, rose.items[0].sellIn);
}
@Test
public void should_not_update_item_quality_but_sell_in_from_2_to_1_when_name_is_any_and_sell_in_is_2_and_quality_is_0() {
// given
Rose rose = new Rose(new Item[]{new Item("any", 2, 0)});
// when
rose.updateQuality();
// then
assertEquals(0, rose.items[0].quality);
assertEquals(1, rose.items[0].sellIn);
}
// 省略剩余测试
抽取方法
将代码中功能独立的片段抽取成方法有助于重构
if (items[i].sellIn < 0) {
// code...
}
这一段比较独立，可以抽取出来
private void handleItemWithSellInSmallerThanZero(Item item) {
if (!item.name.equals("Aged Brie")) {
if (!item.name.equals("Backstage passes to a TAFKAL80ETC concert")) {
if (item.quality > 0) {
if (!item.name.equals("Sulfuras, Hand of Ragnaros")) {
item.quality = item.quality - 1;
}
}
} else {
item.quality = item.quality - item.quality;
}
} else {
if (item.quality < 50) {
item.quality = item.quality + 1;
}
}
}
这里可以用条件反转，提前结束的方式来减少 if else的层次
先反转最外层的 if else
if (item.name.equals("Aged Brie")) {
item.quality += item.quality < 50 ? 1 : 0;
return;
}
if (!item.name.equals("Backstage passes to a TAFKAL80ETC concert")) {
if (item.quality > 0) {
if (!item.name.equals("Sulfuras, Hand of Ragnaros")) {
item.quality = item.quality - 1;
}
}
} else {
item.quality = item.quality - item.quality;
}
继续反转和优化后：
if (item.name.equals("Aged Brie")) {
item.quality += item.quality < 50 ? 1 : 0;
return;
}
if (item.name.equals("Backstage passes to a TAFKAL80ETC concert")) {
item.quality = 0;
return;
}
if (!item.name.equals("Sulfuras, Hand of Ragnaros")) {
item.quality -= item.quality > 0 ? 1 : 0;
}
代码一下就清晰很多，跑一下测试，测试通过，这一段重构完成
再看代码，for循环中第一个if else是所有item都会进入的，所以也可以抽取出来
for (int i = 0; i < items.length; i++) {
if (!items[i].name.equals("Aged Brie")
// code
} else {
// code
}
private void preHandleItem(Item item) {
if (!item.name.equals("Aged Brie")
&& !item.name.equals("Backstage passes to a TAFKAL80ETC concert")) {
if (item.quality > 0) {
if (!item.name.equals("Sulfuras, Hand of Ragnaros")) {
item.quality = item.quality - 1;
}
}
} else {
if (item.quality < 50) {
item.quality = item.quality + 1;
if (item.name.equals("Backstage passes to a TAFKAL80ETC concert")) {
if (item.sellIn < 11) {
if (item.quality < 50) {
item.quality = item.quality + 1;
}
}
if (item.sellIn < 6) {
if (item.quality < 50) {
item.quality = item.quality + 1;
}
}
}
}
}
}
还是反转和提前结束之类的手段
private void preHandleItem(Item item) {
if (!item.name.equals("Aged Brie")
&& !item.name.equals("Backstage passes to a TAFKAL80ETC concert")) {
if (!item.name.equals("Sulfuras, Hand of Ragnaros")) {
item.quality -= item.quality > 0 ? 1 : 0;
}
} else {
if (item.quality >= 50) {
return;
}
item.quality = item.quality + 1;
if (item.name.equals("Backstage passes to a TAFKAL80ETC concert")) {
if (item.sellIn < 11) {
if (item.quality < 50) {
item.quality = item.quality + 1;
}
}
if (item.sellIn < 6) {
if (item.quality < 50) {
item.quality = item.quality + 1;
}
}
}
}
}
可以发现，效果不大，所以应该换个思路
此时原方法变成：
public void updateQuality() {
for (int i = 0; i < items.length; i++) {
preHandleItem(items[i]);
if (!items[i].name.equals("Sulfuras, Hand of Ragnaros")) {
items[i].sellIn = items[i].sellIn - 1;
}
if (items[i].sellIn < 0) {
handleItemWithSellInSmallerThanZero(items[i]);
}
}
}
处理同一类型
前面抽了两个方法：
handleItemWithSellInSmallerThanZero重构效果明显，preHandleItem重构没什么效果，
通过观察两个方法，发现经常出现 xxx.name.equal(yyy)的情况，也就是说代码会根据name来分类处理，
所以可以将两个方法中处理同一种情况的代码抽取出来
这里将处理 name.equal("Backstage passes to a TAFKAL80ETC concert")的代码抽取并通过分析整体条件得出：
// 抽取后
private void handleItemWithSellInSmallerThanZero(Item item) {
if (item.name.equals("Aged Brie")) {
item.quality += item.quality < 50 ? 1 : 0;
return;
}
if (!item.name.equals("Sulfuras, Hand of Ragnaros")) {
item.quality -= item.quality > 0 ? 1 : 0;
}
}
// 抽取后
private void preHandleItem(Item item) {
if(!item.name.equals("Aged Brie")
&& !item.name.equals("Backstage passes to a TAFKAL80ETC concert")) {
if (!item.name.equals("Sulfuras, Hand of Ragnaros")) {
item.quality -= item.quality > 0 ? 1 : 0;
}
} else {
if (item.quality >= 50) {
return;
}
item.quality = item.quality + 1;
}
}
// 新方法
private void handleBackstageItem(Item item) {
if (item.quality < 50) {
if (item.sellIn < 11) {
if (item.quality < 50) {
item.quality = item.quality + 1;
}
}
if (item.sellIn < 6) {
if (item.quality < 50) {
item.quality = item.quality + 1;
}
}
}
if (item.sellIn < 0) {
item.quality = 0;
}
}
原方法变成：
public void updateQuality() {
for (int i = 0; i < items.length; i++) {
preHandleItem(items[i]);
if (!items[i].name.equals("Sulfuras, Hand of Ragnaros")) {
items[i].sellIn = items[i].sellIn - 1;
}
if (items[i].sellIn < 0) {
handleItemWithSellInSmallerThanZero(items[i]);
}
if (items[i].name.equals("Backstage passes to a TAFKAL80ETC concert")) {
handleBackstageItem(items[i]);
}
}
}
抽取所有不同分类
前面先抽取了两个方法，然后再从两个方法里面发现代码根据name来分类处理从而抽取出了
handleBackstageItem方法，所以我们可以沿着这个思路把所有处理不同情况的代码都抽取出形
成一个个handlexxxItem的方法
这里要注意抽取出来的handlexxxItem方法的处理逻辑是不是符合重构前的逻辑，比如先后顺序
之类的。验证最简单的方法就是每抽取一个方法就跑一遍测试，测试通过则抽取没问题
最后抽取出3种情况：
// normal item 就是 name 不等于代码中出现的任何一个 name 的 item
private void handleNormalItem(Item item) {
item.sellIn -= 1;
item.quality -= item.quality > 0 ? 1 : 0;
if (item.sellIn < 0) {
item.quality -= item.quality > 0 ? 1 : 0;
}
}
private void handleAgedItem(Item item) {
item.sellIn -= 1;
if (item.quality < 50) {
item.quality += 1;
}
item.quality += item.quality < 50 ? 1 : 0;
}
private void handleBackstageItem(Item item) {
item.sellIn -= 1;
if (item.quality < 50) {
item.quality += 1;
}
if (item.sellIn < 0) {
item.quality -= item.quality > 0 ? 1 : 0;
}
if (item.quality < 50) {
if (item.sellIn < 11) {
if (item.quality < 50) {
item.quality = item.quality + 1;
}
}
if (item.sellIn < 6) {
if (item.quality < 50) {
item.quality = item.quality + 1;
}
}
}
if (item.sellIn < 0) {
item.quality = 0;
}
}
// 我们可以发现当 name == Sulfuras, Hand of Ragnaros 时代码什么也没做
原方法变成：
public void updateQuality() {
for (int i = 0; i < items.length; i++) {
if (!items[i].name.equals("Sulfuras, Hand of Ragnaros") && !items[i].name.equals("Backstage passes to a TAFKAL80ETC concert") && !items[i].name.equals("Aged Brie")) {
handleNormalItem(items[i]);
}
if (items[i].name.equals("Backstage passes to a TAFKAL80ETC concert")) {
handleBackstageItem(items[i]);
}
if (items[i].name.equals("Aged Brie")) {
handleAgedItem(items[i]);
}
}
}
到这里代码的逻辑已经很清晰了
策略模式
当我们抽取出一个个handlexxxItem方法时，可以考虑到这种根据不同情况进行处理的代码可以转化为策略模式
创建策略接口：
public interface ItemStrategy {
void update(Item item);
}
创建不同策略（就是将handlexxxItem方法复制过来）：
public class AgedStrategy implements ItemStrategy {
@Override
public void update(Item item) {
item.sellIn -= 1;
if (item.quality < 50) {
item.quality += 1;
}
item.quality += item.quality < 50 ? 1 : 0;
}
}
public class SulfurasStrategy implements ItemStrategy {
@Override
public void update(Item item) {}
}
// 省略其他策略
创建策略工厂：
public class ItemStrategyFactory {
static ItemStrategy createItemStrategy(String name) {
switch (name) {
case "Aged Brie":
return new AgedStrategy();
case "Backstage passes to a TAFKAL80ETC concert":
return new BackstageStrategy();
case "Sulfuras, Hand of Ragnaros":
return new SulfurasStrategy();
default:
return new NormalStrategy();
}
}
}
最后给原Item类增加策略属性和相应的调用方法
public class Item {
// 省略其他code
// 增加策略属性
public ItemStrategy strategy;
public Item(String name, int sellIn, int quality) {
this.name = name;
this.sellIn = sellIn;
this.quality = quality;
getItemStrategy(name);
}
private void getItemStrategy(String name) {
strategy = ItemStrategyFactory.createItemStrategy(name);
}
// 调用策略方法
public void updateQuality() {
strategy.update(this);
}
原方法变成：
for (Item item : items) {
item.updateQuality();
}
跑一遍测试，测试通过，至此重构完成
不过我发现其实NormalStrategy中的update方法还是可以简化的，在抽取handleNormalItem方法的时候或是其他什么时候可以简化掉一部分代码，如果有兴趣你可以试着简化它。
1万+
被折叠的 条评论
为什么被折叠?
到【灌水乐园】发言
