Google+

Shineyrock

web design & consultancy

Shineyrock

  1. web design en consultancy

blog

  1. Home
  2. blog
  3. lees
  4. Refactoring Legacy Code: Part 5 - Game’s Testable Methods
  • 28

    Refactoring Legacy Code: Part 5 - Game’s Testable Methods

    1. May 2014
    2. Daily
    3. gelezen 901

    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 our previous tutorial, we tested our Runner functions. In this lesson, it is time to continue where we left off by testing our Game class. Now, when you start with such a big chunk of code like we have here, it is tempting to start to test in a top-down manner, method by method. This is, most of the time, impossible. It is much better to start testing it by its short, testable methods. This is what we'll do in this lesson: find and test those methods.

    Creating a Game

    In order to test a class we need to initialize an object of that specific type. We can consider that our first test is to create such a new object. You will be surprised how many secrets constructors can hide.

    require_once __DIR__ . '/../trivia/php/Game.php';

class GameTest extends PHPUnit_Framework_TestCase {

	function testWeCanCreateAGame() {
		$game = new Game();
	}

}

    To our surprise, Game can actually be created quite easily. No problems while running just new Game(). Nothing breaks. This is a very good start, especially considering that Game's constructor is quite large and it does a lot of things.

    Finding the First Testable Method

    It is tempting to simplify the constructor right now. But we have only the golden master to make sure we do not break anything. Before we go to the constructor, we need to test most of the rest of the class. So, where should we start?

    Look for the first method that returns a value and ask yourself, "Can I call and control the return value of this method?". If the answer is yes, it is a good candidate for our test.

    function isPlayable() {
	$minimumNumberOfPlayers = 2;
	return ($this->howManyPlayers() >= $minimumNumberOfPlayers);
}

    What about this method? It seems like a good candidate. Only two lines and it returns a boolean value. But wait, it calls another method, howManyPlayers().

    function howManyPlayers() {
	return count($this->players);
}

    This is basically just a method that counts the elements in the class' players array. OK, so if we do not add any players, it should be zero. isPlayable() should return false. Let's see if our assumption is correct.

    function testAJustCreatedNewGameIsNotPlayable() {
	$game = new Game();
	$this->assertFalse($game->isPlayable());
}

    We renamed our previous test method to reflect what we really want to test. Then we just asserted the game is not playable. The test passes. But false positives are common in many cases. So for piece of mind, we can assert true and make sure the test fails.

    $this->assertTrue($game->isPlayable());

    And it does!

    PHPUnit_Framework_ExpectationFailedException :
Failed asserting that false is true.

    So far, pretty promising. We managed to test the method's initial return value, the value represented by the initial state of the Game class. Please note the emphasized word: "state". We need to find a way to control the state of the game. We need to change it, so it will have the minimum number of players.

    If we analyze Game's add() method, we will see that it adds elements to our array.

    array_push($this->players, $playerName);

    Our assumption is enforced by the way the add() method is used in RunnerFunctions.php.

    function run() {

	$aGame = new Game();
	$aGame->add("Chet");
	$aGame->add("Pat");
	$aGame->add("Sue");

	// ... //
}

    Based on these observations, we can conclude that by using add() twice, we should be able to bring our Game into a state with two players.

    function testAfterAddingTwoPlayersToANewGameItIsPlayable() {
	$game = new Game();
	$game->add('First Player');
	$game->add('Second Player');
	$this->assertTrue($game->isPlayable());
}

    By adding this second test method, we can ensure isPlayable() returns true, if conditions are met.

    But you may think this is not quite a unit test. We use the add() method! We exercise more than the bare minimum of code. We could instead just add the elements to the $players array and not rely on the add() method at all.

    Well, the answer is yes and no. We could do that, from a technical point of view. It will have the advantage of direct control over the array. However, it will have the disadvantage of code duplication between code and tests. So, pick one of the bad options that you think you can live with and use that one. I personally prefer to reuse methods like add().

    Refactoring Tests

    We are on green, we refactor. Can we make our tests better? Well yes, we can. We could transform our first test to verify all conditions of not enough players.

    function testAGameWithNotEnoughPlayersIsNotPlayable() {
	$game = new Game();
	$this->assertFalse($game->isPlayable());
	$game->add('A player');
	$this->assertFalse($game->isPlayable());
}

    You may have heard about the concept of, "One assertion per test". I mostly agree with it, but if you have a test that verifies a single concept and requires multiple assertions to do its verification, I think it is acceptable to use more than one assertion. This view is also strongly promoted by Robert C. Martin in his teachings.

    But what about our second test method? Is that good enough? I say no.

    $game->add('First Player');
