重构代码的经典案例_重构旧版应用程序：一个案例研究

重构代码的经典案例

旧版代码很烂。 每个体面的开发人员都希望对其进行重构，并且为了对其进行重构，理想情况下，应该有一套单元测试用例来防止回归。 但是，为遗留代码编写单元测试并不容易。 遗留代码通常是一团糟。 要针对遗留代码编写有效的单元测试，您可能需要首先对其进行重构。 并对其进行重构，您需要进行单元测试以确保您没有破坏任何东西。 因此，这是鸡和蛋的情况。 本文介绍了一种方法，可以通过共享我曾经处理过的真实案例来安全地重构遗留代码。

问题陈述

在本文中，我将用一个真实的案例来说明测试和重构遗留系统的实用做法。 这个案例是用Java编写的，但是这些实践应该适用于其他语言。 我更改了方案以保护无辜者，并对其进行了简化以使其更易于理解。 本文介绍的实践有助于重构我最近使用的遗留系统。

本文与单元测试和重构的基本技能无关。 您可以通过阅读诸如Refactoring： Martin Fowler的 《改进现有代码的设计》和Joshua Kerievsky的《 将模式重构 》之类的书来了解更多有关这些主题的知识。 相反，本文描述了现实生活中经常遇到的一些复杂性，并希望提供有用的实践来解决这些复杂性。

我将使用的示例案例研究是一个虚构的资源管理系统，其中的资源是指可以分配某些任务的人员。 可以说，可以将资源分配给票证-人力资源票证或IT票证。 也可以将资源分配给请求-人力资源请求或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 requests

        Map requestBreakDown=getResourceRequestsLoadBreakdown (resourceObjs,startDate,finishDate);
        return requestBreakDown;
   }
}

我确信您会立即被该代码的臭味所排斥，例如，您可能立即想到搜索不是一个有意义的名称，它应该使用Apache Commons库CollectionUtil.isEmpty（）来测试一个集合，并且您可能在质疑生成的地图 ？

但是，等等，恶臭继续累积。 第二个工程师走了出来，按照同样的方式处理票务，因此在代码中，您有：

// get workload for all tickets
Map 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;

忘了命名，结构的平衡和其他美学问题，此代码最臭的事情是返回的M ap对象。 地图是一个黑洞，它吸收了所有内容，并没有为您提供有关其中包含的内容的许多线索。 仅通过编写一些调试代码以递归方式打印地图的内容，我才能够找到其数据结构。

在此示例中， {}表示映射， =>表示键值映射， []表示集合：

