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.
One of the key moments of refactoring a totally legacy code is when we start extracting small pieces from it and we start writing targeted unit tests for those small pieces. But this can be quite difficult, especially when you have code that is written so that it would be hard to compile or run if pieces of it are missing. We can't safely do large surgeries on a code we still barely understand and only a golden master test keeps us breaking it totally. Fortunately there are some techniques that can help us.
What Is a Unit Test?
Throughout the history of automated testing, the past twenty or so years, the term Unit Test was defined in many ways. Initially it was about the scope of the code exercised inside a test. A unit test was a test that tested the smallest possible unit of a particular programming language.
In this sense, for our PHP code, a unit test is a test that exercises a single function or method. When we are programming in an object oriented style, our functions are organized in classes. All the tests associated with a single class are usually called a Test Case.
There are about 25 other definitions for the term of Unit Test, so we will not go into each one. While these definitions are quite different, all of them has two things in common. This leads us to the probably most accepted definition.
A Unit Test is a test that runs in milliseconds and tests a piece of code in isolation.
We must note two key words in the definition: milliseconds - our tests must run fast, very fast; and isolation - we must test our code as isolated as possible. These two key words go hand-in-hand, because in order to make tests faster we must reduce their scope. Databases, network communications, user interfaces, they are just too slow to be tested this way. We need to find and isolate a small enough chunk of code, so that we can compile (if needed) and run that code in the order of milliseconds, that is, in less than ten milliseconds, because that would be a centisecond. Our test framework will add a slight overhead over the pure run time of the code, but that is negligible.
Identifying Code to be Unit Tested
Finding Isolated Methods
If the structure of the code permits, it is recommended to start by writing tests for whatever code we actually can test. This will help us start to build up coverage and it will also force us to concentrate and understand small pieces of code. Remember, we are refactoring, we do not want to change behavior. In fact, at this initial step we do not want to change our production code at all if possible.
We need to analyze our three files, to see what we can test and what not.
GameRunner.php has basically no logic. We created it to be just a delegation. Could we test it? Sure we could. Should we test it? No, we shouldn't. Even though some methods can, in a technical sense, be tested, if there is no logic in them we probably don't want to test them.
RunnerFunctions.php is a different story. There are two functions in there. run() is a big function, doing a whole run of the system. This is not something we can easily test. And it has no return value either, it just outputs to the screen, so we would need to capture output and compare strings. This is not very typical for Unit Testing. On the other hand isCurrentAnswerCorrect() returns a simple true or false based on some conditions. Can we test that?
function isCurrentAnswerCorrect() {
$minAnswerId = 0;
$maxAnswerId = 9;
$wrongAnswerId = 7;
return rand($minAnswerId, $maxAnswerId) != $wrongAnswerId;
}
We already understand that this code generates a random number and compares it to the ID of the wrong number.
Step 1 - go to the GoldenMasterTest.php and mark all tests as skipped. We do not want to run them for the time being. As we start building unit tests, we will run our golden master more rarely. As we write new tests and we do not modify the production code, fast feedback is more important.
Step 2 - create a new test RunnerFunctionsTest.php in our Test directory, alongside GoldenMasterTest.php. Now think about the simplest possible test code that you can write. What is the bare minimum to get it running? Well, it is something like this:
require_once __DIR__ . '/../trivia/php/RunnerFunctions.php';
class RunnerFunctionsTest extends PHPUnit_Framework_TestCase {
function testItCanFindCorrectAnswer() {
}
}
We require the RunnerFunctions.php file, so we test that it can be included and does not produce an error. The rest of the code is pure boilerplate, just a skeleton class and an empty test function. But, now what? What do we do next? Do you know how can we trick rand() to return what we want? I do not know, yet. So let's investigate how it is working right now.
We know how to seed the random generator, so what if we try to seed it with some numbers, would that work? We can write code in our test to figure out how something works.
function testItCanFindCorrectAnswer() {
srand(0);
var_dump(rand(0,9));
srand(1);
var_dump(rand(0,9));
srand(2);
var_dump(rand(0,9));
srand(3);
var_dump(rand(0,9));
srand(4);
var_dump(rand(0,9));
}
We also know that our question IDs are between zero and nine. This produces the output below.
int(8) int(8) int(7) int(5) int(9)
Well, that doesn't look very obvious. In fact I can see no logic on how could we determine the values the rand() function will produce. We will need to modify our production code, so that we can inject the values we need.
Dependencies and Dependency Injection
When most people are talking about "dependency" they think about connections between classes. This is the most common case, especially in object oriented programming. But what if we generalize the term a little bit. Forget about classes, forget about objects, concentrate only on the meaning of "dependency". What does our rand(min,max) method depend on? It depends on two values. A minimum and a maximum.
Can we control rand() by those two parameters? Doesn't rand() predictably return the same number if min and max are the same? Let's see.
function testItCanFindCorrectAnswer() {
var_dump(rand(0,0));
var_dump(rand(1,1));
var_dump(rand(2,2));
var_dump(rand(3,3));
var_dump(rand(4,4));
}
If we are right, each line should dump a number from zero to four in a predictable way.
int(0) int(1) int(2) int(3) int(4)
That looks pretty predictable to me. By sending the same number for min and max to rand() we can be sure we generate the expected number. Now, how do we do this for our function? It has no parameters!
Probably the most common way to inject dependencies into a method is to use parameters with default values. This will preserve the function's current functionality, but will allow us to control its flow when we test it.
function isCurrentAnswerCorrect($minAnswerId = 0, $maxAnswerId = 9) {
$wrongAnswerId = 7;
return rand($minAnswerId, $maxAnswerId) != $wrongAnswerId;
}
Modifying the isCurrentAnswerCorrect() this way will preserve its current behavior and allow us to test it in the same time. You can re-enable your golden master and run it now. The production code was changed, we need to be sure we didn't break it.
As our isCurrentAnswerCorrect() looks now, testing it is just a matter of sending in ten values for each possible number returned by rand().
function testItCanFindCorrectAnswer() {
$this->assertTrue(isCurrentAnswerCorrect(0,0));
$this->assertTrue(isCurrentAnswerCorrect(1,1));
$this->assertTrue(isCurrentAnswerCorrect(2,2));
$this->assertTrue(isCurrentAnswerCorrect(3,3));
$this->assertTrue(isCurrentAnswerCorrect(4,4));
$this->assertTrue(isCurrentAnswerCorrect(5,5));
$this->assertTrue(isCurrentAnswerCorrect(6,6));
$this->assertFalse(isCurrentAnswerCorrect(7,7));
$this->assertTrue(isCurrentAnswerCorrect(8,8));
$this->assertTrue(isCurrentAnswerCorrect(9,9));
}
That test function was built by running our tests after each line. Now that our tests are very fast, we can run them almost continuously. There are actually tools to run tests as soon as a file changes and I've heard about programmers who are running their tests continuously and they just glimpse at the test status bar at the end of each command. As you program you know what to expect, if the test doesn't turn green when you thought it should, you did something wrong. Their feedback look is so tight, it is almost a certainty that something went wrong in the last line or command they wrote.
Even though that may sound extreme test driven development, I imagine it is useful especially when you develop algorithms. I personally prefer to run my tests by pressing a shortcut, a single key shortcut. And as tests are helping me develop my programs, my shortcut for running tests is F1.
Let's get back to our business. That test, with ten assertions, runs in 66 ms, about 6.6 ms per assertion. Each assertion calls and executes a piece of our code. This seems to be as we defined unit tests at the beginning of this tutorial.
Did you spot the assertFalse() for the number seven? I bet half of you missed it. It is buried deep inside a bunch of other assertions. Hard to spot. I think it deserves its own test, so we make explicit the single wrong answer case.
function testItCanFindCorrectAnswer() {
$this->assertTrue(isCurrentAnswerCorrect(0, 0));
$this->assertTrue(isCurrentAnswerCorrect(1, 1));
$this->assertTrue(isCurrentAnswerCorrect(2, 2));
$this->assertTrue(isCurrentAnswerCorrect(3, 3));
$this->assertTrue(isCurrentAnswerCorrect(4, 4));
$this->assertTrue(isCurrentAnswerCorrect(5, 5));
$this->assertTrue(isCurrentAnswerCorrect(6, 6));
$this->assertTrue(isCurrentAnswerCorrect(8, 8));
$this->assertTrue(isCurrentAnswerCorrect(9, 9));
}
function testItCanFindWrongAnser() {
$this->assertFalse(isCurrentAnswerCorrect(7, 7));
}
Refactoring Tests
As we are in a quest of refactoring, making code better, easier to understand, we must not forget about our tests. They are just as important as our production code. We need to keep our tests clean and easy to understand also. We need to refactor our tests and we should do it as soon as we observe something is wrong with them and only when they are passing. In this way, the production code can verify our tests. If we have a green test, we refactor it and it turns red, we broke the test. We can just undo a few steps and try again.
We could extract the correct answer numbers into an array and use that to generate correct answers.
function testItCanFindCorrectAnswer() {
$correctAnserIDs = [0, 1, 2, 3, 4, 5, 6, 8, 9];
foreach ($correctAnserIDs as $id) {
$this->assertTrue(isCurrentAnswerCorrect($id, $id));
}
}
That passes, but also introduces some logic. Maybe we could extract it in a custom assertion. This may be a little bit extreme for such a simple test, but it is a good opportunity to understand the concept.
function testItCanFindCorrectAnswer() {
$correctAnserIDs = [0, 1, 2, 3, 4, 5, 6, 8, 9];
$this->assertAnswersAreCorrectFor($correctAnserIDs);
}
function testItCanFindWrongAnser() {
$this->assertFalse(isCurrentAnswerCorrect(7, 7));
}
private function assertAnswersAreCorrectFor($correctAnserIDs) {
foreach ($correctAnserIDs as $id) {
$this->assertTrue(isCurrentAnswerCorrect($id, $id));
}
}
Now, this helped us in two ways. First, we moved the logic about going over each element of the array an verifying it into a private method. As we usually keep our private methods at the end of the class, out of sight, out of the way of the higher level logic in the public methods, we managed to rise the abstraction of our test. In the test method we don't care about how the answers are verified for correctness. We care about the IDs that should represent correct answers. The second advantage is the breaking of the implementation from the preparation. Keeping the correct answer IDs in the test helped us separate the details of implementation from the premise we need to test.
Test to Production Code Dependencies
One of the most common mistakes any of us commits when writing test is to repeat what it is in the production code. This is a case of both code duplication and a hidden dependency, usually, on some values or constants. In our case the dependency is on the answer ID that represents the wrong answer.
But how to prove this dependency? At first sight it seems only a simple duplication of a single value. To answer your dilemma ask yourself this question: "Should my tests fail if I decide to change the wrong answer's ID?". Of course the answer is no. Changing a simple constant in the production code will not affect behavior, or logic. Thus, the tests should not fail.
That sounds great! But how to do it? Well, the simplest way is just to expose the desired variable as a public class variable, preferable static or constant. In our case, as we have no class, we can just make it a global variable or constant.
include_once __DIR__ . '/Game.php';
const WRONG_ANSWER_ID = 7;
function isCurrentAnswerCorrect($minAnswerId = 0, $maxAnswerId = 9) {
return rand($minAnswerId, $maxAnswerId) != WRONG_ANSWER_ID;
}
function run() {
// ... //
}
First modify the RunnerFunctions.php file so that isCurrentAnswerCorrect() will use a constant instead of a local variable. Then run your unit tests. This ensures us that the change we made to the production code did not break anything. Now it's time for the test.
function testItCanFindWrongAnswer() {
$this->assertFalse(isCurrentAnswerCorrect(WRONG_ANSWER_ID, WRONG_ANSWER_ID));
}
Modify testItCanFindWrongAnswer() to use the same constant. As the file RunnerFunctions.php is included at the beginning of the test file, the declared constant will be accessible to the the test.
Refactoring Tests (Again)
Now, that we rely on the WRONG_ANSWER_ID for our testItCanFindWrongAnswer(), shouldn't we refactor our test so that testItCanFindCorrectAnswer() also relies on the same constant? Well we should. It will not only make our test easier to understand, it will also make it more robust. Yes, because if we would to select a wrong answer ID that is already in the list of correct answers defined in the test, that particular case would fail the test even though the production code would still be correct.
class RunnerFunctionsTest extends PHPUnit_Framework_TestCase {
function testItCanFindCorrectAnswer() {
$correctAnserIDs = $this->getGoodAnswerIDs();
$this->assertAnswersAreCorrectFor($correctAnserIDs);
}
private function getGoodAnswerIDs() {
return [0, 1, 2, 3, 4, 5, 6, 8, 9];
}
}
While having the numbers for the correct answers in the test function itself was a good idea at some point, as we change our test to rely more and more on the values provided by the production code, we also want to hide the details about the numbers. The first step is to apply an Extract Method refactoring and get it in its own method.
class RunnerFunctionsTest extends PHPUnit_Framework_TestCase {
function testItCanFindCorrectAnswer() {
$correctAnserIDs = $this->getGoodAnswerIDs();
$this->assertAnswersAreCorrectFor($correctAnserIDs);
}
private function getGoodAnswerIDs() {
return array_diff(range(0,9), [WRONG_ANSWER_ID]);
}
}
We changed getGoodAnswerIDs() significantly. First of all we generate the list with range() instead of typing all possible IDs by hand. Then we subtract from the array the element containing WRONG_ANSWER_ID. Now the list of correct answer IDs is also independent from the value set in the wrong answer's ID. But is that enough? What about the minimum and maximum IDs? Can't we extract them also in a similar manner? Well, let's see.
include_once __DIR__ . '/Game.php';
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() {
// ... //
}
This looks pretty nice. The constants were only used as default values for parameters of the function isCurrentAnswerCorrect(). This still allows us to inject the required values when testing and it also makes quite clear what those parameters mean. As a nice side effect, a small block of constants at the top of the file started to highlight the key values our RunnerFunctions.php file uses. Nice!
Just don't forget to re-enable from the golden master test the testOutputMatchesGoldenMaster() test function. The constants we introduced are used only in the golden master test. Our unit tests actually shortcut those values always.
Now we need to update our unit test to use the constants.
class RunnerFunctionsTest extends PHPUnit_Framework_TestCase {
function testItCanFindCorrectAnswer() {
$correctAnserIDs = $this->getGoodAnswerIDs();
$this->assertAnswersAreCorrectFor($correctAnserIDs);
}
private function getGoodAnswerIDs() {
return array_diff(range(MIN_ANSWER_ID,MAX_ANSWER_ID), [WRONG_ANSWER_ID]);
}
}
It was simple and easy. We just had to change the parameters to the range() method.
The last step we can do with our test, is to clean up the mess we left behind in our testItCanFindCorrectAnswer() method.
function testItCanFindCorrectAnswer() {
$correctAnserIDs = $this->getGoodAnswerIDs();
$this->assertAnswersAreCorrectFor($correctAnserIDs);
}
We can observe two major problems with this code. First an inconsistency in naming. Once we called answers correct and then we called them good. We must decide on one of the two. Correct seems to be grammatically more fitting. As correct is the opposite of wrong, while good is the opposite of bad.
class RunnerFunctionsTest extends PHPUnit_Framework_TestCase {
function testItCanFindCorrectAnswer() {
$correctAnserIDs = $this->getCorrectAnswerIDs();
$this->assertAnswersAreCorrectFor($correctAnserIDs);
}
private function getCorrectAnswerIDs() {
return array_diff(range(MIN_ANSWER_ID,MAX_ANSWER_ID), [WRONG_ANSWER_ID]);
}
}
We renamed our private method according to the reasoning above. But that is not enough. We need to solve another problem. We assign the return value of a private method to a variable just to use that same variable on the next line. And this is the only use case for the variable. In our case the variable was there because it provided extra clarification about what an array of number meant. It had its use and scope. But now that we have a method with almost the same name, expressing the same concept, the variable outlived its usefulness. This is an unnecessary assignment.
We can use the inline variable refactoring to remove the variable and call the method directly instead of using the variable on the next line.
class RunnerFunctionsTest extends PHPUnit_Framework_TestCase {
function testItCanFindCorrectAnswer() {
$this->assertAnswersAreCorrectFor($this->getCorrectAnswerIDs());
}
private function getCorrectAnswerIDs() {
return array_diff(range(MIN_ANSWER_ID,MAX_ANSWER_ID), [WRONG_ANSWER_ID]);
}
}
Now, what is really cool here is that we started with only two lines of code that was not that clear and it was polluted by duplication and a hidden dependency. After a few steps of changes we ended up with two lines of code also, but we broke the dependency on the numerical ID numbers. Is that cool or what?
Breaking the Run
Are we finished with the RunnerFunctions.php? Well if I see an if() that means logic. If I see logic that means a unit test is needed to verify it. And we have an if() in our run() method's do-while() loop. It's time to use our IDE's refactoring tool to extract a method and then test it.
But what piece of code should we extract? At first glance taking just the conditional statement seems a good idea. This leads to the code below.
function run() {
$notAWinner;
$aGame = new Game();
$aGame->add("Chet");
$aGame->add("Pat");
$aGame->add("Sue");
do {
$dice = rand(0, 5) + 1;
$aGame->roll($dice);
$notAWinner = getNotWinner($aGame);
} while ($notAWinner);
}
function getNotWinner($aGame) {
if (isCurrentAnswerCorrect()) {
$notAWinner = $aGame->wasCorrectlyAnswered();
return $notAWinner;
} else {
$notAWinner = $aGame->wrongAnswer();
return $notAWinner;
}
}
While this looks pretty decent and it was generated by just selecting the right menu item from our IDE, there is a problem that bothers me. The aGame object is used both in the do-while loop and both in the extracted method. What about this?
function run() {
$notAWinner;
$aGame = new Game();
$aGame->add("Chet");
$aGame->add("Pat");
$aGame->add("Sue");
do {
$dice = rand(0, 5) + 1;
$notAWinner = getNotWinner($aGame, $dice);
} while ($notAWinner);
}
function getNotWinner($aGame, $dice) {
$aGame->roll($dice);
if (isCurrentAnswerCorrect()) {
$notAWinner = $aGame->wasCorrectlyAnswered();
return $notAWinner;
} else {
$notAWinner = $aGame->wrongAnswer();
return $notAWinner;
}
}
This solution removes the aGame object from the loop. However it introduces other type of problems. Our parameter count increases. Now we need to send in $dice. While the sheer number of parameters, two, is low enough to not rise any concerns we must also think about how those parameters are used in the method itself. $dice is only used when the roll() method is called on aGame. While the roll() method has a great significance in the Game class, it is not the one that decides if we have a winner or not. By analyzing the code in Game, we can conclude that a winner state can be true only by calling wasCorrectlyAnswered(). This is strange and it highlights some serious naming issues in the Game class we will address in an upcoming lesson.
Based on all the above observations, it is most probably better to go with the first version of our extracted method.
function getNotWinner($aGame) {
if (isCurrentAnswerCorrect()) {
$notAWinner = $aGame->wasCorrectlyAnswered();
return $notAWinner;
} else {
$notAWinner = $aGame->wrongAnswer();
return $notAWinner;
}
}
We can believe in our IDE and by just looking at the code we can be pretty sure nothing has broken. If you feel uncertain, just run your golden master tests. Now let's focus on creating some tests for this nice method.
function testItCanTellIfThereIsNoWinnerWhenACorrectAnswerIsProvided() {}
I came up with this name by transforming what I want to test into the test method's name. It is very important to name your tests about what behavior they should test and not about what they will do. This will help others or yourself six months from now, to understand what that small piece of code should actually do.
But we have a problem. Our tested method needs an object. We need to run it like this:
function testItCanTellIfThereIsNoWinnerWhenACorrectAnswerIsProvided() {
$aGame = ???
getNotWinner($aGame);
}
We need an $aGame object of type Game. But we are doing a unit test, we do not want to use the real, complex and badly understood, Game class. This leads us to a new chapter in testing we will talk about in an another lesson: Mocking, Stubbing and Faking. These are all techniques to create and test object by using other objects that behave in a predefined manner. While using a framework or even PHPUnit's own built-in capabilities can be of help, for our current knowledge for our very simple test we can do a thing many people forget.
We can just create a class similar to Game inside our test file and define on it the only two methods we are interested in. It is very simple.
class RunnerFunctionsTest extends PHPUnit_Framework_TestCase {
// ... //
function testItCanTellIfThereIsNoWinnerWhenACorrectAnswerIsProvided() {
$aGame = new FakeGame();
getNotWinner($aGame);
}
// ... //
}
class FakeGame {
function wasCorrectlyAnswered() {
}
function wrongAnswer() {
}
}
This makes our tests pass and we are still in the millisecond zone. Note that the two skipped tests are the ones from the golden master.
Time: 43 ms, Memory: 3.00Mb OK, but incomplete or skipped tests! Tests: 5, Assertions: 10, Skipped: 2.
Even though we had to name our class differently from Game because we can't declare the same class twice, the code is pretty simple. We just defined the two methods we are interested in. The next step is to actually return something and test for it. But this may be more difficult than we expected because of this line of code:
if (isCurrentAnswerCorrect())
Our method calls isCurrentAnswerCorrect() without any parameters. This is bad for us. We can't control its output. It will just generate random numbers. We need to refactor our code a little bit before we can continue. We need to move the call to this method into the loop and pass its result as a parameter to getNotWinner(). This will allow us to control the result of the expression in the above if statement, thus controlling the path on which our code will go down. For our first test we need it to enter the if and call wasCorrectlyAnswered().
function run() {
// ... //
do {
$dice = rand(0, 5) + 1;
$aGame->roll($dice);
$notAWinner = getNotWinner($aGame, isCurrentAnswerCorrect());
} while ($notAWinner);
}
function getNotWinner($aGame, $isCurrentAnswerCorrect) {
if ($isCurrentAnswerCorrect) {
$notAWinner = $aGame->wasCorrectlyAnswered();
return $notAWinner;
} else {
$notAWinner = $aGame->wrongAnswer();
return $notAWinner;
}
}
Now we have control, all dependencies broken. It's time for testing.
function testItCanTellIfThereIsNoWinnerWhenACorrectAnswerIsProvided() {
$aGame = new FakeGame();
$isCurrentAnswerCorrect = true;
$this->assertTrue(getNotWinner($aGame, $isCurrentAnswerCorrect));
}
This is a passing test, pretty nice. We returned true from our overridden method, of course.
function wasCorrectlyAnswered() {
return true;
}
We need to test the other path through the if() also.
function testItCanTellIfThereIsNoWinnerWhenAWrongAnswerIsProvided() {
$aGame = new FakeGame();
$isCurrentAnswerCorrect = false;
$this->assertFalse(getNotWinner($aGame, $isCurrentAnswerCorrect));
}
We just chose to test false this time, so we differentiate between the two cases easier.
class FakeGame {
function wasCorrectlyAnswered() {
return true;
}
function wrongAnswer() {
return false;
}
}
And our FakeGame was modified accordingly.
Final Cleanup
Refactoring the Extracted Method
We are almost done. Sorry for getting this tutorial so long, I hope you liked it and didn't fall asleep. Final changes before concluding the RunnerFunctions.php file and its tests.
function getNotWinner($aGame, $isCurrentAnswerCorrect) {
if ($isCurrentAnswerCorrect) {
$notAWinner = $aGame->wasCorrectlyAnswered();
return $notAWinner;
} else {
$notAWinner = $aGame->wrongAnswer();
return $notAWinner;
}
}
There are some unnecessary assignments in our method, we should clean it up. Our unit tests will make this change very safe.
function getNotWinner($aGame, $isCurrentAnswerCorrect) {
if ($isCurrentAnswerCorrect) {
return $aGame->wasCorrectlyAnswered();
} else {
return $aGame->wrongAnswer();
}
}
We applied the same inline variable refactoring and it led to its disappearance. Tests still passing and we are still under 100 ms for all the unit tests together. I say this is pretty nice.
Refactoring Tests (Again, Again)
Yes, yes, we can make our test a little bit better also. Since we only have a few lines of code, our refactorings will be easy. The problem is in the code below.
function testItCanTellIfThereIsNoWinnerWhenACorrectAnswerIsProvided() {
$aGame = new FakeGame();
$isCurrentAnswerCorrect = true;
$this->assertTrue(getNotWinner($aGame, $isCurrentAnswerCorrect));
}
function testItCanTellIfThereIsNoWinnerWhenAWrongAnswerIsProvided() {
$aGame = new FakeGame();
$isCurrentAnswerCorrect = false;
$this->assertFalse(getNotWinner($aGame, $isCurrentAnswerCorrect));
}
We have duplicate code by calling new FakeGame() in each method. Time for an extract method.
function testItCanTellIfThereIsNoWinnerWhenACorrectAnswerIsProvided() {
$aGame = $this->aFakeGame();
$isCurrentAnswerCorrect = true;
$this->assertTrue(getNotWinner($aGame, $isCurrentAnswerCorrect));
}
function testItCanTellIfThereIsNoWinnerWhenAWrongAnswerIsProvided() {
$aGame = $this->aFakeGame();
$isCurrentAnswerCorrect = false;
$this->assertFalse(getNotWinner($aGame, $isCurrentAnswerCorrect));
}
Now, this makes the $aGame variable pretty useless. Time for inline variable.
function testItCanTellIfThereIsNoWinnerWhenACorrectAnswerIsProvided() {
$isCurrentAnswerCorrect = true;
$this->assertTrue(getNotWinner($this->aFakeGame(), $isCurrentAnswerCorrect));
}
function testItCanTellIfThereIsNoWinnerWhenAWrongAnswerIsProvided() {
$isCurrentAnswerCorrect = false;
$this->assertFalse(getNotWinner($this->aFakeGame(), $isCurrentAnswerCorrect));
}
This made our code shorter and more expressive on the same time. When we read an assertion it reads like a prose. Assert that we receive true whe we call try to get the not winner using our fake class with correct answer provided. What I still don't like is that we use the same variable and assign to it true or false depending on the test. I think there should be a more expressive way to do it.
function testItCanTellIfThereIsNoWinnerWhenACorrectAnswerIsProvided() {
$this->assertTrue(getNotWinner($this->aFakeGame(), $this->aCorrectAnswer()));
}
function testItCanTellIfThereIsNoWinnerWhenAWrongAnswerIsProvided() {
$this->assertFalse(getNotWinner($this->aFakeGame(), $this->aWrongAnswer()));
}
Wow! Our tests became single liners and they are truly expressing what we are testing. All the details are hidden in private methods, at the end of the test. 99% of the cases you will not care about their implementation and when you do, you can simply CTRL+click on the method's name and the IDE will jump to the implementation.
Back to the Production Code
If we look at our loop, we can see that there is a variable we can get rid of in a blink of an eye.
function run() {
$notAWinner;
$aGame = new Game();
$aGame->add("Chet");
$aGame->add("Pat");
$aGame->add("Sue");
do {
$dice = rand(0, 5) + 1;
$aGame->roll($dice);
$notAWinner = getNotWinner($aGame, isCurrentAnswerCorrect());
} while ($notAWinner);
}
That will turn into this:
function run() {
$aGame = new Game();
$aGame->add("Chet");
$aGame->add("Pat");
$aGame->add("Sue");
do {
$dice = rand(0, 5) + 1;
$aGame->roll($dice);
} while (getNotWinner($aGame, isCurrentAnswerCorrect()));
}
Bye, bye $notAWinner variable. But our method's name is horrible. We know we should always prefer positive naming and behavior and negate it where needed in conditionals. What about this naming?
do {
$dice = rand(0, 5) + 1;
$aGame->roll($dice);
} while (didSomebodyWin($aGame, isCurrentAnswerCorrect()));
But with that name, we need to negate it in the while() and change its behavior also. We start by changing our tests.
class FakeGame {
function wasCorrectlyAnswered() {
return false;
}
function wrongAnswer() {
return true;
}
}
Actually changing only our fake game is better. It keeps the tests really readable, with the new method names.
function testItCanTellIfThereIsNoWinnerWhenACorrectAnswerIsProvided() {
$this->assertTrue(didSomebodyWin($this->aFakeGame(), $this->aCorrectAnswer()));
}
function testItCanTellIfThereIsNoWinnerWhenAWrongAnswerIsProvided() {
$this->assertFalse(didSomebodyWin($this->aFakeGame(), $this->aWrongAnswer()));
}
Getting the Tests to Pass
Of course the tests are failing now. We have to change the method's implementation.
function didSomebodyWin($aGame, $isCurrentAnswerCorrect) {
if ($isCurrentAnswerCorrect) {
return ! $aGame->wasCorrectlyAnswered();
} else {
return ! $aGame->wrongAnswer();
}
}
Fixing the Golden Master
Unit tests are passing, but running our golden master will break. We need to negate the login in the while statement.
do {
$dice = rand(0, 5) + 1;
$aGame->roll($dice);
} while (!didSomebodyWin($aGame, isCurrentAnswerCorrect()));
Done!
Now that makes the golden master pass again and our do-while reads like well written prose also. Now it is really time to stop. Thank you for reading.
Comments