关于 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呢？

评论

8 楼 咱不怕 2014-02-11  

如果在一个函数中多次使用到这个不变temp对象,按照Martin的这条原则...
每次使用都调用一次方法,而不是用一个final局部变量记住...以便多次使用..
效率不是更高吗?
就好比如这样:

String[] strs={"aa","b","c"};
		for(int i=0,length=strs.length();i<length;i++){
			System.out.println(strs[i]);
		}

这个效率不是要比 i<strs.length();高吗.
一直都在纠结这个问题..希望听听您的见解...

7 楼 JavaPanFeng 2007-09-18  

refactoring也要具体情况具体分析，
很多重构的方法都是相对的如Extract Method, Inline Method.具体使用那一个就要在项目中权衡利弊后对待。不能一味的照搬书上的， 重构最终的目的是要增加代码的可维护性和可读性，这才是重构的真谛。

6 楼 sg552 2007-08-16  

NULL
NULL
NULL
NULL
NULL
NULL
NULL
NULL
NULL

5 楼 fixopen 2007-08-15  

引用

replace temp with query

我还是比较笨，搞不清楚temp是怎么来的。我个人可能对于lexical scope比临时啊永久呀更敏感一些吧。我也觉得query引入了一个假定和依赖，不知道为什么会有这么一个重构模式。
我觉得更重要的是McConnell的：方法的参数是不是在同一个抽象级别？【当然，同时还有类的方式是不是在同一个抽象级别等等。】我觉得抽象级别的概念跟lexical scope的结构对应起来应该很有价值，不过我没有深入思考过。

4 楼 Readonly 2007-02-06  

一知半解，捧着个书就以为是圣经，最烦你们这种整天把design pattern挂嘴边，却没有一点技术含量的人

写代码和写诗一样，学学白居易念诗给老大娘听，把代码给一个正常的，受过9年义务教育的人看，能够迅速读懂，就是好代码了...

3 楼 pojo 2007-02-06  

翻了一下书，马丁并没有说“要尽量多地做replace temp with query”。他只是说对temp，可以尝试做replace temp with query，还是很中性的。

同意楼上firebody。同时我也认为像你描述的这种杂乱的系统，重构是很危险的。一般很强调一致性的人，匠气是比较重的，免不了为OO而OO，为重构而重构。

2 楼 firebody 2007-02-05  

我觉得  replace temp with query 主要作用在于 代码的易读和清晰性。不过也是主要用在那个temp的获得比较复杂而且难懂， 后面的很多逻辑重复使用这个temp 。 这样的情况下，才考虑使用 replace temp with query.让那个query的命名清晰的表达temp的意思。

你的同事似乎过于书面化了。


至于重构遗留系统的角度来看，最主要的还是把超出 50行的method 分割成多个小的method,然后在些小的method 上 ，一般都会发现很多重复的地方，然后再针对这些小的method作抽象，一般都能够使得重构后的代码大大少于重构前的代码。

1 楼 laofeng 2007-02-05  

看得出来，您研究的很细啊。

我认为您的同事就像有点重构过度了，就像设计过度一样，不会给软件带来改进，反倒会带来麻烦。