{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（）方法，我首先通过递归打印结果的内容来找到结果的结构，然后verifyResult（）使用此知识来验证其包含正确的数据：

private void verifyResult(Map result, Resource rsc, List<Request> requests, List<Ticket> tickets){
    assertTrue(result.containsKey(rsc.getId()));
       
    // in this simple test case, actual workload is empty
    UtilizationBean emptyActualLoad=createDummyWorkload();        
    List resourceWorkLoad=result.get(rsc.getId());        
     
    UtilizationBean scheduleWorkload=calculateWorkload(rsc,requests);
    assertEquals(emptyActualLoad,resourceWorkLoad.get(0));       
    assertEquals(scheduleWorkload,resourceWorkLoad.get(1));                       
                       
    Map requestDetailWorkload = (Map)resourceWorkLoad.get(3);
    for (Request request : requests) {
        assertTrue(requestDetailWorkload.containsKey(request.getId());
        UtilizationBean scheduleWorkload0=calculateWorkload(rsc,request);
        assertEquals(emptyActualLoad,requestDetailWorkload.get(request.getId()).get(0));
        assertEquals(scheduleWorkload0,requestDetailWorkload.get(request.getId()).get(1));               
    }

    // omit code to check tickets
       ...
}

绕过障碍

上面的测试用例似乎很简单，但是有很多复杂性。 首先，将ResourceBreakdownService（）。search紧密编织到运行时中，它需要访问数据库，其他服务以及其他人知道什么。 而且，像许多旧系统一样，该系统没有构建任何单元测试基础结构。 要访问运行时服务，唯一的选择是启动整个系统，这非常昂贵且不便。

启动系统服务器端的类是ServerMain 。 ServerMain也像化石一样，您可以从中分辨出它是如何演变的。 这个系统是十多年前编写的，那时没有Spring ，没有Hibernate ，只有一点JBoss和一点Tomcat 。 当时的英勇先驱不得不做很多DIY，因此他们创建了一个自制的集群，一个缓存服务和一个连接池等。 后来他们以某种方式插入了JBoss和Tomcat（但不幸的是，他们留下了自己的DIY，因此代码中留有两组事务管理机制和三种类型的连接池。

我决定将ServerMain复制到TestServerMain 。 调用TestServerMain.main（）失败，原因是：

org.springframework.beans.factory.BeanInitializationException: Could not load properties; nested exception is
java.io.FileNotFoundException: class path resource [database.properties] cannot be opened because it does not exist
at
org.springframework.beans.factory.config.PropertyResourceConfigurer.postProcessBeanFactory(PropertyResourceConfigurer.java:78)

好的，很容易解决！ 我抓取了一个database.properties文件，并将其放置到测试类路径中，然后再次运行它。 这次，该异常被抛出：

java.io.FileNotFoundException: .\server.conf (The system cannot find the file specified)
    at java.io.FileInputStream.open(Native Method)
    at java.io.FileInputStream.<init>(FileInputStream.java:106)
    at java.io.FileInputStream.<init>(FileInputStream.java:66)
    at java.io.FileReader.<init>(FileReader.java:41)
    at com.foo.bar.config.ServerConfigAgent.parseFile(ServerConfigAgent.java:1593)
    at com.foo.bar.config.ServerConfigAgent.parseConfigFile(ServerConfigAgent.java:1720)
    at com.foo.bar.config.ServerConfigAgent.parseConfigFile(ServerConfigAgent.java:1712)
    at com.foo.bar.config.ServerConfigAgent.readServerConf(ServerConfigAgent.java:1581)
    at com.foo.bar.ServerConfigFactory.initServerConfig(ServerConfigFactory.java:38)
    at com.foo.bar.util.HibernateUtil.setupDatabaseProperties(HibernateUtil.java:207)
    at com.foo.bar.util.HibernateUtil.doStart(HibernateUtil.java:135)
    at com.foo.bar.util.HibernateUtil.<clinit>(HibernateUtil.java:125)

server.conf，但是这种方法让我感到不舒服。 仅通过编写此单个测试用例，就立即在代码中暴露了一个问题。 顾名思义， HibernateUtil应该只关心应由database.properties提供的数据库信息 。 为什么要访问配置服务器范围设置的server.conf ？ 这是一个代码有臭味的线索：如果您觉得自己正在读一本侦探小说并不断问“为什么”，那么代码通常是不好的。 我本来可以花很多时间阅读ServerConfigFactory ， HibernateUtil和ServerConfigAgent并试图弄清楚如何将HibernateUtil指向database.properties ，但是在这一点上，我太着急了，无法获得正在运行的服务器。 此外，还有一种方法可以绕过它。 武器是AspectJ ：

void around():
    call(public static void com.foo.bar.ServerConfigFactory.initServerConfig()){
    System.out.println("bypassing com.foo.bar.ServerConfigFactory.initServerConfig");
}

对于不懂AspectJ的人，用简单的英语来说，上面的意思是：当运行时是在调用ServerConfigFactory.initServerConfig（）时 ，打印一条消息并返回而无需进入方法本身。 这可能看起来像黑客攻击，但非常划算。 遗留系统充满了难题和问题。 在任何给定的时刻，人们都需要从战略上进行战斗。 那时，就客户满意度而言，对我而言最有意义的事情就是修复资源管理系统中的缺陷并提高其性能。 理顺其他领域的事情不在我的关注范围之内。 但是，我想到了以后再回来整理ServerMain中的混乱情况。

然后在HibernateUtil从server.conf读取所需的信息时，我指出它是从database.properties读取的：

String around():call(public String com.foo.bar.config.ServerConfig.getJDBCUrl()){
    // code omitted, reading from database.properties
}

   String around():call(public String com.foo.bar.config.ServerConfig.getDBUser()){
    // code omitted, reading from database.properties 
}

您现在可能已经猜到了其余的内容：这样做更方便或更自然时，绕过障碍物。 但是，如果有现成的模型，请重用它们。 例如，在某一时刻， TestServerMain.main（）失败并显示：

- Factory name: java:comp/env/hibernate/SessionFactory
- JNDI InitialContext properties:{}
- Could not bind factory to JNDI
javax.naming.NoInitialContextException: Need to specify class name in environment or system property, or as an applet
parameter, or in an application resource file: java.naming.factory.initial
    at javax.naming.spi.NamingManager.getInitialContext(NamingManager.java:645)
    at javax.naming.InitialContext.getDefaultInitCtx(InitialContext.java:288)

这是因为尚未启动JBoss命名服务。 我本可以使用相同的黑客技巧来解决这个问题，但是InitialContext是一个包含许多方法的大型Javax接口，我不想追逐和破解每种方法-这将非常繁琐。 快速搜索显示Spring已经有一个模拟类SimpleNamingContext ，因此我将其放入了测试中：

SimpleNamingContextBuilder builder = new SimpleNamingContextBuilder();
builder.bind(“java:comp/env/hibernate/SessionFactory”,sessionFactory);
builder.activate();

经过几轮，我能够使TestServerMain.main（）成功运行。 与ServerMain相比，它要简单得多，它模拟了许多JBoss服务，并且没有与集群管理纠缠在一起。

建造积木

TestServerMain已连接到真实数据库。 传统系统可能在存储过程中隐藏令人惊讶的逻辑，甚至在触发器中更糟。 遵循同样的大思路，当时我认为尝试了解数据库内部的奥秘并伪造数据库对我来说并不明智，因此我决定让我的测试用例访问真实的数据库。

测试用例将需要重复运行，以确保我对产品代码所做的每一个小改动都能通过测试。 在每次运行时，测试将在数据库中创建资源和请求。 与单元测试中的常规知识不同，有时您不希望在每次运行后通过破坏测试用例创建的数据来清理战场。 到目前为止，测试和重构练习一直是一个事实调查-您可以通过尝试对旧系统进行学习。 您可能想要检查数据库中测试用例生成的数据，或者可能想要使用运行时系统中的数据，以确认所有功能均按预期工作。 这意味着测试用例必须在数据库中创建唯一实体，以避免踩其他测试用例。 还应该有一些实用程序类来轻松创建此类实体。

这是一个用于构建资源的简单构建器块：

public static ResourceBuilder newResource (String userName) {
    ResourceBuilder rb = new ResourceBuilder();
    rb.userName = userName + UnitTestThreadContext.getUniqueSuffix();
    return rb; }

public ResourceBuilder assignRole(String roleName) { 
    this.roleName = roleName + UnitTestThreadContext.getUniqueSuffix();
    return this; 
}
 public Resource create() {
    ResourceDAO resourceDAO = new ResourceDAO(UnitTestThreadContext.getSession());
    Resource rs;
     if (StringUtils.isNotBlank(userName)) {
         rs = resourceDAO.createResource(this.userName);
     } else {
         throw new RuntimeException("must have a user name to create a resource");
     }
 
     if (StringUtils.isNotBlank(roleName)) {
         Role role = RoleBuilder.newRole(roleName).create();
     rs.addRole(role);
     }
     return rs;
 }
 
 public static void delete(Resource rs, boolean cascadeToRole) {
     Session session = UnitTestThreadContext.getSession();
     ResourceDAO resourceDAO = new ResourceDAO(session);
     resourceDAO.delete(rs);
 
     if (cascadeToRole) {
         RoleDAO roleDAO = new RoleDAO(session);
     List roles = rs.getRoles();
     for (Object role : roles) {
         roleDAO.delete((Role)role);
         }
     }
 }

ResourceBuilder是Builder和Factory设计模式的实现； 您可以以“火车残骸”样式使用它：

ResourceBuilder.newResource(“Tom”).assignRole(“Developer”).create();

它还包含一个战场清理方法： delete（） 。 在此重构练习的早期，我不经常调用delete（） ，因为我经常启动系统并提取测试数据并检查条形图是否正确。

在这里非常有用的类是UnitTestThreadContext 。 它存储特定于线程的Hibernate Session对象，并提供要添加到要创建的实体名称的唯一字符串，从而确保实体的唯一性。

public class UnitTestThreadContext {
     private static ThreadLocal<Session> threadSession=new ThreadLocal<Session>();
     private static ThreadLocal<String> threadUniqueId=new ThreadLocal<String>();
     private final static SimpleDateFormat dateFormat = new SimpleDateFormat("yyyy/MM/dd HH_mm_ss_S");
     
     public static Session getSession(){>
         Session session = threadSession.get();
     if (session==null) {
         throw new RuntimeException("Hibernate Session not set!");
     }
     return session;
     }

     public static void setSession(Session session) {
               threadSession.set(session);
     }
     
     public static String getUniqueSuffix() {
         String uniqueId = threadUniqueId.get();
         if (uniqueId==null){
            uniqueId = "-"+dateFormat.format(new Date());
        threadUniqueId.set(uniqueId);
         }
         return uniqueId;        
     }

     …
 }

结语

现在，我可以启动一个最低限度运行的基础架构并运行我的简单测试用例：

protected void setUp() throws Exception {
     TestServerMain.run(); //setup a minimum running infrastructure
 }
     
 public void testResourceBreakdown(){
     Resource resource=createResource(); //use ResourceBuilder to build unique resources
     List requests=createRequests(); //use RequestBuilder to build unique requests
     assignRequestToResource(resource, requests);
     List tickets=createTickets(); //use TicketBuilder to build unique tickets
     assignTicketToResource(resource, tickets);
     Map result=new ResourceBreakdownService().search(resource);
     verifyResult(result);       
 }    
    
 protected void tearDown() throws Exception {
     // use TicketBuilder.delete() to delete tickets
     // use RequestBuilder.delete() to delete requests
     // use ResourceBuilder.delete() to delete resources      

我继续编写更复杂的测试用例，并一路重构了产品代码和测试代码。

有了这些测试用例，我一点一点地分解了God类ResourceBreakdownService 。 我不会告诉你细节。 有很多书籍可以教您如何安全地进行重构。 为了完整起见，这里是重构后的结果结构

现在，可怕的Map_Of_Array_Of_Map_Of…数据结构已组织到ResourceLoadBucket中 ，该结构使用Composite设计模式 。 它包含某个级别的估计工作量和实际工作量，下一级别的工作量可以使用aggregate（）方法汇总 。 生成的代码更清晰，性能也更好。 而且它甚至暴露了一些隐藏在原始代码复杂性中的缺陷。 当然，我一直在发展我的测试用例。

在整个练习过程中，我坚持的主要原则是大视野思维。 我坚持不懈地进行战斗，绕过对手头的任务并不重要的障碍，并建立了一个最小的可行测试基础架构，我的团队继续使用该基础架构来重构其他区域。 测试基础架构中仍然存在一些“黑客”问题，因为清理它们并没有太大的商业意义。 我不仅能够重构非常复杂的功能区域，而且还获得了有关遗留系统的更深入的知识。 将旧版应用程序视为脆弱的中国并不能为您带来更多安全，而是大胆地采用它并对其进行重构，这是旧版应用程序可以在未来生存的方式。

翻译自: https://www.infoq.com/articles/refactoring-legacy-applications/?topicPageSponsorship=c1246725-b0a7-43a6-9ef9-68102c8d48e1

重构代码的经典案例