逸言

代码是如何腐化的

| Comments

代码是如何腐化的？这是一个很大的话题，因为这种腐化的代码样本可能会体现不同的特征，若要彻底总结，可能会又是一本《重构》。我自然没有这个能力和知识。好在有一个简便的说法，即可以诉诸于“破窗理论”的威力。无论多少坏味道识别，重构手法运用，提高代码质量的最佳实践，以及运用诸多甄别代码质量体征的工具，都仅仅限于“术”的运用而已。若未能在开发人员内心树立整洁代码的习惯，时时刻刻对各种代码臭味保持敏感，且具有一颗期待卓越代码之心，那么，随着项目的演进，时间的推移，代码最终还是会慢慢腐烂。

这几日在开发一个User Story时，从诸多测试代码（包括集成测试与验收测试）中，观察到了一些接近腐烂的代码坏味。这些代码虽然不是产品代码，但同样是我们交付工件的一部分。最关键之处在于：它让我察觉到一种危险的趋势，若不能及时扭转，可能会让代码陷入腐烂的泥沼。若能及时解决这些糟糕代码，其实仅仅需要一些简单的重构手法，付出几个小时时间即可。

首先是针对集成测试的数据准备。我们要编写的集成测试针对Spring Batch Job，这些Job需要访问数据库，以验证Job的执行是否符合期望。我们发现在之前已有与Spring Batch Job相关的集成测试存在，并提供了访问数据库，以及启动、访问和停止Ftp服务器的功能。其中，与数据准备有关的功能放到单独定义的Fixture类中。这些Fixture是为特定目的编写的数据准备，可是，随着越来越多的Batch Job出现，有诸多集成测试都需要准备数据，于是开始慢慢产生了测试数据的重叠，逐步浮现出违背DRY原则的征兆了。

对于多数程序员而言，并非不重视重用，但多数却不愿意为了重用付出一些代价。例如针对一些具备差异性的功能，一些程序员更愿意使用Copy And Paste，然后再针对自己的需求对实现进行修改或调整。观察目前的一些集成测试，正是这样一些陋习导致的。

在这些集成测试中，使用了继承的方式来重用数据准备的功能。如下图所示：

在CustomerIntegratedDataFixture中，提供了相关方法实现了对Customer数据的创建。由于需要提供访问FtpServer的功能，因此又定义了CustomerIntegratedDataAndFtpPrepareFixture类，使其继承CustomerIntegratedDataFixture。它定义了startFtpServer()和stopFtpServer()方法，并在JUnit中，运用了@BeforeClass与@AfterClass标记，使其避免为每个测试启动和停止专有的FtpServer。现在，我们编写的集成测试同样需要与Customer有关的数据，但并不需要Ftp功能。换言之，我们希望重用CustomerIntegratedDataFixture。现在看来，似乎并没有问题。例如，我们可以让新增的测试直接继承CustomerIntegratedDataFixture。然而，就在同样的集成测试模块中，我们还发现了其他集成测试同样编写了自己的数据准备类。这些数据准备与Spring Batch Job无关，却同样提供了准备Customer数据的功能。存在的差异是它除了提供Customer数据外，还提供了依赖Customer的Consent数据。

我们没有着急去重用CustomerIntegratedDataFixture，因为我们察觉到代码会随着这种继承体系的延伸，会变得越来越难以重用。如上图的继承体系，使得数据准备与Spring Batch Job紧耦合了，同时又在CustomerIntegratedDataAndFtpPrepareFixture子类中引入了与Ftp有关的耦合，明显违背了单一职责原则。我们需要单独剥离出数据准备的类，它即可以作为超类被集成测试类继承，也可以通过组合的方式被继承了JobLauncherTestUtils的测试子类所调用。这符合Bridge模式的设计原则。因此，我们运用了“Replace Inheritance with Delegation”手法，对其进行了简单重构：

之后，我们对Customer和Consent对应的数据准备类进行了相应的重构与修改，使得这些数据的准备更为内聚，并去除一些不必要的重复，使之更容易被重用。

第二个例子是在JBehave的Story中，我看到了这样的Steps类的组织，如图：

我们看到了什么？——一个“扁平组织”的Steps类。显然，促成这样的结果是一个渐进的过程。由于在之前编写相关的Steps类时，还看不到分类的概念，因此，只是简单地将自己的Steps类放到step之下即可。然后，不断有开发人员增加自己的Steps类，他们找到了step位置，却没有仔细思考是否需要更好地对Steps类进行组织。这就使得Steps类略显零乱，没有展现出好的结构。我们重新组织了这些Steps类：

