综合演练重构 ——以Passalert为案例

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存在往往意味着可能一个构造块（函数，类等）需要抽离出来。如果抽离后还是感觉函数内要标注的时候，很可能说明还需要分拆。