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

RunnerFunctions - From Procedural to Object Oriented

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

const WRONG_ANSWER_ID = 7;
const MIN_ANSWER_ID = 0;
const MAX_ANSWER_ID = 9;

function isCurrentAnswerCorrect($minAnswerId = MIN_ANSWER_ID, $maxAnswerId = MAX_ANSWER_ID) {
	return rand($minAnswerId, $maxAnswerId) != WRONG_ANSWER_ID;
}

function run() {
	$display = new CLIDisplay();
	$aGame = new Game($display);
	$aGame->add("Chet");
	$aGame->add("Pat");
	$aGame->add("Sue");

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

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

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

const WRONG_ANSWER_ID = 7;
const MIN_ANSWER_ID = 0;
const MAX_ANSWER_ID = 9;

class RunnerFunctions {

	function isCurrentAnswerCorrect($minAnswerId = MIN_ANSWER_ID, $maxAnswerId = MAX_ANSWER_ID) {
		return rand($minAnswerId, $maxAnswerId) != WRONG_ANSWER_ID;
	}

	function run() {
		// ... //
	}

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

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

private function generateOutput($seed) {
	ob_start();
	srand($seed);
	(new RunnerFunctions())->run();
	$output = ob_get_contents();
	ob_end_clean();
	return $output;
}

In our GoldenMasterTest.php file, we must modify the way we run our code. The function is generateOutput() and its third line needs to be modified to create a new object and call run() 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;
	$aGame->roll($dice);
} while (!$this->didSomebodyWin($aGame, $this->isCurrentAnswerCorrect()));

We only needed to change the condition of the while statement in the run() method. The new code calls didSomebodyWin() and isCurrentAnswerCorrect() from the current class, by prepending $this-> to them.

This makes the golden master pass, but it brakes the runner tests.

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

The problem is in assertAnswersAreCorrectFor(), 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.

function testItCanFindWrongAnswer() {
	$runner = new RunnerFunctions();
	$this->assertFalse($runner->isCurrentAnswerCorrect(WRONG_ANSWER_ID, WRONG_ANSWER_ID));
}

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

While this makes the code pass, it introduces a bit of code duplication. As we are now with all tests on green, we can extract the runner creation into a setUp() method.

private $runner;

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

function testItCanFindCorrectAnswer() {
	$this->assertAnswersAreCorrectFor($this->getCorrectAnswerIDs());
}

function testItCanFindWrongAnswer() {
	$this->assertFalse($this->runner->isCurrentAnswerCorrect(WRONG_ANSWER_ID, WRONG_ANSWER_ID));
}

function testItCanTellIfThereIsNoWinnerWhenACorrectAnswerIsProvided() {
	$this->assertTrue($this->runner->didSomebodyWin($this->aFakeGame(), $this->aCorrectAnswer()));
}

function testItCanTellIfThereIsNoWinnerWhenAWrongAnswerIsProvided() {
	$this->assertFalse($this->runner->didSomebodyWin($this->aFakeGame(), $this->aWrongAnswer()));
}

private function assertAnswersAreCorrectFor($correctAnserIDs) {
	foreach ($correctAnserIDs as $id) {
		$this->assertTrue($this->runner->isCurrentAnswerCorrect($id, $id));
	}
}

Nice. All these new creations and refactorings got me thinking. We named our variable runner. Maybe 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, don't forget to change your includes manually, because the refactoring will rename the file also.

Now we have a file called GameRunner.php, another one named Runner.php and a third one named Game.php. I don't know about you, but this seems extremely confusing to me. If I was to see these three files for the first time in my life, I would have no idea which one does what. We need to get rid of at least one of them.

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

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

That will do it. We need to include Display.php explicitly, so when Runner tries to create a new CLIDisplay, it will know what to implement.

Analyzing Concerns

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

Surprisingly, these types of questions have a great power in clarifying both business domain and software architecture. We are asking and answering these types of questions in a group at Syneto. Many times when a programmer has a dilemma, he or she just stands up, asks for two minutes of attention from the team in order to find our opinion on a subject. 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 the Runner class. It is hugely more probable to eliminate or transform this class, than Game.

First, should a runner care about how isCurrentAnswerCorrect() working? Should a runner have any knowledge about questions and answers?

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

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

require_once __DIR__ . '/CLIDisplay.php';
include_once __DIR__ . '/Game.php';

class Runner {

	function run() {
		//...//
	}

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

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

But what about didSomebodyWin()? 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.

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

Whatever this method does, it does it on a Game object only. It verifies the current answer returned by game. Then it returns whatever a game object returns in its wasCorrectlyAnswered() or wrongAnswer() methods. This method effectively does nothing on its own. All it cares about is Game. This is a classic example of a code smell called Feature Envy. A 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?

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

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

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

It's now time to change the code as well. Copying and pasting the method into Game will magically make all the tests pass. Both the old ones and the ones moved to GameTest. But 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 fake Game object which we do not need to do anymore since it is part of Game.

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

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

function testItCanTellIfThereIsNoWinnerWhenACorrectAnswerIsProvided() {
	$aGame = \Mockery::mock('Game[wasCorrectlyAnswered]');
	$aGame->shouldReceive('wasCorrectlyAnswered')->once()->andReturn(false);
	$this->assertTrue($aGame->didSomebodyWin($this->aCorrectAnswer()));
}

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

function testItCanTellIfThereIsNoWinnerWhenAWrongAnswerIsProvided() {
	$aGame = \Mockery::mock('Game[wrongAnswer]');
	$aGame->shouldReceive('wrongAnswer')->once()->andReturn(true);
	$this->assertFalse($aGame->didSomebodyWin($this->aWrongAnswer()));
}

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

Final Thoughts

Even though we managed to think about only the Runner, we made great progress today. We learned about responsibilities, we identified methods and variables that belong to another class. We thought on a higher level and we evolved toward a better solution. In the Syneto team, 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. 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.