Refactoring Legacy Code: Part 7 - Identifying the Presentation Layer

Old code. Ugly code. Complicated code. Spaghetti code. Gibberish nonsense. In two words, Legacy Code. This is a series that will help you work and deal with it.

In this seventh chapter of our refactoring tutorials, we will do a different type of refactoring. We observed in the past lessons that there is presentation related code scattered all over our legacy code. We will try to identify all the presentation related code that we can and we will then take the necessary steps to separate it from business logic.

The Driving Force

Whenever we do a refactoring change to our code, we do so guided by some principles. These principles and rules help us identify the problems and in many cases, they are pointing us in the right direction in order to make the code better.

The Single Responsibility Principle (SRP)

SRP is one of the SOLID principles we talked about in great detail in a previous tutorial: SOLID: Part 1 - The Single Responsibility Principle. If you want to dive into the details, I recommend you read the article, otherwise just continue reading on and see a summary of the Single Responsibility Principle below.

SRP basically says that any module, class, or method should have a single responsibility. Such a responsibility is defined as an axes of change. An axes of change is a direction, a reason to change. So, SRP means our class should have a single reason to change.

While that sounds pretty simple, how do you define a "reason for change"? We must think of it from the point of view of the users of our code, both ordinary end users as well as various software departments. These users can be represented as actors. When an actor wants us to change our code, that's a reason of change that determines an axis of change. Such a request should affect only one of our modules, classes, or even methods if possible.

One very obvious example would be if our UI design team would require us to provide all information that needs to be presented in a way that our application can be delivered over an HTML web page, instead of our current command line interface.

As our code stands today, we could just send all the text to some external smart object that would transform it into HTML. But that may work only because HTML is mostly text based. What if our UI team wants to present our trivia game as a desktop UI, with windows, buttons, and various tables?

What if our users want to see the game on a virtual game board represented as a city with streets, and the players as people walking around the block?

We could identify these people as the UI Actor. And we must realize that as our code stands today, we would need to modify our trivia class and almost all of its methods. Does it sound logical to modify the wasCorrectlyAnswered() method from the Game class if I want to fix a typo on the screen in a text, or if I want to present our trivia software as a virtual game board? No. The answer is absolutely not.

Clean Architecture

Clean Architecture is a concept promoted mostly by Robert C. Martin. Basically it says that our business logic should be well defined and clearly separated by boundaries from other modules not related to the core functionality of our system. This leads to decoupled and highly testable code.

You may have seen this drawing throughout my tutorials and courses. I consider it so important, that I never write code or talk about code without thinking about it. It totally changed the way we write code at Syneto, and how our project looks. Before we had all our code in an MVC framework, with business logic in the Models. This was both difficult to understand and hard to test. Plus the business logic was totally coupled to that specific MVC framework. While doing this may work with small pet-projects, when it comes to a large project on which the future of a company depends, including all its employees in a way, you must stop playing around with MVC frameworks, and you must start thinking about how to organize your code. Once you do this, and get it right, you will never want to return to the ways you architected your projects before.

Observing Belonging

We already started to separate our business logic from presentation in the previous few tutorials. We sometimes observed some printing functions and extracted them into private methods in the same Game class. This was our unconscious mind telling us to push presentation out of business logic at the method level.

Now it is time to analyze and observe.

This is the list of all variables, methods, and functions from our Game.php file. The things marked with an orange "f" are variables. The red "m" means method. If it is followed by a green lock, it is public. It it is followed by red lock it is private. And from that list, all that we are interested in, is the following part.

All the selected methods have something in common. All their names start with "display" ... something. They are all methods related to printing things on the screen. They were all identified by us in previous tutorials and seamlessly extracted, one at a time. Now we must observe that they are a group of methods that belong together. A group that does one specific thing, satisfies one single responsibility, displays information on the screen.

The Extract Class Refactoring

