2010年6月12日
9:29
?
我超爱这个函数?，它真是讲解重构的极品。这段代码意图是比较容易看得到的，因为它就写在alertword变量内——检查密码如果符合以下其情况，就返回提示文字
1.?密码为空
2.?密码为连续字符
1.?密码总共只用了不超过2个符号
2.?密码位数不到6位；
我一直在寻找各种各样的案例，然后看到这个案例后，我觉得好像找到了“终极案例”——可以
用很多的重构手法用于此处。比如：
1.?变量就近声明
1.?去除重复
1.?函数抽取
2.?对象内聚
1.?封装简单类型
这段代码还可以演示从一个函数，到一组函数，在到类的过程。对于建立OO思维是很好的案例。
不如读者们自己先试试看这些优化的方法，这样看下面的文字才更有效果。
?
????????private?string?PasswordAlert(string?pwd)
????????{
????????????bool?IsSequence?=?true,?Is2Char?=?true,?IsShort?=?true,?IsEmpty?=?true;
????????????string?tempStr?=?string.Empty,?a?=?string.Empty,?b?=?string.Empty;
????????????string?alertWord?=?"提醒，你的密码有下列情况之一：\n\n1、密码为空；\n2、密码为连续字符；\n3、密码总共只用了不超过2个符号；\n4、密码位数不到6位；\n\n为了密码安全，请修改你的密码。";
?
????????????IsEmpty?=?pwd.Length?==?0;
????????????if?(pwd.Length?<=?1)
????????????????return?IsEmpty?||?IsSequence?||?Is2Char?||?IsShort???alertWord?:?string.Empty;
?
????????????for?(int?i?=?0;?i?<?pwd.Length?-?1;?i++)
????????????{
????????????????if?((int)pwd[i]?+?1?==?(int)pwd[i?+?1]?||?(int)pwd[i]?-?1?==?(int)pwd[i?+?1])
????????????????????continue;
?
????????????????IsSequence?=?false;
????????????????break;
????????????}
????????????for?(int?i?=?0;?i?<?pwd.Length;?i++)
????????????{
????????????????tempStr?=?pwd.Substring(i,?1);
????????????????if?(String.IsNullOrEmpty(a))
????????????????????a?=?tempStr;
????????????????if?(!String.IsNullOrEmpty(a)?&&?String.IsNullOrEmpty(b)?&&?a?!=?tempStr)
????????????????????b?=?tempStr;
????????????????if?(tempStr?==?a?||?tempStr?==?b?||?a?==?string.Empty?||?b?==?string.Empty)
????????????????????continue;
?
????????????????Is2Char?=?false;
????????????????break;
????????????}
????????????IsShort?=?pwd.Length?<?6;
????????????return?IsEmpty?||?IsSequence?||?Is2Char?||?IsShort???alertWord?:?string.Empty;
????????}
?
。
?
代码存在的问题
?
问题1在于：它的意图和代码本身并不自然的匹配，反而存在些不自然的写法。比如第一个if判断的情况和以上4条并不直接相关，第一个for就是检查密码字符是否连续，和2匹配，第二个for检查和规则3匹配，最后一个????????????IsShort?=?pwd.Length?<?6;和规则4一致。大部分还是中规中矩。
问题2?在于：但是为什么那么多的中间变量，并且变量的生命周期，看起来有些复杂，到底那里改了，需要看引用，凡是需要看引用才能知道生命周期的，都是需要考虑如何避免的。
直观的想法就是，代码应该改成这样样子：
?
????????private?string?PasswordAlert(string?pwd)
??????{
?????string?alertWord?=?"XXX...";
return?IsEmpty()?||?IsSequence()?||?Is2Char()?||?IsShort()??alertWord?:?string.Empty;
??????}
?
每个IsXXX函数都直接和语义相配合，从而语义上更加直接呢？
好，我们把他当成目标，来看我们如何把现在的代码变成我们的目标代码！这就是不改变代码的外部行为，但是通过各种方法让代码的内部质量得以提升的过程就是重构。
?
重构第一步：去掉死代码
?
比如：
?if?(pwd.Length?<=?1)
????return?IsEmpty?||?IsSequence?||?Is2Char?||?IsShort???alertWord?:?string.Empty;
代码执行到这里，其实?IsSequence?||?Is2Char?||?IsShort?必然都是true。也就是说代码和
??if?(pwd.Length?<=?1)
????return?alertWord?;
完全等效。
?
既然如此?IsEmpty?||?IsSequence?||?Is2Char?||?IsShort???alertWord?:?string.Empty;就可以说大部分都是死代码，放在这里对阅读者就是一种干扰——尽管我明白，它的意思是当pwd长度为1的时候，符合3种情况。
?
重构第二步：变量就近声明
?
不但函数有职责，函数内的代码块也同样需要按职责来组织。同样职责的代码，尽可能的放到一起，从而更加容易理解。从这个角度分析，最上面的变量声明块
????????????bool?IsSequence?=?true,?Is2Char?=?true,?IsShort?=?true,?IsEmpty?=?true;
????????????string?tempStr?=?string.Empty,?a?=?string.Empty,?b?=?string.Empty;
就应该各就各位，而不是都堆在最上面了。比如tempStr，a,b都仅仅被第二个for的代码块引用，因此应该把这些变量移动下来，放到第二for之上。这就是“变量就近声明”的重构手法。
手工改当然是可以的，但是不够安全——非常幸运的是有工具可以帮助我们来做这个事情。Coderush?express有一个功能叫做
重构第三步：抽取函数
鉴于抽取函数太过简单，太过常用，因此这里就不讲了。没有什么讲头。
一旦完成，在同一个类内，就会有多个新的函数出现，并且和老的PasswordAlert并列。
形如：
??????????????????private?string?PasswordAlert(string?pwd)???{…???????}
?????????private?static?bool?IsShort_(string?pwd){…???????}
????????private?static?bool?IsEmpty_(string?pwd){…???????}
????????private?static?bool?Is2Char(string?pwd){…???????}
????????private?static?bool?IsSequence(string?pwd){…???????}
?
重构第四步：合并函数为类
显然，函数IsShort,IsEmpty,Is2Char,IsSequence?应该是成为一组的。这一组功能和其他代码关系不大，而他们之间则是非常内聚的——都是为了对password进行验证而存在的。因此，从耦合关系上看，把这些方法放到一个构造块内是必要的。不仅从语义上分析应该内聚，从这些个函数的参数内也可以看出端倪。他们有共同的参数就是password字符串。因此引入的新类可以就是Password，这个类内聚有IsShort,IsEmpty,Is2Char,IsSequence方法，并且一旦这样做了后，也不必在一个个的给每个函数传递password参数，而是通过构造函数一次传递进来，其他的函数都可以通过成员变量pwd来共享这个外部参数。
这样完成后，代码形如
Class?Password
{
???????string?pwd;
??????????????????private?string?Password(string?pwd)???{this.pwd=pwd;?}
?????????private?static?bool?IsShort_(){…???????}
????????private?static?bool?IsEmpty_(){…???????}
????????private?static?bool?Is2Char(){…???????}
????????private?static?bool?IsSequence(){…???????}
}
删除多余的参数是很爽的事情！
?
总结
?
演示了从大函数到小函数，从多个小函数变成类的过程。这个方法对我而言已经是一个很好的套路——多次使用，屡试不爽。比如在很久以前的一个项目中，我从大类内的大型函数到若干函数，形成类，把类分离出来。类之间采用委托来耦合，很成功的把一个职责不清的大类分拆成为若干个职责清晰、功能灵活组合的小类。
在去年的一个项目中，也通过类似的手法，把一个大类变成30几个小类，每个类不过200行。
?
可以用自动化的工具来帮助安全的重构。Eric?gamma说，重构是有风险的，风险在于变化中的很多细枝末节，一个不小心就可能改错。他根据和kent?beck的合作经验，认为?small?step?once?time（一次一小步）?的方式可以更好的规避这个风险。
?
在这个案例里面，也说明通过重构工具可以更加安全的重构。以这个案例为例，作为c#的代码，可以通过devexpress?coderush?xpress来帮助重构。步骤二（变量就近声明），步骤三（函数抽取）都是可以用工具来做的，步骤一和四无法用工具，但是步骤四内的”删除参数“是可以用工具的。
工具协助下做的重构都是必然是安全的。它遵守的是等价变换的原则。因此用工具可以让整个过程更加平顺。尽可能用工具，修改后，连测试都是不需要的?。实际上我就是这样去做重构的。
?
对于存在标注函数体，通常是小函数的存在的信号，应该分而治之。另外一个信号是region的存在。region存在往往意味着可能一个构造块（函数，类等）需要抽离出来。如果抽离后还是感觉函数内要标注的时候，很可能说明还需要分拆。
相关文章
