精益求精，抑或得过且过

本文转自http://www.cnblogs.com/wayfarer/archive/2011/02/18/1957530.html, 其目的在于长期保留这篇文章, 从长远来看, 代码质量引起的问题太大也太多了, 希望能引起更多人的注意, 把模块化设计, 单元测试, 代码重构做到切实可行.

程序员面临的最痛苦之事，莫过于修改旧代码；如果还有比这更痛苦的，就是修改糟糕透顶，乱得一团糟的烂代码。最近因为手底下一帮程序员都在忙，市场部正好又反馈过来一个要命的bug，一时手痒，就领下了这个任务。我们这个产品是针对教育行业的，它是在好几年前开发，然后不断完善和维护。这些阶段都是在我来到这家公司之前完成的。所以，我对于产品的代码并不熟悉。

原来的需求是假定客户设置分数段时，不同的分数段有不同的有效分，对应着也就有不同的名次。这些数据都是经过分析器分析获得，并持久化到数据库中。当我们需要生成学生报告时，再从数据库中获取，并将数据填充到iReport设置好的模板中，一个是二维表，一个是柱状和曲线图。

现在，我们发现某些学校需要给不同的分数段设置完全相同的有效分，以及相同的名次。报告打印出来，二维表没有错，曲线图却出现了“缺斤少两”的现象。例如设置五个分数段，却可能只显示了四条曲线。

阅读代码，我明白了原因。在原来的实现中，由于默认不同分数段有不同名次，因此，在获取这些分数段的值时，是将它们放入到一个Map<Subject, Map<Integer, Double>>中。这个Map是根据科目进行分类的，子Map的key值为Integer类型，其实就是分数段对应的名次，value则是设置的有效分。由于现在的名次存在重复，导致Map中的元素存在偏差。这就是五个分数段只显示四条曲线的缘由。

事实上，在我看到这样的Map时，就明白这种“超级强大”的容器，事实上往往会成为坏代码的泥沼。当我们将这样的对象作为参数，在方法之间传来传去的时候，会带来诸多问题：
1）性能影响。这种可能比较庞大的容器对象，有可能会形成性能瓶颈；
2）强类型。虽然这里使用了泛型，但泛型类型却使用了基本类型；
3）封装性不够好。这样的容器对象暴露了太多的数据细节，且不利于为其定义职责行为。
4）可读性差。看到这样的Map，你并不会在第一时间明白它到底存放了什么。
5）可扩展性差：当这个Map作为方法的参数时，相当于这个参数没有被对象化。如果拥有这个参数的方法被公开，且广泛调用，一旦需要改变参数，牵连到的代码就会非常多。

事实正是如此。当我在分析我们的遗留代码时，发现很多地方都在重复获取这个Map对象，并且这个Map对象也在多个方法之间传递。因为这种容器对象自身的缺陷，为我的bug修复带来了很多阻碍。要解决这个Bug，就不能再将名次作为Map的key值。查看相关的数据表，事实上我们还给出了一个分数段的名称。当名次和有效分存在重复的情况下，结合分数段名称就能确定唯一值。因此，一个简单地做法就是将Map的key修改为名次加名称的组合，即将Integer修改为String。

现在，我需要决策。如果希望一劳永逸，就应该摒弃这种Map的做法。我们应该利用封装来实现这一目标。例如定义ValidScore和ValidScoreRange。后者相当于之前的Map<Integer, Double>。ValidScore则包含属性：Rank, LineName, Score。事实上，ValidScoreRange正是ValidScore的集合。我们还可以在ValidScore和ValidScoreRange中定义许多与该领域相关的行为。通过这样的封装，问题会变得简单许多。

然而，思考良久，我却放弃了这个符合OO原则的做法。原因在于，遗留代码中使用Map<Subject, Map<Integer, Double>>的类和方法有很多，特别让人烦恼的是在这些方法中，有很多还定义在某些公共类中，被系统的大多数Client所调用。例如这样的代码：

public static JFreeChart createEliteTotalChart(Grade grade, String partial,
        ExamSet es, Student stu, Map subToStuTotalMap,
        Map<Subject, Map<Integer, Double>> subToValidScoreMap, 
        List subList,long stuRank,
        String[] validLineSeries,double barWidth,Color barColor) {
    if (subToStuTotalMap == null || subList == null) {
        return null;
    }
    Color[] color = {Color.RED,Color.BLUE,Color.GREEN,
                     Color.CYAN,Color.BLACK,Color.MAGENTA};
    CategoryDataset[] dataset = EIDatasetFactory.createEliteTotalDataset(
            subToStuTotalMap, subToValidScoreMap, subList,validLineSeries);

    //......为清晰起见，其余代码略
}

注意，在createEliteTotalChart()方法中，调用了EIDatasetFactory的createEliteTotalDataset()，对Map<Subject, Map<Integer, Double>>对象进行了处理。EIDatasetFactory正是一个公开的公共类，createEliteTotalDataset()方法也是一个被广泛调用的方法。

好吧，我们可以利用重构来完成代码的改造。但是我现在不敢轻易修改这些方法。因为我并不知道这些方法除了生成有效分报告外，还有哪些功能会调用。我们当然可以通过寻找引用来识别到底有哪些功能会调用，事实上，我找到了好几个引用。可是我依旧需要保持足够的谨慎。究其原因，是之前的遗留代码并没有提供充分的单元测试，无法通过单元测试用例来保证这一修改的正确性。

好吧。我得承认重构本来就离不开单元测试，除非是那些直接可以利用工具就能完成的重构。问题不是我不能重构，也并非我的能力达不到；问题也不是我无法保证修改的正确性；真正的问题是我没有时间。作为公司的技术领导，我可以向CEO争取这个时间吗？不能！因为CEO必须考虑成本，他要考虑花这么多时间去做看似不能增加功能的重构是否值得。或许你会说，做这样一个小小的重构需要花太多时间吗？问题是在这个遗留系统中，这样的代码简直数不胜数。所以，在这里时间才是真正的罪恶。不过，最大的罪恶还是人，是编写代码的人。如果在一开始开发系统的时候，我们就能注意代码质量，随时重构，并遵循面向对象设计原则，也许境况会变得焕然一新。这就是所谓的“破窗理论”了。从中可见，代码质量对系统架构的影响。

面对这样一座满是破窗的大楼，我们是下定决心推倒重建，还是修修补补维持现状？这实在是一个哈姆雷特似的问题。站在技术人员的角度，我倾向于推倒重建；然而站在公司领导的角度，却不得不思考成本与时间。

因为这个遗留系统还可以苟延残喘，因为我的时间不允许停留在这个遗留系统太久，我还有新产品的架构工作要做。最终我选择了“得过且过”。我针对这一特定需求，找到了调用的方法。为了不影响其他功能，我利用Copy&Paste的方式复制了该方法，只是为它换了一个名字。当然，我还可以利用Extract Method重构来尽量避免代码的重复。然后，将Map<Subject, Map<Integer, Double>>对象修改为Map<Subject, Map<String, Double>>。因为该方法又连续调用多个其他方法，传递了Map<Subject, Map<Integer, Double>>对象，我又如此炮制地复制了这些被调用方法，最终解决了这个Bug。可是看到这么多Copy&Paste的重复代码，我不寒而栗，决定仓皇逃走。在时间这个强大敌人的面前，我悲哀地选择了投降。