Best exemplified and explained in Refactoring - Improving the Design of Existing Code by Martin Fowler, the basic idea of the Extract Class refactoring is that after you realize that your class does work and that it should be done by two classes, you take actions to make two classes. There are specific mechanics to this, as explained in the quote below from the above mentioned book.

Decide how to split the responsibilities of the class.
Create a new class to express the split-off responsibilities.
- If the responsibilities of the old class no longer match its name, rename the old class.
Make a link from the old to the new class.
- You may need a two-way link. But don't make the back link until you find you need it.
Use Move Field on each field you wish to move.
Compile and test after each move.
Use Move Method to move methods over from old to new. Start with lower-level methods (called rather than calling) and build to the higher level.
Compile and test after each move.
Review and reduce the interfaces of each class.
- If you did have a two-way link, examine to see whether it can be made into a one way.
Decide whether to expose the new class. If you do expose the class, decide whether to expose it as a reference object or as an immutable value object.

Applying the Extract Class

Unfortunately at the moment of writing this article, there is no IDE in PHP that can do an extract class by just selecting a group of methods and applying an option from the menu.

As it never hurts to know the mechanics of the processes that imply working with code, we will take the steps above, one by one and apply them to our code.

Deciding How to Split the Responsibilities

We already know this. We want to break presentation from business logic. We want to take outputting, displaying functions and other code, and move them somewhere else.

Create a New Class

Our first action is to create a new, empty class.

class Display {

}

Yep. That's all for now. And finding a proper name for it was also quite easy. Display is the word all our methods we are interested in start with. It is the common denominator of their names. It is a very powerful suggestion about their common behavior, the behavior after which we named our new class.

If you prefer and your programming language supports it, PHP does, you can create the new class inside the same file as the old one. Or, you can create a new file for it from the start. I personally found no definitive reason to go either way or to forbid any of the two ways. So its up to you. Just decide and go ahead.

Linking from the Old Class to the New Class

This step may not sound very familiar. What it means is to declare a class variable in the old class and make it an instance of the new one.

require_once __DIR__ . '/Display.php';

function echoln($string) {
	echo $string . "\n";
}

class Game {
	static $minimumNumberOfPlayers = 2;
	static $numberOfCoinsToWin = 6;

	private $display;

	// ... //

	function  __construct() {

		//...//

		$this->display = new Display();
	}

	// ... all the other methods ... //
}

Simple. Isn't it? In Game's constructor we just initialized a private class variable that we named the same as the new class, display. We also needed to include the Display.php file in our Game.php file. We don't yet have an autoloader. Maybe in a future tutorial we will introduce one if needed.

And as usual, don't forget to run your tests. Unit tests are enough at this stage, just to make sure there are no typos in the newly added code.

The Move Field and Compile / Test

Let's take these two steps at once. What fields can we identify that should go from Game to Display?

By just looking at the list...

static $minimumNumberOfPlayers = 2;
static $numberOfCoinsToWin = 6;

private $display;

var $players;
var $places;
var $purses;
var $inPenaltyBox;

var $popQuestions;
var $scienceQuestions;
var $sportsQuestions;
var $rockQuestions;

var $currentPlayer = 0;
var $isGettingOutOfPenaltyBox;

... we can't find any variable / field that must belong to Display. Maybe some will emerge in time. So nothing to do for this step. And about the tests, we already ran them a moment ago. Time to move on.

Move Methods to the New Class

This is in itself, another refactoring. You can do it in several ways and you will find a nice definition of it in the same book that we talked about earlier.

As mentioned above, we should start with the lowermost level of methods. Those that are not calling other methods. Instead they are called.

private function displayPlayersNewLocation() {
	echoln($this->players[$this->currentPlayer] . "'s new location is " . $this->places[$this->currentPlayer]);
}

displayPlayersNewLocation() seems to be a good candidate. Let's analyze what it does.

