关于 Replace Temp With Query

这个I disagree系列里面我准备把所有在工作中技术上的争执记录下来。也有立此存照的意思。也许再过几年，回头一看，会自己bs自己一把呢。

今天要记录的，是一个关于martin的refactoring那本书里提到的"Replace Temp With Query"的重构技术。

事情是这样的。在和同事pair的时候，对他频繁使用的这个重构不太同意。搞得同事很不爽。很不好意思的是，我并没有读过这本书，所以对这个重构模式事先是一无所知。这就更加让同事不爽。

当他无奈地指出“我用的是Martin推荐的”的时候，我当时还真有点不敢相信。于是我马上把书拿了过来，读了一遍Replace Temp With Query。

读过之后，我的感觉是，只能部分同意Martin，而对同事对这个重构的使用仍然是无法苟同。

先说对Martin的保留意见：
马丁说这个技术对把大的函数切割成小函数很有作用。这个我是同意的。有时候，当使用eclipse的"extract method"的时候，IDE会给这个新函数提示出七八个参数。这个时候，如果某些参数可以用query代替，自然会减少参数的个数。
不能同意的有：
1。马丁说在作extract method之前，要尽量多地做replace temp with query。而我认为，有那么两三个参数没什么不好。用参数传递比用query来隐含地传递信息有灵活性和清晰性上的好处。通过参数传递相比于通过query传递，有点像dependency injection vs. service locator。
所以，我认为只有在发现某个temp真的影响了extract method，才去做replace temp比较好。
一个类里面如果到处充斥着各种各样的query函数，在我看来也是味道不好闻。

2。马丁对这个重构的局限性和危险性只是一笔带过。实际上，我感觉这个重构的适用范围极小。在一个临时变量的值会变化的时候，或者后面会发生副作用的时候，当然不能直接用这个重构，这点martin也说了。但是，martin举的例子都是query返回一个原始类型。实际使用中更多的是一个query需要返回一个对象。
而java作为一个引用不透明的语言，任何对引用类型局部变量的replace with query动作，理论上都不是安全的。

举个例子：

interface Profile {
  Account getPrimaryAccount();
}
interface Account {
  ...
  Contribution getPreTaxContribution();
}
interface Contribution {
  Balance getBalance();
}

void f(Profile profile, Service service) {
  Account acct = profile.getPrimaryAccount();
  Contribution contrib = acct.getPreTaxContribution();
  Balance balance = contrib.getBalance();
  ...
  if(contrib.isMandatory()){...}
  ...
  service.setContribution(..., contrib);
  ...
  if(balance.isGood()) { ...}
  ...
  service.transferBalance(..., balance);
  ...
}

我们能够简简单单地把contrib和balance这两个temp变成下面的query么？

Contribution contribution(Profile profile) {
  return profile.getPrimaryAccount().getPreTaxContribution();
}
Balance balance(Profile profile) {
  return contribution(profile).getBalance();
}
void f(Profile profile, Service service) {
  ...
  if(contribution(profile).isMandatory()){...}
  ...
  service.setContribution(..., contribution(profile));
  ...
  if(balance(profile).isGood()) { ...}
  ...
  service.transferBalance(..., balance(profile));
  ...
}

Contribution, Profile, Account这些东西都是接口。而在接口的javadoc上没有明确表示getContribution(),getAccount(), getBalance()这些方法都必然一直返回一个对象的引用的时候，我是不敢这么做的。

要知道，我们要重构的是一个架构很凌乱的系统里面的几十个大函数（一百行以上）中的一个，这个系统凌乱到没有一个人清楚知道总体到底是怎么回事，跟踪查找一个bug可能要按F3或者"Reference - Hierarchy"二十多次。
更讨厌的是，要重构的函数没有很好的单元测试。（当然，要是有良好的单元测试，也不会写成这个德性了）

对这种系统，任何不能从理论上证明等价的重构都是危险的。也许，这么重构了之后，不会马上发现问题，但是，它会在我幼小的心灵里面留下的“我那个重构没有问题吧？？？”的阴影的。


google了一下"Replace Temp With Query"，发现马丁有这么一个补充声明：

引用

Paul Haahr pointed out that you can't do this refactoring if the code in between the the assignment to the temp and the use of the temp changes the value of the expression that calculates the temp. In these cases the code is using the temp to snapshot the value of the temp when it's assigned. The name of the temp should convey this fact (and you should change the name if it doesn't).

He also pointed out that it is easy to forget that creating a reference object is a side effect, while creating a value object isn't.

这段话虽然语焉不详，但是它还是呼应了我对这个重构的保留："create a reference object"也是一个side effect。而麻烦的是，对一个接口里面的getSomething()，我基本无法知道这里面有没有一个"create a reference object"。（我的同事对这点不是很同意我的，他会说，我们目前有的两个实现都没有create a reference object。而我在面对一个接口的时候，更倾向于不去看现有的实现类里面到底如何实现的，我只在乎接口的spec,除非spec说这里不允许create a new reference object，我是宁可不做任何假设的。做个proxy之类，把返回值封装一下再返回的这种技术对我来说不是很不可思议的。）




下面再说我对同事的对这个重构的使用方法的不同意见。
1。同事基本上就是上来就能replace的就replace。有一个函数居然最后被重构成：

private Account[] accts;
Account[] filteredAccounts(String type){
  ArrayList ret = new ArrayList();
  for(...) {
    if(type.equals(accts[i].getType())
      ret.add(accts[i]);
  }
  return ret.toArray(new Account[ret.size()]);
} 
void f(String type) {
  for(int i=0; i<filteredAccounts(type).length; i++){
    if(filteredAccounts(type)[i].getBalance()<0) {
      filteredAccounts(type)[i].setValid(false);
    }
  }
}

哎，就算我装作看不见循环里面重复的子循环，或者捏着鼻子念着“不要过早优化”的咒语，这代码阅读起来，调试起来，也不如下面这个简单明了吧？局部变量真的这么可怕？

void f(String type) {
  Account[] found = filteredAccounts(type);
  for(int i=0; i<found.length; i++){
    if(found[i].getBalance()<0) {
      found[i].setValid(false);
    }
  }
}

2。在我表达了我对效率和副作用的担心之后，同事很富有团队精神地声明了几个局部变量来避免这个问题

private Account acct;
private Contribution contrib;
private Balance balance;
private void cleanStates(){
  acct = null;
  contrib = null;
  balance = null;
}
Account account(Profile profile){
  if(acct==null) acct = profile.getAccount();
  return acct;
}
Contribution contribution(Profile profile) {
  if(contrib==null) contrib = account(profile).getPreTaxContribution();
  return contrib;
}
Balance balance(Profile profile) {
  if(balance==null) balance = contribution(profile).getBalance();
  return balance;
}
void f(Profile profile, Service service) {
  cleanStates();
  ...
}

我很不好意思地跟同事说，相比于前一个，我更不喜欢这个方案。两点问题：
1。副作用。我很讨厌引入可变的对象状态。它带来更大的bug几率，还有同步问题。
2。复杂性。代码比原来更多，更复杂了。本来是局部变量的现在变成了全局变量。而我记得从开始学写程序开始，都是局部变量优先于全局变量的。


其实，归根结底，我想跟同事说的是：
replace with query还是小心使用为上。我们先看看有没有必要把这些东西变成query好不好？如果这几个temp真的影响了重构，再研究怎么处置不行么？我在自己的代码重构中似乎还真是很少发现需要使用replace with query的。

你怎么看replace temp with query呢？