@__edorian writes: @elblinkin I can’t seem to make the BowlingGameTest fit your coding standard: [gist here] How does that work out for you? 
Here is the code he was talking about:
<?php
class GameTest extends PHPUnit_Framework_TestCase {
/**
* @var Game
*/
private $game;
protected function setUp() {
$this->game = new Game();
}
public function testGutterGame() {
$this->rollMany(20, 0);
$this->assertSame(0, $this->game->score());
}
public function testRollAllOnes() {
$this->rollMany(20, 1);
$this->assertSame(20, $this->game->score());
}
public function testRollSpare() {
$this->rollSpare();
$this->game->roll(1);
$this->rollMany(17, 0);
$this->assertSame(12, $this->game->score());
}
public function testRollStrike() {
$this->game->roll(10);
$this->game->roll(1);
$this->game->roll(3);
$this->rollMany(16, 0);
$this->assertSame(18, $this->game->score());
}
public function testPerfectGame() {
$this->rollMany(12, 10);
$this->assertSame(300, $this->game->score());
}
public function testThenStrikesThenTwoFives() {
$this->rollMany(10, 10);
$this->rollMany(2, 5);
$this->assertSame(285, $this->game->score());
}
private function rollSpare() {
$this->game->roll(5);
$this->game->roll(5);
}
private function rollMany($rolls, $pins) {
for (; $rolls; --$rolls) {
$this->game->roll($pins);
}
}
/**
* @expectedException InvalidArgumentException
*/
public function testRollingNegativeAmountOfPinsFails() {
$this->game->roll(-1);
}
/**
* @expectedException InvalidArgumentException
*/
public function testRollingMoreThan10PinsFails() {
$this->game->roll(11);
}
}
?>
This is one of many canonical examples used for teaching how to use an xUnit framework. It is not necessarily a comment on how to properly test or how to write testable code. It is an example to show that you can potentially use all normal language constructs in a test.
The failed sniffs were mainly due to the private methods.
phpcs --standard=PHPUnitStandard TestExample.php
52 | ERROR | Private methods are prohibited; found rollSpare
57 | ERROR | Private methods are prohibited; found rollMany
57 | ERROR | Function's cyclomatic complexity (2) exceeds allowed maximum of
| | 1
57 | ERROR | Function's nesting level (1) exceeds allowed maximum of 0
You can refer back to what I said in a a previous article to guess what my opinion is.
One possible solution is that you could copy and paste the internals of the private functions into the tests that call them. Most times private functions are small and not used in as many places in the test as you think. You may have heard that putting the couple lines into a well named private function increases readability, but it rarely does. It leads to jumping up and down in the code file, and most times it is more readable to see the code in a single block. The test shouldn’t be so long that you cannot see it in a single block.
BTW: A spare isn’t always two 5′s. So the private method doesn’t really describe the whole picture anyways.
This test however is pointing to a couple code smells.
First we will pick on rollSpare(). rollSpare() is likely a function that should exist in Game. It is very likely that you will want to rollSpare() when you really are playing a Game and not just when you are testing a Game. As I said in a previous article: Tests should always be treated like every other consumer of the subject under test.
NOTE: If it seems inappropriate to have the rollSpare() as part of the Game interface, then consider having some other class, maybe a helper, encapsulate this functionality within your production code base.
The second private function to pick on is rollMany(). This function adds logic which violates the cyclomatic complexity and nesting level constraints in addition to the no private methods constraint specified in the PHPUnit Unit Testing Code Standard. rollMany() probably cannot be argued to become part of the Game interface or even a helper in production code. This one could be a comment on your test. As in, you have redundant tests can lead to too many test.
It might sound silly that you could possibly test too much, but you can.
When writing unit tests you should write no more than one test per each equivalence class partition. It may sound harmless to do more than that, but you will be wasting precious time. You will waste time writing the extra tests. Your peers will waste time learning those tests. You and your peers will waste time maintaining those tests. All this waste will be for nothing, as the extra tests will not add extra coverage or safety. They are redundant.
rollMany() is an indicator that you have too many tests. You should only need the following tests to test roll()
- Single Roll
- Negative Pins
- More Than 10 Pins
- Zero Pins
- Ten Pins
- A Number of Pins that is greater than 0 and less than 10
- Multiple Rolls
Uh oh! We missed a couple tests! How are we going to test that we only bowl the ten frames with the correct number of rolls.
You could copy and paste the roll() call the number of times you need to roll the ball, and the number of time you need to roll the ball plus one. You would have to do this for a game with no spares, a game with all strikes, a game with one spare, a game with two spares, a game with three spares, a game with four spares, etc. and don’t forget that in the tenth frame you might need 3 balls! Oy!
Again, this is pointing to a problem with your production code. The interface for Game is too small, or maybe it is that we are missing an object in our model.
If we introduced a Frame, that had the roll() method, possibly like this
<?php
class Frame {
private $is_final_frame;
private $rolls;
public function __construct(
$is_final_frame = false
) {
$this->is_final_frame = $is_final_frame;
$this->rolls = array();
}
public function roll($number_of_pins) {
if (($number_of_pins < 0) || ($number_of_pins > 10)) {
throw new Exception(
"Must roll 0-10 pins, you rolled: $number_of_pins");
}
if (!$this->isComplete()) {
throw new Exception("Frame is complete");
}
$this->rolls[] = $number_of_pins;
}
public function isComplete() {
switch (count($this->rolls)) {
case 0:
return false;
case 1:
return $this->isStrike();
case 2:
return ($this->isStrike() || $this->isSpare())
&& !$this->is_final_frame;
default:
return true;
}
}
public function isStrike() {
return $this->rolls[0] === 10;
}
public function isSpare() {
return !$this->isStrike()
&& ($this->rolls[0] + $this->rolls[1]) === 10;
}
public function getRoll($roll_number) {
if ($roll_number >= count($this->rolls)) {
return 0;
}
return $this->rolls[$roll_number];
}
}
?>
…then we could do all our single roll tests and our frame completion tests in a sane manner with no control structures. Our tests would be
- Single Roll
- Negative Pins
- More Than 10 Pins
- Zero Pins
- Ten Pins
- A Number of Pins that is greater than 0 and less than 10
- Multiple Rolls – Not Final Frame
- One strike
- One strike, fail on second roll
- Two rolls, sum less than 10
- A spare
- Try to roll three balls
- Multiple Rolls – Final Frame
- One strike plus two more rolls
- A spare plus one more roll
- Fail on a strike or a spare followed by enough rolls to total four
- Two rolls, sum less than 10
- Fail on three rolls, when sum of first two rolls is less than 10
Finally, we can then have Game deal with Frames instead of roll()s, and can test that the game never goes beyond ten frames, or the number of frames it is given.
Game could look something like this
<?php
class Game {
private $frames;
private $current_frame_index;
public function __construct($frames) {
$this->frames = $frames;
$this->current_frame_index = 0;
}
public function roll($number_of_pins) {
if ($this->isComplete()) {
throw new Exception("Your game is complete");
}
$this->frames[$this->current_frame_index]
->roll($number_of_pins);
if ($this->frames[$this->current_frame_index]->isComplete()) {
$this->current_frame_index++;
}
}
public function isComplete() {
$max_frames = count($this->frames);
return ($this->current_frame_index >= $max_frames)
&& $this->frames[$max_frames - 1]->isComplete();
}
public function score() {
$score = 0;
$previous_was_strike = false;
$previous_was_spare = false;
foreach ($this->frames as $frame) {
if ($previous_was_strike) {
$score += 2 * ($frame->getRoll(0) + $frame->getRoll(1));
} else if ($previous_was_spare) {
$score += (2 * $frame->getRoll(0)) + $frame->getRoll(1));
} else {
$score += $frame->getRoll(0) + $frame->getRoll(1);
}
}
$last_frame = $this->frames[count($this->frames) - 1];
$score += $last_frame->getRoll(3);
return $score;
}
}
?>
I would recommend a Game_Factory for setting you up with the number of frames you want in the Game. Partly because it is bad practice to have logic or function calls in your constructor, but mostly because injecting complex objects or pre-populated arrays allows you to get your subject under test (SUT) to a testable state faster. (Note to self: I should probably write up a full example of that in a later article or workshop.) Not to mention our Game is much easier to vary now as we could swap in a different number of Frames or even a different variety of Framess. We could eventually extract a Scorer to handle scoring different Frame combinations. After all, even though this looks like a bowling game, it wasn’t called BowlingGame.
Anyways, I hope you are not distressed by now and think that this is the only solution. There are many ways to code a solution, but hopefully this will help you think about how to look at your tests as a reflection on your production code.