We can see that it does not call other methods on Game. Instead, it uses three fields: players, currentPlayer, and places. Those can turn into two or three parameters. So far pretty nice. But what about echoln(), the only function call in our method? Where is this echoln() coming from?

It is at the top of our Game.php file, outside of the Game class itself.

function echoln($string) {
	echo $string . "\n";
}

It definitely does what it says. Echoes a string with a new line at the end. And this is pure presentation. It should go into the Display class. So let's copy it over there.

class Display {

	function echoln($string) {
		echo $string . "\n";
	}

}

Run our tests again. We can keep the golden master disabled until we finish extracting all the presentation to the new Display class. At any time, if you feel that output may have been modified, re-run the golden master tests also. At this point, the tests will attest that we did not introduce typos or duplicate function declarations, or any other errors for that matter, by copying the function to its new place.

Now, go and delete echoln() from the Game.php file, run our tests, and expect them to fail.

PHP Fatal error:  Call to undefined function echoln() in /.../Game.php on line 55

Nice! Our unit test is of great help here. It runs very fast and it tells us the exact position of the problem. We go to line 55.

Look! There is an echoln() call there. Tests never lie. Let's fix it by calling $this->dipslay->echoln() instead.

function add($playerName) {
	array_push($this->players, $playerName);
	$this->setDefaultPlayerParametersFor($this->howManyPlayers());

	$this->display->echoln($playerName . " was added");
	echoln("They are player number " . count($this->players));
	return true;
}

That makes the test pass through line 55 and fail on 56.

PHP Fatal error:  Call to undefined function echoln() in /.../Game.php on line 56

And the solution is obvious. This is a tedious process, but it is at least easy.

function add($playerName) {
	array_push($this->players, $playerName);
	$this->setDefaultPlayerParametersFor($this->howManyPlayers());

	$this->display->echoln($playerName . " was added");
	$this->display->echoln("They are player number " . count($this->players));
	return true;
}

That actually makes the first three tests pass and also tells us the next place where there is a call which we should change.

PHP Fatal error:  Call to undefined function echoln() in /.../Game.php on line 169

That is in wrongAnswer().

function wrongAnswer() {
	echoln("Question was incorrectly answered");
	echoln($this->players[$this->currentPlayer] . " was sent to the penalty box");
	$this->inPenaltyBox[$this->currentPlayer] = true;

	$this->currentPlayer++;
	if ($this->shouldResetCurrentPlayer()) {
		$this->currentPlayer = 0;
	}
	return true;
}

Fixing these two calls, pushes our error down to line 228.

private function displayCurrentPlayer() {
	echoln($this->players[$this->currentPlayer] . " is the current player");
}

A display method! Maybe this should be our first method to move. We try to do a little test driven development (TDD) here. And when the tests fail, we are not allowed to write any more production code that isn't absolutely necessary to make the test pass. And all that entails is just changing the echoln() calls until all our unit tests pass.

You can speed up this process by using your IDE's or editor's search and replace functionality. Just run all the tests, including the golden master after you are done with this replacement. Our unit tests do not cover all the code, and all the echoln() calls.

We can start with out first candidate, displayCurrentPlayer(). Copy it over to Display and run your tests.

Then, make it public on Display and in displayCurrentPlayer() in Game call $this->display->displayCurrentPlayer() instead of directly doing an echoln(). Finally, run your tests.

They will fail. But by doing the change this way, we've ensured that we changed only one thing that could fail. All other methods are still calling Game's displayCurrentPlayer(). And this is the one delegating to Display.

 Undefined property: Display::$display

Our method uses class fields. These need to be made parameters to the function. If you follow your test errors, you should end up with something like this in Game.

private function displayCurrentPlayer() {
	$this->display->displayCurrentPlayer($this->players[$this->currentPlayer]);
}

And this in Display.

function displayCurrentPlayer($currentPlayer) {
	$this->echoln($currentPlayer . " is the current player");
}

