重构遗留程序的学习案例

重构遗留程序的学习案例2014-08-07 infoq Chen Ping 译:邵思华遗留代码经常是腐臭的，每个优秀的开发者都想把它重构。而进行重构的一个理想的先决条件是，它应该包含一组单元测试用例，以避免产生回归缺陷。但是为遗留代码编写单元测试可不是件容易的事，因为它经常是一团糟。要想为遗留代码编写有效的单元测试，你大概得先把它重构一下。但要重构它，你又需要单元测试来确保你没有破坏任何功能。这种状况相当于要回答是先有鸡还是先有蛋。这篇文章通过分享一个我曾参与过的真实案例，描述了一种可以安全地重构遗留代码的方法。

问题描述

在这篇文章中，我将用一个真实案例来描述测试与重构遗留系统的有效实践。这个例子的代码由Java编写，不过这个实践对其它语言也是适用的。我将原始场景稍做了些改动以免误伤无辜，并稍做简化以便让它更容易被理解。这篇文章所介绍的实践帮助我重构了近期我所参与的一个遗留系统。

这篇文章并不打算介绍单元测试与重构的基本技巧。你可以通过阅读相关书籍以学习该主题的更多内容，如的及的。相对而言，这篇文章的内容将描述一些真实场景中的复杂性，我也希望它能够为解决这些复杂性提供一些有用的实践。

在这个案例中我将描述一个虚构的资源管理系统，其中资源指的是可指派给其任务的某个人。可以为某个资源指派一个HR票据（ticket）或者IT票据，也可以为某个资源指派一个HR请求或IT请求。资源经理可以记录某个资源处理某项任务的预计时间，而资源本身可以记录他们在某个票据或请求上工作的实际时间。

可以用饼图的方式表示资源的使用情况，图中同时显示了预计时间与实际花费的时间。

好像不太复杂嘛？不过，真实的系统能够为资源分配多种类型的任务，当然从技术上讲这也不是多么复杂的设计。但当我初次看到系统的代码时，我感觉自己似乎看到了一件老古董，从中看得出代码是如何从开始逐步进化的（或者不如说是退化的）。在一开始，这一系统仅能用来处理请求，之后才加入了处理票据以及其它类型任务的功能。某位工程师开始编写代码以处理请求：首先从数据库中获取数据，随后按照饼图的方式显示数据。他甚至没有考虑过要将信息组织为合适的对象：

class ResourceBreakdownService {public Map search （Session context） throws SearchException{ //omitted twenty or so lines of code to pull search criteria out of context and verify them, such as the below:if（resourceIds==null || resourceIds.size （）==0）{throw new SearchException（“Resource list is not provided”）;}if（resourceId！=null || resourceIds.size（）>0）{ resourceObjs=resourceDAO.getResourceByIds（resourceIds）;} //get workload for all requestsMap requestBreakDown=getResourceRequestsLoadBreakdown （resourceObjs,startDate,finishDate）;return requestBreakDown; }}

我相信你肯定被这段代码里的坏味道吓到了吧？比方说，你大概很快就会发现search并不是一个有意义的名称，还有应该使用Apache Commons类库中的CollectionUtils.isEmpty（）方法来检测一个集合，此外你大概也会疑惑该方法返回的Map对象到底包含了些什么？

别着急，坏味道陆续有来。接下来的一位工程师继承了先人的衣钵，按照相同的方式对票据进行了处理，以下就是修改后的代码：

// get workload for all ticketsMap ticketBreakdown =getResourceRequestsLoadBreakdown（resourceObjs,startDate,finishDate,ticketSeverity）;Map result=new HashMap（）;for（Iterator i = resourceObjs.iterator（）; i.hasNext（）;） {Resource resource=（Resource）i.next（）;Map requestBreakdown2=（Map）requestBreakdown.get（resource）;List ticketBreakdown2=（List）ticketBreakdown.get（resource）;Map resourceWorkloadBreakdown=combineRequestAndTicket（requestBreakdown2, ticketBreakdown2）;result.put（resource,resourceWorkloadBreakdown）}return result;

先不管那糟糕的命名、失衡的代码结构以及其它任何代码美观度上的问题了。这段代码中最坏的味道就是它返回的Map对象了，这个Map对象完全是个黑洞，里面塞满了各种数据，但又不会提示你里面究竟包含的是什么。我只能编写了一些调试代码，将Map中的内容循环打印出来后，才看懂了它的数据结构。

在这个示例中，{} 代表一个Map，=> 代表健值映射，而[] 代表一个集合：

{resource with id 30000=> [SummaryOfActualWorkloadForRequestType,SummaryOfEstimatedWorkloadForRequestType,{30240=>[ ActualWorkloadForReqeustWithId_30240, EstimatedWorkloadForRequestWithId_30240], 30241=>[ ActualWorkloadForReqeustWithId_30241, EstimatedWorkloadForRequestWithId_30241] } SummaryOfActualWorkloadForTicketType, SummaryOfEstimatedWorkloadForTicketType, {20000=>[ ActualWorkloadForTicketWithId_2000, EstimatedWorkloadForTicketWithId_2000], }]}

这个糟糕的数据结构使得数据的编码与解码逻辑在直观上非常冗长乏味，可读性很差。

集成测试

希望我已经让你认识到这段代码确实是非常复杂的。如果在开始重构之前让我首先解开这团乱麻，然后理解每一行代码的意图，那我非疯了不可。为了保持我的心智健全，我决定采用一种自顶向下的方式来理解代码逻辑。也就是说，我决定首先尝试一下这个系统的功能，进行一些调试，以了解系统的整体情况，而不是一上来就直接阅读代码，并试图从中推断出代码的逻辑。

我所使用的方法与编写测试代码完全相同，传统的方法是编写小段的测试代码以验证每一段代码路径，如果每一段测试都通过，那么当所有的代码路径组织在一起之后，方法能够按照预期方式工作的机会就很高了。但这种传统方式在这里行不通， ResourceBreakdownService简直就是一个，如果我仅凭着对系统整体情况的一些了解就对这个类进行分解，很可能会造成很多问题 – 在遗留系统的每个角落里都有可能隐藏着众多不为人知的秘密。

我编写了以下这个简单的测试，它反映了我对整个系统的理解：

public void testResourceBreakdown（）{Resource resource=createResource（）;List requests=createRequests（）;assignRequestToResource（resource, requests）;List tickets=createTickets（）;assignTicketToResource（resource, tickets）;Map result=new ResourceBreakdownService（）.search（resource）;verifyResult（result,resource,requests,tickets）;}

注意一下verifyResult（）这个方法，我首先循环式地将result的内容打印出来，以找出其中的结构，随后verifyResult（）方法根据这个结构对结果进行验证，确保其中包含了正确的数据：