敏捷软件开发——第5章　重构

第5章重构

在Martin Fowler的名著《重构》一书中，他把重构定义为：“在不改变代码外在行为的前提下对对代码做出修改，以改进代码内部结构的过程。”可是我们为什么要改进已经能够工作的代码结构呢？我们不是都知道“如果它没有坏，就不要去修理它！”吗？

每一个软件模块都有3项职责。第一个职责是它运行起来所完成的功能。这也是该模块得以存在的原因。第二个职责是它要应对的变化。几乎所有的模块在它们的生命周期中都要变化，开发者有责任保证这种变化应尽可能地简单。一个难以改变的模块是有问题的，即使能够工作，也需要对它进行修正。第三个职责是要能够使其阅读者理解。对该模块不熟悉的开发人员能够比较容易地阅读并理解它。一个无法被理解的模块也是有问题的，同样需要对它进行修正。

怎样才能让软件易于阅读、易于修改呢？本书的主要内容都是关于一些原则和模式的，这些原则和模式主要目标就是为了帮助你创建出更加灵活和具有实用性的软件模块。然而，要使软件模块易于阅读和修改，所需要的不仅仅是一些原则和模式。还需要你的注意力，需要纪律约束，需要创造美的激情。

5.1 素数产生程序：一个简单的重构示例

观察如下代码，这个程序会产生素数。它是一个大函数，其中有辅助阅读的注释和很多单字母变量。

/// <remark>
/// This class Generates prime numbers up to a user specified
/// maximum. The algorithm used is the Sieve of Eratosthenes.
///
/// Eratosthenes of Cyrene, b. c. 276 BC, Cyrene, Libya --
/// d. c. 194, Alexandria. The first man to calculate the
/// circumference of the Earth. Also known for working on
/// calendars with leap years and ran the library at
/// Alexandria.
///
/// The algorithm is quite simple. Given an array of integers
/// starting at 2. Cross out all multiples of 2. Find the
/// next uncrossed integer, and cross out all of its multiples.
/// Repeat until you have passed the square root of the
/// maximum value.
///
/// Written by Robert C. Martin on 9 Dec 1999 in Java
/// Translated to C# by Micah Martin on 12 Jan 2005.
///</remark>
using System;
/// <summary>
/// author: Robert C. Martin
/// </summary>
public class GeneratePrimes
{
    ///<summary>
    /// Generates an array of prime numbers.
    ///</summary>
    ///
    /// <param name="maxValue">The generation limit.</param>
    public static int[] GeneratePrimeNumbers(int maxValue)
    {
        if (maxValue >= 2) // the only valid case
        {
            // declarations
            int s = maxValue + 1; // size of array
            bool[] f = new bool[s];
            int i;
            // initialize array to true.
            for (i = 0; i < s; i++)
                f[i] = true;
            // get rid of known non-primes
            f[0] = f[1] = false;
            // sieve
            int j;
            for (i = 2; i < Math.Sqrt(s) + 1; i++)
            {
                if (f[i]) // if i is uncrossed, cross its multiples.
                {
                    for (j = 2 * i; j < s; j += i)
                        f[j] = false; // multiple is not prime
                }
            }
            // how many primes are there?
            int count = 0;
            for (i = 0; i < s; i++)
            {
                if (f[i])
                    count++; // bump count.
            }
            int[] primes = new int[count];
            // move the primes into the result
            for (i = 0, j = 0; i < s; i++)
            {
                if (f[i]) // if prime
                    primes[j++] = i;
            }
            return primes; // return the primes
        }
        else // maxValue < 2
            return new int[0]; // return null array if bad input.
    }
}

5.1.1 单元测试

为GeneratePrimes编写单元测试：

using NUnit.Framework;
[TestFixture]
public class GeneratePrimesTest
{
    [Test]
    public void TestPrimes()
    {
        int[] nullArray = GeneratePrimes.GeneratePrimeNumbers(0);
        Assert.AreEqual(nullArray.Length, 0);
        int[] minArray = GeneratePrimes.GeneratePrimeNumbers(2);
        Assert.AreEqual(minArray.Length, 1);
        Assert.AreEqual(minArray[0], 2);
        int[] threeArray = GeneratePrimes.GeneratePrimeNumbers(3);
        Assert.AreEqual(threeArray.Length, 2);
        Assert.AreEqual(threeArray[0], 2);
        Assert.AreEqual(threeArray[1], 3);
        int[] centArray = GeneratePrimes.GeneratePrimeNumbers(100);
        Assert.AreEqual(centArray.Length, 25);
        Assert.AreEqual(centArray[24], 97);
    }
}