Replace calls in Game to the local method with the one in Display. Don't forget about moving the parameters up one level, also.

private function displayStatusAfterRoll($rolledNumber) {
	$this->display->displayCurrentPlayer($this->players[$this->currentPlayer]);
	$this->displayRolledNumber($rolledNumber);
}

Finally, remove the unused method from Game. And run your tests to make sure all is OK.

This is a tedious process. You can speed it up a little bit by taking several methods at once and using whatever your IDE can do to help move and replace code between classes. The rest of the methods will remain an exercise for you or you can read on more this chapter with the highlights of the process. The finished code attached to this article will contain the complete Display class.

Ah, and don't forget about the code that is not yet extracted in the "display" methods inside Game. You may move those echoln() calls to display directly. Our goal is to not call echoln() at all from Game, and make it private on Display.

After just a half an hour or so of work, Display starts to look nice.

All the display methods from Game are in Display. Now we can look for all echoln calls that remained in Game and move them, too. Tests are passing, of course.

But as soon as we are faced with the askQuestion() method, we realize it is just presentation code as well. And that means that the various question arrays should also go to Display.

class Display {
	private $popQuestions = [];
	private $scienceQuestions = [];
	private $sportsQuestions = [];
	private $rockQuestions = [];

	function __construct() {
		$this->initializeQuestions();
	}
	// ... //
	private function initializeQuestions() {
		$categorySize = 50;
		for ($i = 0; $i < $categorySize; $i++) {
			array_push($this->popQuestions, "Pop Question " . $i);
			array_push($this->scienceQuestions, ("Science Question " . $i));
			array_push($this->sportsQuestions, ("Sports Question " . $i));
			array_push($this->rockQuestions, "Rock Question " . $i);
		}
	}
}

That looks fitting. Questions are just strings, we present them and they fit better here. When we do this kind of refactoring, it is also a good opportunity to refactor the newly moved code. We defined initial values in the declaration of fields, we also made them private, and created a method with the code that needs to be executed so that it is not just lingering in the constructor. Instead, it is hidden at the bottom of the class, out of the way.

After extracting the next two methods, we realize that it is nicer to name them, inside the Display class, without the "display" prefix.

function correctAnswer() {
	$this->echoln("Answer was correct!!!!");
}

function playerCoins($currentPlayer, $playerCoins) {
	$this->echoln($currentPlayer . " now has " . $playerCoins . " Gold Coins.");
}

With our tests green and doing well, we can now refactor and rename our methods. PHPStorm can handle the rename refactorings quite well. It will rename function calls in Game accordingly. Then there is this piece of code.

Look carefully at the selected line, 119. That looks just like our recently extracted method in Display.

function correctAnswer() {
	$this->echoln("Answer was correct!!!!");
}

But if we call it instead of the code, the test will fail. Yes! There is a typo. And NO! You should not fix it. We are refactoring. We must keep the functionality unchanged, even if there is a bug.

The rest of the method represents no special challange.

Review and Reduce the Interfaces

Now that all presentation functionality is in Display, we must review the methods and keep public only the ones used in Game. This step is also motivated by the Interface Segregation Principle that we talked about in a past tutorial.

In our case, the easiest way to figure out what methods need to be public or private, is to just make each one private at a time, run the tests, and if they fail revert to public.

Because golden master tests run slow, we can also rely on our IDE to help us speed up the process. PHPStorm is smart enough to figure out if a method is unused. If we make a method private, and it suddenly becomes unused, it is clear it was used outside of Display and needs to remain public.

Finally, we can reorganize Display so that private methods are at the end of the class.

Final Thoughts

Now, the last step of the Extract Class Refactoring principle is irrelevant in our case. So with that, this concludes the tutorial, but this does not yet conclude the series. Stay tuned for our next article where we will work further toward a Clean Architecture and invert dependencies.

HIGHLIGHTS OF THE DAY