$game->add('Second Player');

    These two calls bother me a little. They are a detailed implementation without an explicit explanation in our method. Why not extract them into a private method?

    function testAfterAddingEnoughPlayersToANewGameItIsPlayable() {
	$game = new Game();
	$this->addEnoughPlayers($game);
	$this->assertTrue($game->isPlayable());
}

private function addEnoughPlayers($game) {
	$game->add('First Player');
	$game->add('Second Player');
}

    This is much better and it also leads us to another concept that we missed. In both tests, we expressed in one way or another the concept of "enough players". But how much is enough? Is it two? Yes, for now it is. But do we want our test to fail if the Game's logic will require at least three players? We do not want this to happen. We can introduce a public static class field for it.

    class Game {
	static $minimumNumberOfPlayers = 2;

	// ... //

	function  __construct() {
		// ... //
	}

	function isPlayable() {
		return ($this->howManyPlayers() >= self::$minimumNumberOfPlayers);
	}

	// ... //
}

    This will allow us to use it in our tests.

    private function addEnoughPlayers($game) {
	for($i = 0; $i < Game::$minimumNumberOfPlayers; $i++) {
		$game->add('A Player');
	}
}

    Our little helper method will just add players until enough is added. We can even create another such method for our first test, so we add almost enough players.

    function testAGameWithNotEnoughPlayersIsNotPlayable() {
	$game = new Game();
	$this->assertFalse($game->isPlayable());
	$this->addJustNothEnoughPlayers($game);
	$this->assertFalse($game->isPlayable());
}

private function addJustNothEnoughPlayers($game) {
	for($i = 0; $i < Game::$minimumNumberOfPlayers - 1; $i++) {
		$game->add('A player');
	}
}

    But this introduced some duplication. Our two helper methods are fairly similar. Can't we extract a third one from them?

    private function addEnoughPlayers($game) {
	$this->addManyPlayers($game, Game::$minimumNumberOfPlayers);
}

private function addJustNothEnoughPlayers($game) {
	$this->addManyPlayers($game, Game::$minimumNumberOfPlayers - 1);
}