它才用了一种统计学的方法，检测产生器能够产生0,2,3以及100以内的素数。第一种情况下，应该没有素数。在第二中情况下，应该有一个素数，并且该素数是2。在第三种情况下，应该有两个素数，它们应该是2和3.最后一种情况应该有25个素数，其中最后一个应该是97.如果所有的这些测试都通过了，那么就认为产生器是可以工作的。我怀疑这种做法的可靠性，但是我不能想象出一个合理的情况，在这个情况下这些测试都将通过但是函数却是错误的。

5.1.2 重构

为了有助于重构程序，我使用了带有ReSharper重构附件（来自JetBrains）的Visual Studio。使用该工具可以非常容易地提取方法及重命名变量和类。

显然，主函数分成了3个独立函数。第一个函数对所有的变量进行初始化，并做好过滤所需的准备工作；第二个函数执行过滤工作；第三个函数把过滤结果存放在一个整型数组中。为了更请示地展现这个结构，我把这些函数提取出来放在3个分离的方法中。我还去掉了一些不必要的注释，并且把类名更改为PrimeGenerator。更改后的代码仍然能通过所有测试。

对这3个函数的提取迫使我把该函数的一些局部变量提升为类的静态域。这更清楚地表明了哪个是局部变量，哪个变量的影响更广泛。代码如下：

///<remark>
/// This class Generates prime numbers up to a user specified
/// maximum. The algorithm used is the Sieve of Eratosthenes.
/// Given an array of integers starting at 2:
/// Find the first uncrossed integer, and cross out all its
/// multiples. Repeat until there are no more multiples
/// in the array.
///</remark>
using System;
public class PrimeGenerator
{
    private static int s;
    private static bool[] f;
    private static int[] primes;
    public static int[] GeneratePrimeNumbers(int maxValue)
    {
        if (maxValue < 2)
            return new int[0];
        else
        {
            InitializeSieve(maxValue);
            Sieve();
            LoadPrimes();
            return primes; // return the primes
        }
    }
    private static void LoadPrimes()
    {
        int i;
        int j;
        // how many primes are there?
        int count = 0;
        for (i = 0; i < s; i++)
        {
            if (f[i])
                count++; // bump count.
        }
        primes = new int[count];
        // move the primes into the result
        for (i = 0, j = 0; i < s; i++)
        {
            if (f[i]) // if prime
                primes[j++] = i;
        }
    }
    private static void Sieve()
    {
        int i;
        int j;
        for (i = 2; i < Math.Sqrt(s) + 1; i++)
        {
            if (f[i]) // if i is uncrossed, cross its multiples.
            {
                for (j = 2 * i; j < s; j += i)
                    f[j] = false; // multiple is not prime
            }
        }
    }
    private static void InitializeSieve(int maxValue)
    {
        // declarations
        s = maxValue + 1; // size of array
        f = new bool[s];
        int i;
        // initialize array to true.
        for (i = 0; i < s; i++)
            f[i] = true;
        // get rid of known non-primes
        f[0] = f[1] = false;
    }
}

InitializeSieve函数有一些凌乱，所有我对它进行了相当大的调整。先把所有变量s的地方替换为f.Length。然互，更改了3个函数名字，使它们更具表达力。最后重新安排了InitializeArrayOfIntegers（也就是原来的InitializeSieve）的内部结构，使它更易于阅读。最后代码仍然通过所有的测试。代码如下：

public class PrimeGenerator
    {
        private static bool[] f;
        private static int[] result;
        public static int[] GeneratePrimeNumbers(int maxValue)
        {
            if (maxValue < 2)
                return new int[0];
            else
            {
                InitializeArrayOfIntegers(maxValue);
                CrossOutMultiples();
                PutUncrossedIntegersIntoResult();
                return result;
            }
        }
        private static void InitializeArrayOfIntegers(int maxValue)
        {
            // declarations
            f = new bool[maxValue + 1];
            f[0] = f[1] = false; //neither primes nor multiples.
            for (int i = 2; i < f.Length; i++)
                f[i] = true;
        }
    }

下一步，来看看CrossOutMultiples。这个函数和其他一些函数中有许多形如if(f[i]==true)的语句。这条语句的意图是检查i是否没有被筛选过，所有我把f改名为unCrossed。但是改名后产生了像unCrossed[i]=false这样难看的语句。我发现双重否定会令人迷惑。所以把数组更名为isCrossed，并且更改了所有布尔值的含义。

