任务3 bugzero

[flymianmian]

bug修复的时候有几步有点大。。

[EthanLin-TWer]

这个争取明天看完哈，看了一下最终结果好像跟我提供那分支差不多。

问一下学员想法，觉得提供一个参考分支是不是有必要呢？

[EthanLin-TWer]

为了达到原子提交的效果，这个提交最好和下个修改测试的提交合并一起提交。这样回退/回滚的时候测试仍然能在提交间保持运行通过。

前面做小调整修修的lint的节奏很好。

[EthanLin-TWer]

赞！“旧的不变，新的创建”。

[EthanLin-TWer]

嗯，字段搬移过来的时候先设置为public是简化“替换使用点(getter)”的常见办法，可以使替换getter的时候步子变得更小。

[EthanLin-TWer]

不知道这个地方做的时候会不会感觉步子大？

一般套路是在setter的地方同样“旧的不变，新的创建”一下，再把getter替换到新的字段上，最后删除老的setter：

goldCoins[currentPlayer]++;
tempPlayers.get(currentPlayer).goldCoin++; // 先同时set到新旧两个地方

然后再做下面getter部分的替换，最后删除上面第一行的旧setter。

[EthanLin-TWer]

还没看后面的提交，不过这个地方完成替换以后一般习惯是先用getGoldCoin()把public字段先封装起来。这个提交 12a280a 也是一样的。

[EthanLin-TWer]

这个地方可不必手搬，见下个commit comment。

[EthanLin-TWer]

这三个提交做的事是把gainGoldCoin()的行为搬移到Player上，在搬移上有自动化的手法/快捷键可以提高效率，也是一种常见的处理思路：

1. 先利用“提炼变量”(《重构2》6.3节, cmd + option + v/ctrl + alt + v)提炼出currentPlayer来：

    private void gainGoldCoin() {
        final Player player = tempPlayers.get(currentPlayer);
        player.goldCoin++;
        System.out.println(getCurrentPlayer()
                + " now has "
                + player.goldCoin
                + " Gold Coins.");
    }

2. 将剩余部分的方法体利用“提炼方法”（《重构2》6.1节，cmd + option + m/ctrl + alt + m）提炼出去：

    private void gainGoldCoin() {
        final Player player = tempPlayers.get(currentPlayer);
        gainGoldCoin(player);
    }

    private void gainGoldCoin(Player player) {
        player.goldCoin++;
        System.out.println(getCurrentPlayer()
                + " now has "
                + player.goldCoin
                + " Gold Coins.");
    }

3. 对getCurrentPlayer()方法应用“内联函数”(《重构2》6.2节，cmd + option + n/control + alt + n)，因为getCurrentPlayer()对Player为private不可见：

    private void gainGoldCoin(Player player) {
        player.goldCoin++;
        System.out.println(player.getName()
                + " now has "
                + player.goldCoin
                + " Gold Coins.");
    }

4. 至此gainGoldCoin()方法中的数据都来源于Player，对该方法应用“搬移函数”(《重构2》8.1节，F6)，将方法搬移到Player类上：

    private void gainGoldCoin() {
        final Player player = tempPlayers.get(currentPlayer);
        player.gainGoldCoin();
    }

5. 完成搬移，对方法做些后处理：先inline player，再inline整个方法：

    private void gainGoldCoin() {
        tempPlayers.get(currentPlayer).gainGoldCoin();
    }

    public boolean wasCorrectlyAnswered() {
        if (tempPlayers.get(currentPlayer).isInPenaltyBox) {
            if (isGettingOutOfPenaltyBox) {
                ...
                nextPlayer();
                tempPlayers.get(currentPlayer).gainGoldCoin();

                return didPlayerWin();
            } else { ... }
        } else { ... }
    }

这里有两个点：

1. “先提炼，后内联”：提炼出可供自动化搬移的结构，搬移后再内联回来。同样也是对过程的设计
2. 过程需要处理下不在目标函数作用域中的变量/函数，在上面就是getCurrentPlayer()

手法虽然看起来很长，但其实练熟了速度更快，还不容易出错。

[EthanLin-TWer]

isWin()可跟上述手法同理搬运。然后搞到这就发现两次为了搬运做了提炼getCurrentPlayer()方法的操作，不如重构完就提炼出这个方法。我做完这个提炼后发现一共有10处地方在用getCurrentPlayer()，也能一定程度为后面“搬移哪些字段”做提示。

[EthanLin-TWer]

后面提交还有些也同理，都可以用这个方法进行搬运，如penaltyBox几个处理方法那块。

[EthanLin-TWer]

这里相当于直接抄了份新方法，改完以后直接把调用点切换过去，而没有在新旧方法中间建立桥梁。那如果这次调用点切换完测试挂了，你能怎么办？回滚copy的所有新东西？调试？因没有搭建桥梁故，所以这种手法不够小步。

我可能会这么做：