private function addManyPlayers($game, $numberOfPlayers) {
	for ($i = 0; $i < $numberOfPlayers; $i++) {
		$game->add('A Player');
	}
}

    That is better, but it introduces a different problem. We reduced duplication in these methods, but our $game object is now passed down three levels. It is getting difficult to manage. It's time to initialize it in the test's setUp() method and reuse it.

    class GameTest extends PHPUnit_Framework_TestCase {

	private $game;

	function setUp() {
		$this->game = new Game;
	}

	function testAGameWithNotEnoughPlayersIsNotPlayable() {
		$this->assertFalse($this->game->isPlayable());
		$this->addJustNothEnoughPlayers();
		$this->assertFalse($this->game->isPlayable());
	}

	function testAfterAddingEnoughPlayersToANewGameItIsPlayable() {
		$this->addEnoughPlayers($this->game);
		$this->assertTrue($this->game->isPlayable());
	}

	private function addEnoughPlayers() {
		$this->addManyPlayers(Game::$minimumNumberOfPlayers);
	}

	private function addJustNothEnoughPlayers() {
		$this->addManyPlayers(Game::$minimumNumberOfPlayers - 1);
	}

	private function addManyPlayers($numberOfPlayers) {
		for ($i = 0; $i < $numberOfPlayers; $i++) {
			$this->game->add('A Player');
		}
	}

}

    Much better. All irrelevant code is in private methods, $game is initialized in setUp() and a lot of pollution was removed from the test methods. However, we did have to make a compromise here. In our first test, we start with an assertion. This presumes that setUp() will always create an empty game. This is OK for now. But at the end of the day, you must realize there is no such thing as perfect code. There is just code with compromises that you are willing to live with.

    The Second Testable Method

    If we are scanning our Game class from the top towards the bottom, the next method on our list is add(). Yes, the same method we used in our tests in the previous paragraph. But can we test it?

    function testItCanAddANewPlayer() {
	$this->game->add('A player');
	$this->assertEquals(1, count($this->game->players));
}

    Now this is a different way of testing objects. We call our method and then we verify the object's state. As add() always returns true, there is no way we can test its output. But we can start with an empty Game object and then check if there is a single user after we add one. But is that enough verification?

    function testItCanAddANewPlayer() {
	$this->assertEquals(0, count($this->game->players));
	$this->game->add('A player');
	$this->assertEquals(1, count($this->game->players));
}

    Wouldn't it be better to also verify if there are no players before we call add()? Well, it may be a little bit too much here, but as you can see in the code above, we could do it. And whenever you are not sure of the initial state, you should do an assertion on it. This also protects you from future code changes that may change your object's initial state.

    But are we testing all of the things the add() method does? I say no. Besides adding a user, it also sets a lot of settings for it. We should also check for those.

    function testItCanAddANewPlayer() {
	$this->assertEquals(0, count($this->game->players));
	$this->game->add('A player');
	$this->assertEquals(1, count($this->game->players));
	$this->assertEquals(0, $this->game->places[1]);
	$this->assertEquals(0, $this->game->purses[1]);
	$this->assertFalse($this->game->inPenaltyBox[1]);
}

    This is better. We verify each action that the add() method does. This time, I preferred to directly test the $players array. Why? We could have used the howManyPlayers() method which basically does the same thing, right? Well, in this case we considered that it is more important to describe our assertions by the effects that the add() method has on the state of the object. If we need to change add(), we would expect that the test which is testing its strict behavior, will fail. I've had endless debates with my colleagues at Syneto about this. Especially because this type of testing introduces a strong coupling between the test and how the add() method is actually implemented. So, if you prefer to test it the other way around, that does not mean your ideas are wrong.

    We can safely ignore the testing of the output, the echoln() lines. They are just outputting content on the screen. We don't want to touch these methods, yet. Our golden master totally relies on this output.

    Refactoring Tests (Bis)

    We have another tested method with a brand new passing test. It's time to refactor both of them, just a little bit. Let's start with our tests. Aren't the last three assertions a little bit confusing? They don't seem to be related strictly to adding a player. Let's change it:

    function testItCanAddANewPlayer() {
	$this->assertEquals(0, count($this->game->players));
	$this->game->add('A player');
	$this->assertEquals(1, count($this->game->players));
	$this->assertDefaultPlayerParametersAreSetFor(1);
}

    That's better. The method is now more abstract, reusable, expressively named and hides all of the unimportant details.

    Refactoring the add() Method

    We can do something similar with our production code.

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

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

    We extracted the unimportant details into setDefaultPlayerParametersFor().

    private function setDefaultPlayerParametersFor($playerId) {
	$this->places[$playerId] = 0;
	$this->purses[$playerId] = 0;
	$this->inPenaltyBox[$playerId] = false;
}

    Actually this idea came to me after I wrote the test. This is another nice example of how tests force us to think about our code from a different point of view. This different angle on the problem, is what we must exploit and let our tests guide our design of the production code.

    The Third Testable Method

    Let's find our third candidate for testing. howManyPlayers() is too simple and indirectly already tested. roll() is too complex to be tested directly. Plus it returns null. askQuestions() seems to be interesting at first sight, but it is all presentation, no return value.

    currentCategory() is testable, but it is pretty difficult to test. It is a huge selector with ten conditions. We need a ten lines long test and then we need to seriously refactor this method and most certainly the tests as well. We should take note of this method and come back to it after we are finished with the easier ones. For us, this will be in our next tutorial.

    wasCorrectlyAnswered() is to complicated again. We will need to extract from it, small pieces of code that are testable. However, wrongAnswer() seems promising. It outputs stuff on the screen, but it also changes the state of our object. Let's see if we can control it and test it.

    function testWhenAPlayerEntersAWrongAnswerItIsSentToThePenaltyBox() {
	$this->game->add('A player');
	$this->game->currentPlayer = 0;
	$this->game->wrongAnswer();
	$this->assertTrue($this->game->inPenaltyBox[0]);
}

    Grrr... It was quite hard to write this test method. wrongAnswer() relies on $this->currentPlayer for its behavioral logic, but it also uses $this->players in its presentation part. One ugly example of why you should not mix logic and presentation. We will deal with this in a future tutorial. For now, we tested that the user enters the penalty box. We must also observe that there is an if() statement in the method. This is a condition that we do not yet test, as we only have a single player and thus we are not satisfying the condition. We could test for the final value of $currentPlayer though. But adding this line of code to the test will make it fail.

    $this->assertEquals(1, $this->game->currentPlayer);

    A closer look at the private method shouldResetCurrentPlayer() reveals the problem. If the index of the current player equals with the number of players, it will be reset to zero. Aaaahhh! We actually enter the if()!

    function testWhenAPlayerEntersAWrongAnswerItIsSentToThePenaltyBox() {
	$this->game->add('A player');
	$this->game->currentPlayer = 0;
	$this->game->wrongAnswer();
	$this->assertTrue($this->game->inPenaltyBox[0]);
	$this->assertEquals(0, $this->game->currentPlayer);
}

