Refactoring learning cases for legacy programs

Legacy code is often rotten, and every good developer wants to refactor it. An ideal prerequisite for refactoring is that it should contain a set of unit test cases to avoid regression defects. But it's not easy to write unit tests for legacy code, because it's often a mess. To write a valid unit test for a legacy code, you probably have to refactor it first. But to refactor it, you need unit testing to make sure you don't break any functionality. This situation is equivalent to whether the answer is chicken or egg first. This article describes a way to safely refactor legacy code by sharing a real case that I've been involved in.

Problem description

In this article, I will use a real case to describe the effective practice of testing and refactoring legacy systems. The code for this example is written in Java, but this practice applies to other languages as well. I made some minor changes in the original scene to avoid accidentally killing innocent people and simplifying them to make them easier to understand. The practice described in this article has helped me refactor a legacy system I've been involved in recently.

This article is not intended to introduce the basic techniques of unit testing and refactoring. You can learn more about this topic by reading related books, such as Martin Fowler's refactoring: improving the design of existing code and Joshua Kerievsky's refactoring and patterns. In contrast, the content of this article will describe some of the complexities of real-world scenarios, and I hope it can provide some useful practice for addressing these complexities.

In this case I will describe a fictitious resource management system in which resources refer to someone who can be assigned to their task. You can assign an HR ticket (ticket) or it ticket to a resource, or you can assign an HR request or it request to a resource. A resource manager can record the estimated time that a resource is working on a task, and the resource itself can record the actual time that they work on a ticket or request.

You can represent the use of a resource in a pie chart, showing both the estimated time and the actual time spent.

Doesn't that seem too complicated? However, a real system can assign multiple types of tasks to a resource, but technically this is not a very complicated design. But when I first saw the code for the system, I felt like I was seeing an old relic of how the code evolved (or degraded) from the beginning. At the outset, the system can only be used to process requests before it is added to the function of handling bills and other types of tasks. An engineer starts writing code to process the request: first get the data from the database, and then display the data in a pie chart format. He didn't even think about organizing information as an appropriate object:

Class Resourcebreakdownservice {public
    Map Search (Session context) throws searchexception{   

        //omitted twenty or So lines of code to pull search criteria out 
of 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 to all requests

        Map Requestbreakdown=getresourcerequestsloadbreakdown (resourceobjs,startdate,
finishdate);
        Return Requestbreakdown
   }
}

I'm sure you're scared by the bad smell in this code, right? For example, you might soon find that search is not a meaningful name, and you should use the collectionutils.isempty () method in the Apache Commons Class library to detect a collection. In addition, you probably wonder what the map object that the method returns contains.

Don't worry, the bad smell is coming. The next engineer inherited the mantle of the ancestors and handled the bill in the same way, following the modified code:

Get workload to 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
;

Regardless of the bad naming, the unbalanced code structure, and any other problems with the aesthetic dimensions of the code. The worst flavor of the code is the map object It returns, and the map object is a black hole full of data, but it doesn't prompt you what it contains. I can only write some debugging code, the content of the map in the loop printed out to understand its data structure.

In this example,{} represents a map,=> represents a health value mapping, and [] represents a collection:

{Resource with ID 30000=> [
    summaryofactualworkloadforrequesttype,
    Summaryofestimatedworkloadforrequesttype,
    {30240=>[
             actualworkloadforreqeustwithid_30240,
             ESTIMATEDWORKLOADFORREQUESTWITHID_30240],                  
             30241=>[
                 actualworkloadforreqeustwithid_30241,
                 ESTIMATEDWORKLOADFORREQUESTWITHID_30241]
     }
     summaryofactualworkloadfortickettype,
     Summaryofestimatedworkloadfortickettype,
     {20000=>[
             actualworkloadforticketwithid_2000,
             ESTIMATEDWORKLOADFORTICKETWITHID_2000],
     }                 
     ]
}

This poor data structure makes the coding and decoding logic of a dataset visually tedious and very readable.

Integration Testing

Hopefully I've made you realize that this code is really complicated. If I had to unravel this mess before I started refactoring and then understand the intent of each line of code, I'd be crazy. To keep my sanity, I decided to take a top-down approach to understanding code logic. In other words, I decided to try the system's function first, to do some debugging to understand the overall situation of the system, instead of reading the code directly and trying to infer the logic of the code from it.

The method I use is exactly the same as writing the test code, and the traditional approach is to write small snippets of test code to validate each piece of code path, and if every test passes, then when all the code paths are organized together, the chances are high that the method will work as expected. But this traditional approach does not work here, Resourcebreakdownservice is simply a "God class", and if I break this class with some understanding of the whole system, it is likely to cause many problems- In every nook and cranny of a legacy system, there may be many hidden secrets.

I wrote the following simple test, which reflects my understanding of the whole system:

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);
}

Notice the verifyresult () method, I first print out the contents of result in a loop to find the structure, and then the Verifyresult () method validates the results based on the structure. Make sure that it contains the correct data: