My First Refactor – Part 2

Continuing from my previous post, I’ve created a characterisation test for my Stock Analysis application. I captured the HTML output it produced and wrote a simple test which ensured that it always produced the same output. I used PHPUnit as the testing framework and used composer.js for dependency management.

Creating the Characterisation Test

Part of my initial code produced a raw HTML <table>  with the  results of the investment strategy. You can see what this looks like from this screenshot:

Dollar Cost Averager Screenshot
Dollar Cost Averager Screenshot

I restructured the code from being all in one index.php file to being in two files engine.php and index.php. In doing so I broke up the code into two partitions:

Application Partitioning
Application Partitioning

I moved all the logic which interacted with the database and which produced the <table> into the engine.php and kept the <form> and application title elements in the index.php. 

This is good design because now the Application partition can focus on processing the data and the Main partition can focus on displaying the data to the user and interacting with the user.  At a high level at least this application structure now follows the Single Responsibility Principle. Meaning that each part of the application should do one and only one thing. So now, my index.php looks like this:

I hope you’ll agree that this looks much simpler than before and easier to read. This restructuring also enables me to capture the output of the new calculateReturnOnInvestment()  method. This method returns the full HTML table element. So I can run it from the php interactive console and then copy and paste the output into a new file called golden_output.php. This file contains a method called getGoldenOutput() which returns a string with the “golden” HTML which I will use for comparison.

Finally I write the test. The test itself is a comparison of the golden output and what the calculateReturnOnInvestment() method currently produces. You can see the test here:


I run my test using PHPUnit’s CLI interface:

Running this test now  instantly informs me if I change the current behaviour of the code. So I can focus entirely on performing the refactor without worrying that I’ve broken anything.

Refactoring the Application Partition

So far, to get my application under test I’ve just copied the hard to read blob of code into a single function. I need to break this function up so that the code is more understandable and easy to change. I turned to Martin Fowler’s book ” Refactoring – Improving the Design of Existing Code” to learn about the different mechanisms for refactoring.

Martin Fowler - Refactoring Improving the Design of Existing Code
Refactoring Improving the Design of Existing Code

 

Mr. Fowler provides a catalogue of refactoring techniques which I’ve tried to employ during my refactoring efforts. In this section, I highlight examples of how I used these techniques. To start, lets have a look at the calculateReturnOnInvestment() function:

The first thing you might notice about this code is that it’s hard to follow. So how to improve this code? As I mentioned in a previous blog post, Robert C. Martin suggests that “classes hide in long functions”. You can see that the function has lots of temporary variables and various blocks of logic all using those variables. This sounds like those blocks could be methods for the class and the temporary variables could be members. So in this case changing this function into a class sounds like an ideal first step.

Refactoring Technique: Extract Class

To do this, I create a new class called ReturnOnInvestmentTablePrinter and move the logic into it. While it might seem that I’m just bloating the system with more code,  creating this class now enables me to employ more refactoring techniques. I’m now able to split out this method into different code blocks.

Using this technique, I split up the code into header, rows and footer generation methods. Now the generateReturnOnInvestmentTable() method is bit easier to read:

 

The getTableRowsHTML() method is still pretty long and complicated. There are lots of different code blocks and temporary variables within it. It looks like I should perform another Extract Class. This would separate the table printing code from the strategy simulation code. I create a new class called DollarCostAverageStrategy with a getInvestmentResults() method and move the code from getTableRowsHTML() into it.

DollarCostAverageStrategy::getInvestmentResults() will return an array with the the $monthly_data which the table printer class can print neatly into a <table> element. Now the getTableRowsHTML() method looks like this:

Refactoring Technique: Remove Temps

My current goal is to split up the big getInvestmentResults()  method into smaller, more easily understood methods. However there are lots of temporary variables which are needed by all the different code blocks in this method. This is preventing me from breaking this long hard-to-understand method up. To get around this I need to remove these temporary variables. I have a number of options – I can promote a variables to be a member of the class so that it will be accessible from all methods. Alternatively I can replace a temporary variable with a query method and finally I can inline a temporary variable where it is used into the code.

I usually take one temporary variable at a time and perform the appropriate technique. Once complete, the code now looks like this:

Notice how the readability of the code has improved, especially in the: getInvestmentResults(), processStockPriceData() and calculateInvestmentResults() methods.

Dependency Injection

You may have noticed that I now pass the stock price data array into the Strategy and pass the Strategy object into the Table Printer. PHP isn’t a strongly typed language so it might not be fully clear that I’m trying to ensure that the Table Printer doesn’t rely on the Dollar Cost Averaging Strategy, instead it relies on an object which can getInvestmentResults(). The table printer shouldn’t know how the investment results are generated – it just knows that it will be passed an object from which it can getInvestmentResults().

There is still some coupling between the table printer and the strategy – the printer still knows about the table headers and about all the type of results available in the $monthly_data array. This can be improved later but at least we’re moving towards a loosely coupled design.

Changing the Behaviour

Up until now all I’ve cared about is improving the design. The Characterisation Test helped me do this. Now that I want to change the implementation to do something different. I can’t use a catch-all test anymore because this will break as soon as I change how anything behaves.

To do this, I need to write some more finely grained tests so I can ensure that my existing behaviour will continue to work while I add more functionality.

 

 

 

Leave a comment

Your email address will not be published.

This site uses Akismet to reduce spam. Learn how your comment data is processed.