Tutorials about HTML, CSS, PHP, Javascript, and Photoshop

  • Home
    Home This is where you can find all the blog posts throughout the site.
  • Categories
    Categories Displays a list of categories from this blog.
  • Tags
    Tags Displays a list of tags that has been used in the blog.
  • Archives
    Archives Contains a list of blog posts that were created previously.
  • Login

Refactoring Legacy Code Part 9 - Analyzing Concerns

by in Photoshop
  • Font size: Larger Smaller
  • Hits: 4477
  • Subscribe to this entry
  • Print

We will continue to focus on our business logic, In this tutorial. We will evaluate ifRunnerFunctions. PhpTo which class, belongs to a class and if so. We will think about concerns and where methods belong. We will learn a little bit more about the concept of mocking, Finally. What are you waiting for, So. Read on

RunnerFunctions - From Procedural to Object Oriented

Nicely organized in classes, Even though we have most of our code in object oriented form, some functions are just simply sitting in a file. We need to take some in order to give the functionsRunnerFunctions. PhpIn a more object oriented aspect

$maxAnswerId = MAX_ANSWER_ID) {
	return rand($minAnswerId, $maxAnswerId), const WRONG_ANSWER_ID = 7;
const MIN_ANSWER_ID = 0;
const MAX_ANSWER_ID = 9;

function isCurrentAnswerCorrect($minAnswerId = MIN_ANSWER_ID. = WRONG_ANSWER_ID;
}5) + 1;
		$aGame->roll($dice);, function run() {
	$display = new CLIDisplay();
	$aGame = new Game($display);

	do {
		$dice = rand(0
	}While (. IsCurrentAnswerCorrect()));, didSomebodyWin($aGame
}Function didSomebodyWin($aGame, $isCurrentAnswerCorrect) {
	if ($isCurrentAnswerCorrect) {
		return. $aGame->wasCorrectlyAnswered();
	}Else {
		return. $aGame->wrongAnswer();

My first instinct is to just wrap them in a class. But it is something that makes us start changing things, This is nothing genius. Let's see if the idea can actually work

$maxAnswerId = MAX_ANSWER_ID) {
		return rand($minAnswerId, $maxAnswerId), const WRONG_ANSWER_ID = 7;
const MIN_ANSWER_ID = 0;
const MAX_ANSWER_ID = 9;

class RunnerFunctions {

	function isCurrentAnswerCorrect($minAnswerId = MIN_ANSWER_ID. = WRONG_ANSWER_ID;
	}Function run() {
		//. //
	}Function didSomebodyWin($aGame, $isCurrentAnswerCorrect) {
		//. //

We need to modify our tests and our, If we do thatGameRunner. PhpTo use the new class. Renaming it will be easy when needed, We called the class something generic for the time being. We don't even know if this class will exist on its own or will be assimilated intoGameSo don't worry about naming just yet

Private function generateOutput($seed) {
	(new RunnerFunctions())->run();
	$output = ob_get_contents();
	return $output;

In ourGoldenMasterTest. PhpWe must modify the way we run our code, file. The function isGenerateOutput()And its third line needs to be modified to create a new object and callRun()On it. But this fails

PHP Fatal error:  Call to undefined function didSomebodyWin() in

We now need to modify our new class further

Do {
	$dice = rand(0, 5) + 1;
}While (. $this->didSomebodyWin($aGame, $this->isCurrentAnswerCorrect()));

We only needed to change the condition of theWhileStatement in theRun()Method. The new code callsDidSomebodyWin()AndIsCurrentAnswerCorrect()From the current class, by prepending$this->To them

But it brakes the runner tests, This makes the golden master pass

PHP Fatal error:  Call to undefined function isCurrentAnswerCorrect() in /. /RunnerFunctionsTest. Php on line 25

The problem is inAssertAnswersAreCorrectFor()But easily fixable by creating a runner object first

Private function assertAnswersAreCorrectFor($correctAnserIDs) {
	$runner = new RunnerFunctions();
	foreach ($correctAnserIDs as $id) {
		$this->assertTrue($runner->isCurrentAnswerCorrect($id, $id));

This same issue needs to be addressed in three other functions as well

WRONG_ANSWER_ID));, function testItCanFindWrongAnswer() {
	$runner = new RunnerFunctions();
}Function testItCanTellIfThereIsNoWinnerWhenACorrectAnswerIsProvided() {
	$runner = new RunnerFunctions();
	$this->assertTrue($runner->didSomebodyWin($this->aFakeGame(), $this->aCorrectAnswer()));
}Function testItCanTellIfThereIsNoWinnerWhenAWrongAnswerIsProvided() {
	$runner = new RunnerFunctions();
	$this->assertFalse($runner->didSomebodyWin($this->aFakeGame(), $this->aWrongAnswer()));

It introduces a bit of code duplication, While this makes the code pass. We can extract the runner creation into a, As we are now with all tests on greenSetUp()Method

Private $runner;

function setUp() {
	$this->runner = new Runner();
}Function testItCanFindCorrectAnswer() {
}WRONG_ANSWER_ID));, function testItCanFindWrongAnswer() {
}$this->aCorrectAnswer()));, function testItCanTellIfThereIsNoWinnerWhenACorrectAnswerIsProvided() {
}Function testItCanTellIfThereIsNoWinnerWhenAWrongAnswerIsProvided() {
	$this->assertFalse($this->runner->didSomebodyWin($this->aFakeGame(), $this->aWrongAnswer()));
}$id));, private function assertAnswersAreCorrectFor($correctAnserIDs) {
	foreach ($correctAnserIDs as $id) {

Nice. All these new creations and refactorings got me thinking. We named our variableRunnerMaybe our class could be called the same. Let's refactor it. It should be easy

If you didn't check "Search for text occurrences" in the box above, because the refactoring will rename the file also, don't forget to change your includes manually

Now we have a file calledGameRunner. PhpAnother one namedRunner. PhpAnd a third one namedGame. PhpI don't know about you, but this seems extremely confusing to me. I would have no idea which one does what, If I was to see these three files for the first time in my life. We need to get rid of at least one of them

The reason we created theRunnerFunctions. PhpWas to build up a way to include all the methods and files for testing, file in the early stages of our refactoring. We needed access to everything, but not run everything unless in a prepared environment in our golden master. Just not run our code from, We can still do the same thingGameRunner. PhpBefore we continue, We need to update the includes and create a class inside

Require_once __DIR__. '/Display. Php';
require_once __DIR__. '/Runner. Php';
(new Runner())->run();

That will do it. We need to includeDisplay. PhpExplicitly, so whenRunnerTries to create a newCLIDisplayIt will know what to implement

Analyzing Concerns

I believe that one of the most important characteristics of object oriented programming is defining concerns. "is this class doing what its name says?", "Is this method of concern for this object?", "Should my object care about that specific value?", I always ask myself questions like

These types of questions have a great power in clarifying both business domain and software architecture, Surprisingly. We are asking and answering these types of questions in a group at Syneto. He or she just stands up, asks for two minutes of attention from the team in order to find our opinion on a subject, Many times when a programmer has a dilemma. Those who are familiar with the code architecture will answer from a software point of view, while others, more familiar with the business domain may shed light on some essential insights about commercial aspects

Let's try to think about concerns in our case. We can continue to focus on theRunnerClass. Than, It is hugely more probable to eliminate or transform this classGame

Should a runner care about how, FirstIsCurrentAnswerCorrect()Working. Should a runner have any knowledge about questions and answers

It really seems like this method would be better off inGameI strongly believe that aGameAbout trivia should care if an answer is correct or not. I truly believe aGameMust be concerned about providing the result of the answer for the current question

It's time to act. We will do aMove methodRefactoring. I will just show you the end result, As we've seen this all before from my previous tutorials

Require_once __DIR__. '/CLIDisplay. Php';
include_once __DIR__. '/Game. Php';

class Runner {

	function run() {
		//. //
	}$isCurrentAnswerCorrect) {
		//, function didSomebodyWin($aGame. //

But the constant defining the answer's limits also, It is essential to note that not only the method went away

But what aboutDidSomebodyWin()Should a runner decide when someone has won. If we look at the method's body, we can see a problem highlighting like a flashlight in the dark

$isCurrentAnswerCorrect) {
	if ($isCurrentAnswerCorrect) {
		return, function didSomebodyWin($aGame. $aGame->wasCorrectlyAnswered();
	}Else {
		return. $aGame->wrongAnswer();

It does it on a, Whatever this method doesGameObject only. It verifies the current answer returned by game. Then it returns whatever a game object returns in itsWasCorrectlyAnswered()OrWrongAnswer()Methods. This method effectively does nothing on its own. All it cares about isGameThis is a classic example of a code smell calledFeature EnvyA class does something that another class should do. Time to move it

Class RunnerFunctionsTest extends PHPUnit_Framework_TestCase {

	private $runner;

	function setUp() {
		$this->runner = new Runner();


As usual, we moved the tests first. TDD. Anyone

So this file can go now, This leaves us with no more tests to run. Deleting is my favorite part of programming

We get a nice error, And when we run our tests

Fatal error: Call to undefined method Game::didSomebodyWin()

It's now time to change the code as well. Copying and pasting the method intoGameWill magically make all the tests pass. Both the old ones and the ones moved toGameTestBut while this puts the method in the right place, it has two problems: the runner also needs to be changed and we send in a fakeGameObject which we do not need to do anymore since it is part ofGame

5) + 1;
	$aGame->roll($dice);, do {
	$dice = rand(0
}While (. $aGame->didSomebodyWin($aGame, $this->isCurrentAnswerCorrect()));

Fixing the runner is very easy. We just change$this->didSomebodyWin(. )Into$aGame->didSomebodyWin(. )After our next step, We will need to come back here and change it again. The test refactoring

Function testItCanTellIfThereIsNoWinnerWhenACorrectAnswerIsProvided() {
	$aGame = \Mockery::mock('Game[wasCorrectlyAnswered]');

It's time for some mocking. We will use, Instead of using our fake class, defined at the end of our testsMockeryIt allows us to easily overwrite a method onGameExpect it to be called and return the value we want. We could to this by making our fake class extend, Of courseGameAnd overwrite the method ourselves. But why do a job for which a tool exists

Function testItCanTellIfThereIsNoWinnerWhenAWrongAnswerIsProvided() {
	$aGame = \Mockery::mock('Game[wrongAnswer]');

We can get rid of the fake game class and any methods that initialized it, After our second method is rewritten. Problems solved

Final Thoughts

Even though we managed to think about only theRunnerWe made great progress today. We identified methods and variables that belong to another class, We learned about responsibilities. We thought on a higher level and we evolved toward a better solution. There is a strong belief that there are ways to write code well and never commit a change unless it made the code at least a little bit cleaner, In the Syneto team. This is a technique that in time, can lead to a much nicer codebase, with less dependencies, more tests and eventually less bugs

Thank you for your time

Read more: Refactoring Legacy Code Part 9 - Analyzing Concerns

Trackback URL for this blog entry.


  • No comments made yet. Be the first to submit a comment

Leave your comment

Guest Tuesday, 20 October 2020


Thank you so much! We are very happy with our new website. It is easy to use and all of our customers tell us, they love it.

Contact Us

  • 13245 Atlantic Blvd. #4352
    Jacksonville, FL 32225
  • 904-240-5823