只需要简单地归类，调整结构，整个Steps类就变得更加清晰了。于是，我们发现了可以重用的可能。观察重新组织之后的batch包，这里面包含的UpdateCustomerTypeSteps，ProductSystemLinkLoaderSteps与DeleteOrphanedRecordsSteps，都是与Btach Job有关的Steps类。MaintainProspectsSteps类则是我们新增的类，它同样需要用到启动Batch Job的方法。在之前存在的Steps类中，已经存在相似的代码了。例如在UpdateCustomerTypeSteps类中：

    private String waitAndGetSatus(Map<String, String> params) throws InterruptedException {
        String status = null;
        for (int index = 0; index < MAX_TRY_TIME; index++) {
            status = getJobStatus(params);
            if ("COMPLETED".equals(status)) {
                break;
            }
            Thread.sleep(WAIT_INTERVAL);
        }
        return status;
    }

    private String getJobStatus(Map<String, String> params) {
        return db2JdbcTemplate.queryForObject("select jobExec.STATUS from TCIST_JOB_EXECUTION jobExec " +
                "inner join TCIST_JOB_PARAMS jobParam on jobExec.JOB_INSTANCE_ID = jobParam.JOB_INSTANCE_ID " +
                "and jobParam.KEY_NAME = 'retry' " +
                "and jobParam.STRING_VAL = ?", String.class, params.get("retry"));
    }

再看DeleteOrphanedRecordsSteps类：

    private String waitAndGetSatus(String currentTime) throws InterruptedException {
        String status = null;
        for (int index = 0; index < MAX_TRY_TIME; index++) {
            status = getJobStatus(currentTime);
            if ("COMPLETED".equals(status)) {
                break;
            }
            Thread.sleep(WAIT_INTERVAL);
        }
        return status;
    }

    private String getJobStatus(String currentTime) {
        return jdbcTemplate.queryForObject("select jobExec.STATUS from TCIST_JOB_EXECUTION jobExec " +
                "inner join TCIST_JOB_PARAMS jobParam on jobExec.JOB_INSTANCE_ID = jobParam.JOB_INSTANCE_ID " +
                "and jobParam.KEY_NAME = 'time' " +
                "and jobParam.STRING_VAL = ?", String.class, currentTime);
    }

比较这些方法，除了jobParam的key与value存在细微区别，其余实现完全相同。若按照这样一个态势发展，随着与Batch Job有关的Story逐渐增多，不发现这种代码的臭味并即刻解决，这些代码就会逐渐蔓延，最后变得“无法自拔”。想要修改，已经变得极为困难了。

我们为这些Steps类提供了一个抽象的超类AbstractBatchJobSteps，并将这些可能重用的方法提取到这个超类中：

public class AbstractBatchJobSteps extends AbstractSteps {
    private static final int WAIT_INTERVAL = 1000;
    private static final int MAX_TRY_TIME = 30;

    protected String waitAndGetSatus(Map<String, String> params, String paraKey) throws InterruptedException {
        String status = null;
        for (int index = 0; index < MAX_TRY_TIME; index++) {
            status = getJobStatus(params, paraKey);
            if ("COMPLETED".equals(status)) {
                break;
            }
            Thread.sleep(WAIT_INTERVAL);
        }
        return status;
    }

    private String getJobStatus(Map<String, String> params, String paraKey) {
        return jdbcTemplate.queryForObject("select jobExec.STATUS from TCIST_JOB_EXECUTION jobExec " +
                "inner join TCIST_JOB_PARAMS jobParam on jobExec.JOB_INSTANCE_ID = jobParam.JOB_INSTANCE_ID " +
                "and jobParam.KEY_NAME = '" + paraKey + "' " +
                "and jobParam.STRING_VAL = ?", String.class, params.get(paraKey));
    }
}

如上的例子都可以通过一些细小的重构手法改进代码，使得代码的结构更加清晰，并有利于代码的重用。我深信大多数开发人员都具备这样的技能，且只需要稍加思索，即能发现这些代码的坏味。然而，我们总是因为种种原因，对这种还不太严重的“破窗”风景视而不见。殊不知当我们开始对这种不够整洁的代码采取纵容态度时，就可能会是代码腐化之始。一旦真正腐化，就将积重难返，到了那时，我们就可能真正无能为力了。

你是否遭遇过这样的情形？面对一个承担了无数职责似乎无所不能的上帝类，它被无数多的Client调用，且又没有足够覆盖率的测试，你是否会产生心有余而力不足的感慨。这时的你，是否像一位奋战沙场，出生入死却无力挽回败局的将军，面对那汹涌而来占据压倒性优势的敌军，唯有对天长叹：“某有心杀贼，却无力回天啊！”

Comments