function testCurrentPlayerIsNotResetAfterWrongAnswerIfOtherPlayersDidNotYetPlay() {
	$this->addManyPlayers(2);
	$this->game->currentPlayer = 0;
	$this->game->wrongAnswer();
	$this->assertEquals(1, $this->game->currentPlayer);
}

    Good. We created a second test, to test the specific case when there are still players who did not play. We don't care about the inPenaltyBox state for the second test. We are only interested in the index of the current player.

    The Final Testable Method

    The last method we can test and then refactor is didPlayerWin().

    function didPlayerWin() {
	$numberOfCoinsToWin = 6;
	return !($this->purses[$this->currentPlayer] == $numberOfCoinsToWin);
}

    We can immediately observe that its code structure is very similar to isPlayable(), the method we tested first. Our solution should be something similar as well. When your code is so short, only two to three lines, doing more than one tiny step is not that big of a risk. In worst case scenarios, you revert three lines of code. So let's do this in a single step.

    function testTestPlayerWinsWithTheCorrectNumberOfCoins() {
	$this->game->currentPlayer = 0;
	$this->game->purses[0] = Game::$numberOfCoinsToWin;
	$this->assertTrue($this->game->didPlayerWin());
}

    But wait! That fails. How is that possible? Shouldn't it pass? We provided the correct number of coins. If we study our method, we'll discover a little misleading fact.

    return !($this->purses[$this->currentPlayer] == $numberOfCoinsToWin);

    The return value is actually negated. So the method is not telling us if a player won, it tells us if a player did not win the game. We could go in and find the places where this method is used and negate its value there. Then change its behavior here, to not falsely negate the answer. But it is used in wasCorrectlyAnswered(), a method we can not yet unit test. Maybe for the time being, a simple renaming to highlight the correct functionality will be enough.

    function didPlayerNotWin() {
	return !($this->purses[$this->currentPlayer] == self::$numberOfCoinsToWin);
}

    Thoughts & Conclusion

    So this about wraps up the tutorial. While we do not like the negation in the name, this is a compromise we can make at this point. This name will surely change when we start refactoring other parts of the code. Additionally, if you take a look at our tests, they look odd now:

    function testTestPlayerWinsWithTheCorrectNumberOfCoins() {
	$this->game->currentPlayer = 0;
	$this->game->purses[0] = Game::$numberOfCoinsToWin;
	$this->assertFalse($this->game->didPlayerNotWin());
}

    By testing false on a negated method, exercised with a value that suggests a true result, we introduced quite a lot of confusion to our codes readability. But this is good for now, as we do need to stop at some point, right?

    In our next tutorial, we will start working on some of the more difficult methods within the Game class. Thank you for reading.

    Photo

    martijn broeders

    founder/ strategic creative bij shineyrock web design & consultancy
    e-mail: .(JavaScript must be enabled to view this email address)
    telefoon: 434 210 0245
    0
    Plaats reactie...
    Comments

Per - categorie

Op - datum

Copyright © 1999 - 2026 Shineyrock web design en consultancy, Alle rechten voorbehouden / Algemene voorwaarden / Sitemap: XML, HTML, Images, Mobile, News