1. 先对原来的currentCategory()应用提炼函数（6.1, cmd + option + m/control + alt + m）：

    private String currentCategory() {
        return temp_currentCategory();
    }

    private String temp_currentCategory() {
        if (getCurrentPlace() == 0) return "Pop";
        if (getCurrentPlace() == 4) return "Pop";
        if (getCurrentPlace() == 8) return "Pop";
        if (getCurrentPlace() == 1) return "Science";
        if (getCurrentPlace() == 5) return "Science";
        if (getCurrentPlace() == 9) return "Science";
        if (getCurrentPlace() == 2) return "Sports";
        if (getCurrentPlace() == 6) return "Sports";
        if (getCurrentPlace() == 10) return "Sports";
        return "Rock";
    }

2. 修改temp_currentCategory()的返回值类型为Category，在旧方法中间将其转换为String并保持功能不变：

    private String currentCategory() {
        return temp_currentCategory().getValue();
    }

    private Category temp_currentCategory() {
        if (getCurrentPlace() == 0) return Category.POP;
        if (getCurrentPlace() == 4) return Category.POP;
        if (getCurrentPlace() == 8) return Category.POP;
        if (getCurrentPlace() == 1) return Category.SCIENCE;
        if (getCurrentPlace() == 5) return Category.SCIENCE;
        if (getCurrentPlace() == 9) return Category.SCIENCE;
        if (getCurrentPlace() == 2) return Category.SPORTS;
        if (getCurrentPlace() == 6) return Category.SPORTS;
        if (getCurrentPlace() == 10) return Category.SPORTS;
        return Category.ROCK;
    }

3. 对调用点也先做“旧的不变，新的创建”的操作：

    private void askQuestion() {
        String currentCategory = currentCategory();
+        Category temp = Category.valueOf(currentCategory.toUpperCase());
        System.out.println("The category is " + currentCategory);

        ...
    }

4. 先替换一个调用点试试水：

    private void askQuestion() {
        String currentCategory = currentCategory();
        Category temp = Category.valueOf(currentCategory.toUpperCase());
-        System.out.println("The category is " + currentCategory);
+        System.out.println("The category is " + temp.getValue());

        ...
    }

5. 4测试通过的话再替换其他引用点，不通过的话出错范围只有1行，回滚之找找原因：

    private void askQuestion() {
-        String currentCategory = currentCategory();
        Category temp = Category.valueOf(currentCategory.toUpperCase());
        System.out.println("The category is " + temp.getValue());
        if (temp == Category.POP)
            System.out.println(questionMap.get(Category.POP).removeFirst());
        if (temp == Category.SCIENCE)
            System.out.println(questionMap.get(Category.SCIENCE).removeFirst());
        if (temp == Category.SPORTS)
            System.out.println(questionMap.get(Category.SPORTS).removeFirst());
        if (temp == Category.ROCK)
            System.out.println(questionMap.get(Category.ROCK).removeFirst());
    }

6. 清理，先内联String currentCategory变量，最后删除无用的currentCategory()并将temp_currentCategory()重命名回来：

    private void askQuestion() {
        Category temp = temp_currentCategory();
        System.out.println("The category is " + temp.getValue());
        if (temp == Category.POP)
            System.out.println(questionMap.get(Category.POP).removeFirst());
        if (temp == Category.SCIENCE)
            System.out.println(questionMap.get(Category.SCIENCE).removeFirst());
        if (temp == Category.SPORTS)
            System.out.println(questionMap.get(Category.SPORTS).removeFirst());
        if (temp == Category.ROCK)
            System.out.println(questionMap.get(Category.ROCK).removeFirst());
    }

这里的重点是要在新旧函数中间搭建调用桥梁，这样后续才能每改一两行代码都跑测试验证。copy整份方法及调用点并直接替换的方式，在方法更长、更复杂、调用点更多的场景下就会步子很大，增加出错概率和排查范围。

[EthanLin-TWer]

这个提交是不是也能用之前搬移方法的方式做？

[EthanLin-TWer]

不错哟，做到了只需要加一行代码就能新增特性。

[EthanLin-TWer]

同理直接用F6快捷键搬过来就行，不需要手搬。

[EthanLin-TWer]

方法名是否可尝试简短些？例如：

Category currentCategory = Category.in(getCurrentPlace());

[EthanLin-TWer]

这三个提交 4c6f0d8 912615a 39ead3d 步子大了，真正执行替换时 e7d61bc 应该做不到每改一行代码都能运行测试吧？

替换方式可以像前面讲的，在setter的地方多set一次到PlayerContainer中去，再替换getter，最后删除一个字段，完成替换。然后再进行其他字段的替换。一次只搬移一个字段。

可参考《重构2》8.2节 搬移字段（Move Field）。

[EthanLin-TWer]

嗯，这个地方把字段用getter函数封装起来了很好。

[EthanLin-TWer]

Review done。

一些地方的手法还有可打磨的地方，不过总体的步子和手法应用还是很不错的。在执行“旧的不变，新的创建”时，需要更多依赖快捷键和IDE帮你做提炼并搭建好中间的调用桥梁，不需要手动去做创建，这样有利于进一步缩小步子，使重构更可控。

继续精益求精。很棒哟~