Let’s Deal with Legacy Code
This article is in continuation with the previous article where we defined some of the key aspects of Legacy Code. In this article we will take a Legacy code and add a new feature to it.
Before we begin with an example, let’s take a moment to understand Broken Window Theory.
Broken Window Theory
An academic theory proposed by James Q. Wilson and George Kelling in 1982 that used broken windows as a metaphor for disorder within neighbourhoods.
One broken window, if left unrepaired for a substantial amount of time, instills a sense of abandonment. So another window gets broken. People start littering. Graffiti appears. Serious structural damage begins. In a relatively short time, the building becomes damaged beyond the owner’s desire to fix it, and the sense of abandonment becomes reality.
Let’s not abandon our code, let’s repair the code as soon as we get an opportunity to repair it and let’s not get ourselves into a situation where damage is beyond our capacity to fix. Let’s see our theory in action.
Problem Definition Overview
The below code belongs to a hypothetical application “Movie Rental” which allows its customers to rent either a Regular or a Children’s movies for fixed number of days. The application also allows generation of a statement which the business calls as “Text Statement”. This application has been running in Production for a long time without issues and has become very popular. Now business wants to generate an HTML statement with exact same logic for amount computation.
The team decides to discuss different ways to handle this new requirement in legacy code.
And the team agrees to improve the code before implementing the new functionality. Scott and Jessica will be pairing on this. But, where do they start from? As mentioned in their discussion, they need to understand the code first so they decide to write Characterization Test(s).
First Characterization Test
Scott> How many tests should we write?
Jessica> Let’s look at the code. It should give us some hints.
Scott> I get it. We need a few rentals consisting of Regular and Children’s movie and the number of days rented for which these movies can be greater than 2 or 3. So, one test should cover a decent functionality.
Jessica> I can’t agree more. So let’s write it then.
Scott> Let’s run this and see it fail.
org.junit.ComparisonFailure:
Expected: ""
Actual:
Rental Record for John
Black Panther 3.5
Lion King 3.0
Amount owed is 6.5Jessica> Great. We have made some progress. Let’s correct our test.
Jessica and Scott agree to write one test case covering a decent portion of the code. If this gives us confidence, we can live with one test for now else we can write a few more or include movies with daysRented < 2.
Scott> Jessica, what type of test should a Characterization test be? Unit, Functional, Integration?
Jessica> Scott, it is not always possible to write unit or functional tests for legacy code. You might end up writing an integration test to begin with because you just want to know what system does. But, as soon as you get an opportunity, get your tests closer to the code.
Scott> Sure Jessica, let’s start the fun part. Let’s fix a broken window.
Refactoring
Scott> Where do we start from?
Jessica> I believe statement() method is a long method. We should try and make it a little shorter.
Scott> Agreed.
Jessica and Scott agreed that statement() method is a long method. But, this agreement was not based on the number of lines in the method. It was based on how easy it is to comprehend the method or is a method doing more than one thing at a time or can it be decomposed further.
Jessica> Switch statement has gone out and the extracted amount() method does one thing which is getting amount for a given rental.
Scott> Let’s continue refactoring. I am in a mood to clean up everything.
Jessica> Hold on Scott, we need to run tests before we move on.
And the test ran successfully.
While working with Legacy Code it is important to take smaller steps and follow refactoring cycle. Refactor -> Run Tests -> Refactor
Scott> Sure. Jessica, are we in a position to remove the comment “determine amounts for each line” from previous code?
Jessica> Yes, we can remove it.
Remove comments from legacy code when you have captured their complete essence . Though I did take some liberty to rename variable along with removing comment, it is always ideal to take smaller steps when you are beginning to understand legacy code. As you grow in confidence, you might want to take bigger steps but one test failure and the reality reveals itself.
Scott> Let’s look at amount() method. It depends on priceCode from movie but is placed in Customer. We should move this method to the place where it belongs.
Jessica> Yes, let’s do a few method movements (in the interest of this article).
I did a few movements. Moved amount() method to Rental and then to Movie and ran the tests. It should be noted that this is our first opportunity to write unit tests for Rental and Movie. I won’t, for this article, but I assume you will.
Scott> Jessica, I have a question. Movie has a switch statement based on different types of movies. Shall we introduce some polymorphism here?
Jessica> I don’t think it is coming in our way of implementing HTML statement functionality.
Scott has raised a valid point but we need to remember one thing, “we refactor the code which comes in our way”. At this point in time, we need to implement HTML statement and switch code does not come in the way of our new feature, neither do the magic numbers 2 or 1.5. If you want to continue with small refactorings which are not coming in your way, say changing Magic Numbers to Constants, go ahead and do it but do not move away from your actual task: implementing HTML statement.
Scott> I get that. Thank you. The statement() method in Customer is short enough. Shall we pause our refactoring here?
Jessica> We could, but one thing that is bothering me is this method seems to be generating 3 parts of the statement and I can see it clearly — header, body and footer. If the effort is not huge we should try and extract this code into different methods.
Scott> You clearly have an eye for refactoring. Let’s do it.
I cheated again. Did lot more than what I should have done,renamed methods to be text*, duplicated for loops (over rentals) to calculate totalAmount(), repeated the same in textBody().
Is that justified? Well, how many rentals do we expect to have for a customer? What is the cost of iterating over them twice? If it is not significant, go ahead and use it. What does it give me? Look at the statement() method now.
Jessica> Now, we are done with refactoring. We can introduce HTML statement functionality now.
Conclusion
Jessica and Scott went on to implement HTML functionality (with tests) and they did a lot to clean up the existing code. This is much more understandable that it used to be.
They might not have cleaned up everything but they clearly have left a great deal of understanding trace for others to follow.
They followed Cover and Modify, Boy Scout rule, Refactoring Cycle and refactored enough to finish the new functionality, in-short dealt with Legacy Code professionally.
References
- Refactoring — Improving The Design Of Existing Code
- Refactoring Catalog