我去掉了isCrossed[0]和isCrossed[1]为true的初始化语句，并确保函数中的所有部分都不会使用小于2的索引访问isCrossed数组。我提取了CrossOutMultiples函数内部循环部分，把它命名为CrossOutMultiplesOf。同样，我觉得if(isCrossed[i]==false)也会令人迷惑，所以创建了一个名为NotCrossed的函数，把原来if语句更改为if(NotCrossed(i)).

我在一个注释上花了一点时间，这个注释试图解释为何只需要变量至数组长度的平方根。这引导我把计算部分提取成一个函数，其中可以放置说明性的注释。

代码如下：

public class PrimeGenerator
{
    private static bool[] isCrossed;
    private static int[] result;
    public static int[] GeneratePrimeNumbers(int maxValue)
    {
        if (maxValue < 2)
            return new int[0];
        else
        {
            InitializeArrayOfIntegers(maxValue);
            CrossOutMultiples();
            PutUncrossedIntegersIntoResult();
            return result;
        }
    }
    private static void InitializeArrayOfIntegers(int maxValue)
    {
        isCrossed = new bool[maxValue + 1];
        for (int i = 2; i < isCrossed.Length; i++)
            isCrossed[i] = false;
    }
    private static void CrossOutMultiples()
    {
        int maxPrimeFactor = CalcMaxPrimeFactor();
        for (int i = 2; i < maxPrimeFactor + 1; i++)
        {
            if (NotCrossed(i))
                CrossOutputMultiplesOf(i);
        }
    }
    private static int CalcMaxPrimeFactor()
    {
        // We cross out all multiples of p, where p is prime.
        // Thus, all crossed out multiples have p and q for
        // factors. If p > sqrt of the size of the array, then
        // q will never be greater than 1. Thus p is the
        // largest prime factor in the array and is also
        // the iteration limit.
        double maxPrimeFactor = Math.Sqrt(isCrossed.Length) + 1;
        return (int)maxPrimeFactor;
    }
    private static void CrossOutputMultiplesOf(int i)
    {
        for (int multiple = 2 * i;
        multiple < isCrossed.Length;
        multiple += i)
            isCrossed[multiple] = true;
    }
    private static bool NotCrossed(int i)
    {
        return isCrossed[i] == false;
    }
}

最后一个要重构的函数是PutUncrossedIntegersIntoResult。这个函数有两部分功能。第一部分计算了数组中没有过滤掉的整数数目，并创建了一个同样大小的数组来存放这些结果。第二部分把那些没有过滤掉的整数搬移到结果数组中。我把第一部分功能取出来，放到它自己的函数中，并做了其他一些清理工作。代码如下：

private static void PutUncrossedIntegersIntoResult()
    {
        result = new int[NumberOfUncrossedIntegers()];
        for (int j = 0, i = 2; i < isCrossed.Length; i++)
        {
            if (NotCrossed(i))
                result[j++] = i;
        }
    }
    private static int NumberOfUncrossedIntegers()
    {
        int count = 0;
        for (int i = 2; i < isCrossed.Length; i++)
        {
            if (NotCrossed(i))
                count++; // bump count.
        }
        return count;
    }

5.1.3 最后审视

接着，我对整个程序做了最后的审视，从头到尾阅读了一遍，几乎像阅读集合证明，这是非常关键的一步。迄今为止，我们重构的都是代码片段，现在要看这些片段结合在一起是否是一个具有可读性的整体。

我把一些不适合的名字给改掉。去掉了没有必要的“+1”。由于担心一些边角的地方没有测试到，所以另外编写了测试，用来检查2~500所产生的素数列表中没有倍数存在。

重构最终版如下：

///<remark>
/// This class Generates prime numbers up to a user specified
/// maximum. The algorithm used is the Sieve of Eratosthenes.
/// Given an array of integers starting at 2:
/// Find the first uncrossed integer, and cross out all its
/// multiples. Repeat until there are no more multiples
/// in the array.
///</remark>
using System;
public class PrimeGenerator
{
    private static bool[] crossedOut;
    private static int[] result;
    public static int[] GeneratePrimeNumbers(int maxValue)
    {
        if (maxValue < 2)
            return new int[0];
        else
        {
            UncrossIntegersUpTo(maxValue);
            CrossOutMultiples();
            PutUncrossedIntegersIntoResult();
            return result;
        }
    }
    private static void UncrossIntegersUpTo(int maxValue)
    {
        crossedOut = new bool[maxValue + 1];
        for (int i = 2; i < crossedOut.Length; i++)
            crossedOut[i] = false;
    }
    private static void PutUncrossedIntegersIntoResult()
    {
        result = new int[NumberOfUncrossedIntegers()];
        for (int j = 0, i = 2; i < crossedOut.Length; i++)
        {
            if (NotCrossed(i))
                result[j++] = i;
        }
    }
    private static int NumberOfUncrossedIntegers()
    {
        int count = 0;
        for (int i = 2; i < crossedOut.Length; i++)
        {
            if (NotCrossed(i))
                count++; // bump count.
        }
        return count;
    }
    private static void CrossOutMultiples()
    {
        int limit = DetermineIterationLimit();
        for (int i = 2; i <= limit; i++)
        {
            if (NotCrossed(i))
                CrossOutputMultiplesOf(i);
        }
    }
    private static int DetermineIterationLimit()
    {
        // Every multiple in the array has a prime factor that
        // is less than or equal to the root of the array size,
        // so we don't have to cross off multiples of numbers
        // larger than that root.
        double iterationLimit = Math.Sqrt(crossedOut.Length);
        return (int)iterationLimit;
    }
    private static void CrossOutputMultiplesOf(int i)
    {
        for (int multiple = 2 * i;
        multiple < crossedOut.Length;
        multiple += i)
            crossedOut[multiple] = true;
    }
    private static bool NotCrossed(int i)
    {
        return crossedOut[i] == false;
    }
}

测试代码：

using NUnit.Framework;
[TestFixture]
public class GeneratePrimesTest
{
    [Test]
    public void TestPrimes()
    {
        int[] nullArray = PrimeGenerator.GeneratePrimeNumbers(0);
        Assert.AreEqual(nullArray.Length, 0);
        int[] minArray = PrimeGenerator.GeneratePrimeNumbers(2);
        Assert.AreEqual(minArray.Length, 1);
        Assert.AreEqual(minArray[0], 2);
        int[] threeArray = PrimeGenerator.GeneratePrimeNumbers(3);
        Assert.AreEqual(threeArray.Length, 2);
        Assert.AreEqual(threeArray[0], 2);
        Assert.AreEqual(threeArray[1], 3);
        int[] centArray = PrimeGenerator.GeneratePrimeNumbers(100);
        Assert.AreEqual(centArray.Length, 25);
        Assert.AreEqual(centArray[24], 97);
    }
    [Test]
    public void TestExhaustive()
    {
        for (int i = 2; i < 500; i++)
            VerifyPrimeList(PrimeGenerator.GeneratePrimeNumbers(i));
    }
    private void VerifyPrimeList(int[] list)
    {
        for (int i = 0; i < list.Length; i++)
            VerifyPrime(list[i]);
    }
    private void VerifyPrime(int n)
    {
        for (int factor = 2; factor < n; factor++)
            Assert.IsTrue(n % factor != 0);
    }
}

结论

重构后的程序读起来比一开始要好很多。程序工作也更好一些。程序变的得容易理解，因此也更容易改进。并且，程序结构的各个部分之间相互隔离，这同样也使它更容易更改。

你也许担心提取出仅仅调用一次的函数会对性能造成负面的影响。我认为在大多数情况下，提取出方法所增加的可读性是值得花额外的一些微小开销的。然而，也许那些很小的开销存在于很深的内循环中，这样会造成较大的性能损耗。我建议假设这种损耗是可以忽略的，等待以后再去证明这种假设是错误的。

这值得我们花费时间吗？毕竟，程序已经可以完成所需的功能。我强烈推荐你经常对你所编写和维护的每一个模块进行这种重构实践。所投入的时间和随后为自己和他人节省的努力相比起来是非常少的。

重构的目的是为了每天、每时、每分都清洁你的代码。我们不想让脏乱积累。我们想通过最小的努力就能够对我们的系统进行扩展和修改。要想具有这种能力，最重要的就是保持代码整洁。

关于这一点，我怎么强调都不过分。本书中的所有原则和模式对于脏乱的代码来说将没有任何价值。在学习原则和模式前，首先学习编写清洁的代码。

摘自：《敏捷软件开发：原则、模式与实践(C#版)》Robert C.Martin Micah Martin 著