Garden Race Pt 2: Adding multiple players
This post is part of a series on the (very) basics of iterative development using the example of a simple Snakes and Ladders-like game. Links to each post in the series will be added to the index page.
Um, ok, so the customers (firstborn and I) weren't overly impressed with the demo from part 1. It did help illustrate the most pressing deficiencies though:
- Doesn't support multiple players
- Doesn't have anything even remotely resembling snakes or ladders, let alone fairies
- No gui
Our story list currently looks like this:
A player can roll the die, and then move that many spaces along the board.- A player that ends his or her turn on a "feature square" (a square containing a creature or obstacle), will be moved to the square connected with that feature.
- There can be 1-4 players, and each player has their turn in sequence.
A player that reaches the final square wins the game.
There are no GUI stories currently defined, but we'll need something resembling a GUI eventually. I am tempted to start working on some GUI stories, because I don't want to get too far through the code and then find that it won't play nice with a graphical interface. It is really the main point of this software after all. On the other hand, the Game class is still very basic, so maybe it would be a good idea to knock over one of the original stories on our list. Story 2 seems like it could tie in with the GUI pretty strongly -- the view will have to show the player's move, then perform some kind of animation in the event that the player lands on a feature square. How about Story 3? Pretty basic, and essential to the game. At least that way firstborn and I can race each other to the end of the command line demo :)
I'll just check with the customers... be right back.
Ok, customer is asleep, so I'll take that as "that's fine Dad" :)
Quick aside, normally we would estimate "points" or some other unit of how much effort each story would take, and how many units could be done in an iteration, then have the customer prioritise the stories for this iteration.
Our test list
What tests could we write for multiple players?
- Should be able to set the number of players for a new game
- For a new, 2 player game, both player's should be off-the-board (square 0)
- After first player's roll, current player should be player 2
- After first player's roll, current player's position should be off-the-board (square 0)
- First player rolls a 3, second player rolls a 2, then current player should be player 1 on square 3.
Frankly, I don't really like how these look. Implementation details keep coming to mind, and I want to ignore them and focus on what I want to achieve (not how I want to achieve it). Let's start with these anyway and we'll see how it goes.
Starting iteration 2
Running all our tests shows we are all green, and all good to go. The first test on the list looks easy -- set the number of players in the game. Let's do that one.
//From GameSpec.cs
[Fact]
public void Should_be_able_to_create_2_player_game() {
var twoPlayerGame = new Game(10, 2);
Assert.Equal(2, twoPlayerGame.NumberOfPlayers);
}
//From Game.cs
public int NumberOfPlayers { get { return numberOfPlayers; } }
public Game(int boardSize) {
this.boardSize = boardSize;
}
public Game(int boardSize, int numberOfPlayers) {
this.boardSize = boardSize;
this.numberOfPlayers = numberOfPlayers;
}
What about the previous constructor that just takes the boardSize? Well that should probably just start a new one player game I guess. Let's write a test for how we think it should work.
[Fact]
public void New_game_should_have_1_player_by_default() {
var onePlayerGame = new Game(10);
Assert.Equal(1, onePlayerGame.NumberOfPlayers);
}
This fails because the number of players initialises to zero.
public Game(int boardSize) {
this.boardSize = boardSize;
this.numberOfPlayers = 1;
}
Fixed. Now let's look at test 2, checking the position of each player for a new game.
//From GameSpec.cs:
[Fact]
public void New_game_should_start_all_players_off_the_board() {
var newThreePlayerGame = new Game(10, 3);
var players = new[] {1, 2, 3};
foreach (var player in players) {
Assert.Equal(0, newThreePlayerGame.GetSquareFor(player));
}
}
//From Game.cs:
public int GetSquareFor(int player) {
return 0;
}
An obviously deficient implementation like this GetSquareFor() method suggests we need to writes some more tests to flesh out a better one.
[Fact]
public void Positions_should_be_correct_after_first_two_players_roll() {
var threePlayerGame = new Game(10, 3);
const int firstRoll = 3;
const int secondRoll = 5;
threePlayerGame.Roll(firstRoll);
threePlayerGame.Roll(secondRoll);
Assert.Equal(firstRoll, threePlayerGame.GetSquareFor(1));
Assert.Equal(secondRoll, threePlayerGame.GetSquareFor(2));
Assert.Equal(0, threePlayerGame.GetSquareFor(3));
}
Now we are potentially looking at a bigger step. We need Roll() to affect only the position of the current player. We don't have the concept of a current player. We'll also probably need an array or similar structure to store each player's position. Roll() will then store update the position of the current player, and change the current player to the next player. The CurrentSquare implementation will probably need to change to refer to the current player too. And then we'll have to add code to change the position in the event the player lands on a feature square! Argh!
Stop worrying! Try baby steps...
Let's back up a bit. I'm fairly confident we can write up the code above, but it will only be covered by one test and we are touching a lot of the Game class without direct guidance from the tests. We still have this test on our test list: "After first player's roll, current player should be player 2". This deals with the concept of the current player without requiring addition position arrays. It should only affect the Roll() implementation. Let's skip our last test by updating the attribute to [Fact(Skip="Too big a step for now")] (I could delete the test and rewrite it if we need it, but it did illustrate our need to deal with the current player concept, so I'll leave it for now). I started off coding our new test with two separate assertions:
var newTwoPlayerGame = new Game(10, 2); Assert.Equal(1, newTwoPlayerGame.CurrentPlayer); newTwoPlayerGame.Roll(2); Assert.Equal(2, newTwoPlayerGame.CurrentPlayer);
The split asserts are ugly, and we can split this into two more specific tests. Here's the passing code, which was written one step at a time (not shown is chaining the Game(int) constructor to Game(int, int), so everything gets initialised properly in either case. Check the download at the end for the finished code):
//From GameSpec.cs:
[Fact]
public void Current_player_for_new_game_should_be_player_1() {
var newTwoPlayerGame = new Game(10, 2);
Assert.Equal(1, newTwoPlayerGame.CurrentPlayer);
}
[Fact]
public void After_first_players_roll_it_should_be_the_second_players_turn() {
var newTwoPlayerGame = new Game(10, 2);
newTwoPlayerGame.Roll(2);
Assert.Equal(2, newTwoPlayerGame.CurrentPlayer);
}
//From Game.cs:
public int CurrentPlayer { get; private set; }
public Game(int boardSize, int numberOfPlayers) {
this.boardSize = boardSize;
this.numberOfPlayers = numberOfPlayers;
this.playerPositions = new int[numberOfPlayers];
this.CurrentPlayer = 1;
}
public void Roll(int dieValue) {
CurrentSquare += dieValue;
CurrentPlayer++;
}
Before we go back to the test we skipped, I'd like to flesh out more of the CurrentPlayer property. Fifth test on our list was "First player rolls a 3, second player rolls a 2, then current player should be player 1 on square 3". Let's do a simpler version and just verify that this scenario ends up with the correct CurrentPlayer.
//In GameSpec.cs:
[Fact]
public void After_all_players_have_had_a_turn_it_should_be_first_players_turn_again() {
var newThreePlayerGame = new Game(10, 3);
newThreePlayerGame.Roll(1);
newThreePlayerGame.Roll(1);
newThreePlayerGame.Roll(1);
Assert.Equal(1, newThreePlayerGame.CurrentPlayer);
}
//In Game.cs:
public void Roll(int dieValue) {
CurrentSquare += dieValue;
CurrentPlayer++;
if (CurrentPlayer > NumberOfPlayers) CurrentPlayer = 1;
}
A quick refactor from the green bar
We now have a green bar (well, yellow if you count the skipped test I guess). Time to take a look for potential refactoring opportunities. I've been a bit slack about this up to now as I haven't noticed anything obvious while coding and haven't explicitly stopped to think about refactoring. I have a bad habit of doing this -- I do design work while writing the code to pass the test, rather than deferring it to the refactoring stage. This can lead me to generalising to early or changing the design without getting clear direction from the tests and passing implementation. Note to self: premature generalisation is one of the many roots of all evil :)
In this case we've done the Right ThingTM and written simple code to pass the test, then looked at refactoring based on what the current implementation needs, rather than what we think it will need. The code within Roll() contains the logic for selecting the next player as well as for updating the current square. Let's Extract Method to make this more obvious.
public void Roll(int dieValue) {
CurrentSquare += dieValue;
nextPlayer();
}
private void nextPlayer() {
CurrentPlayer++;
if (CurrentPlayer > NumberOfPlayers) CurrentPlayer = 1;
}
This is purely a matter of taste. I find it reflects the intention more. If you don't, then leave it un-refactored. :-) Either way, key lesson here (for me, you probably know it already :)) is to defer design stuff until the production code shows a clear need, or until a test is too hard to write. Either way, design from the green bar whenever possible.
Time to face the music...
We probably shouldn't put it off any longer. Let's re-enable the test we skipped earlier:
//In GameSpec.cs:
[Fact]
public void Positions_should_be_correct_after_first_two_players_roll() {
var threePlayerGame = new Game(10, 3);
const int firstRoll = 3;
const int secondRoll = 5;
threePlayerGame.Roll(firstRoll);
threePlayerGame.Roll(secondRoll);
Assert.Equal(firstRoll, threePlayerGame.GetSquareFor(1));
Assert.Equal(secondRoll, threePlayerGame.GetSquareFor(2));
Assert.Equal(0, threePlayerGame.GetSquareFor(3));
}
This give the following assertion failure:
TestCase 'DaveSquared.GardenRace.Tests.GameSpec.Positions_should_be_correct_after_first_two_players_roll' failed: Assert.Equal() Failure Expected: 3 Actual: 0
This is failing because our implementation for GetSquareFor(...) stinks -- it's just returning 0. My fault, not yours. Let's get back to the green bar as soon as possible, then we'll worry about getting the design right. My original guess for passing this test was that we would need an array of player positions, and Roll() would just update the position for the current player. We have a current player concept in the code now, so let's chuck in an array and see how it goes:
//Bits and pieces from Game.cs:
public class Game {
//...
private readonly int[] playerPositions;
//...
public Game(int boardSize, int numberOfPlayers) {
this.boardSize = boardSize;
this.numberOfPlayers = numberOfPlayers;
this.playerPositions = new int[numberOfPlayers];
this.CurrentPlayer = 1;
}
public void Roll(int dieValue) {
CurrentSquare += dieValue;
playerPositions[CurrentPlayer - 1] = CurrentSquare;
nextPlayer();
}
//...
public int GetSquareFor(int player) {
return playerPositions[player - 1];
}
}
Running this fails with a new message:
TestCase 'DaveSquared.GardenRace.Tests.GameSpec.Positions_should_be_correct_after_first_two_players_roll' failed: Assert.Equal() Failure Expected: 5 Actual: 8
It is failing on our second assertion, Assert.Equal(secondRoll, threePlayerGame.GetSquareFor(2));. This gives us a good hint as to what's happening -- our first and second rolls of 3 and 5 are both being added to the same array index. Hold on, that's only half the story. Let's have a closer look at this:
public void Roll(int dieValue) {
CurrentSquare += dieValue;
playerPositions[CurrentPlayer - 1] = CurrentSquare;
nextPlayer();
}
Brilliant Dave. What a fantastic coder I am :) I'm still adding all rolls to CurrentSquare, then assigning that to the current player position. In this case, CurrentSquare is 3+5=8 which fails our test. Why didn't you point this out? Luckily I had my tests to do it in your absence :) We'll fix it right now:
public void Roll(int dieValue) {
CurrentSquare += dieValue;
playerPositions[CurrentPlayer - 1] += dieValue;
nextPlayer();
}
Tests pass. My own ineptitude aside, this test that was initially giving us troubles has been trivial to solve. We now have a green bar, so let's refactor. First obvious bit of duplication is the two square increments in the Roll() method. Let's update CurrentSquare to use our GetSquareFor(int player) method:
public int CurrentSquare {
get { return GetSquareFor(CurrentPlayer); }
}
//...
public void Roll(int dieValue) {
playerPositions[CurrentPlayer - 1] += dieValue;
nextPlayer();
}
CurrentSquare is no longer a getter/setter, but is a convenient shorthand for GetSquareFor(int player). Our Roll() method is fairly clear. We could make it a little bit clearer by extracting a moveCurrentPlayer(int squares) method, but would also mean more indirection. I'll make you a deal, if we end up with a few playerPositions[CurrentPlayer - 1] style array indexes (the -1 is pretty ugly), then we do something about it. For now it is probably ok.
Looking at Game from the perspective of the S.O.L.I.D. principles, I think the main one we have to be wary of is violating the Single Responsibility Principle (SRP). Current Game is responsible for:
- Number of squares on the board
- Number of players
- Keeping track of whose turn it is
- Keeping track of each player's position
- Knowing when the game has finished
Are these cohesive enough to count under the banner of one responsibility? I'm not sure, but I can't currently think of a better abstraction (feel free to leave comments :-)). For now let's stay conscious of this and we'll revisit it if it starts becoming a problem.
We're done? No we're not!
So are we done for our multiple players story? After running a larger test it appears so. However, after updating the demo we see some strange behaviour. An extract of from the demo code and the output are shown below:
//From main() in Program.cs:
const int numberOfSquares = 20;
const int numberOfPlayers = 2;
//...
Console.WriteLine("Creating a new game with " + numberOfSquares + " squares and " + numberOfPlayers + " players.");
var game = new Game(numberOfSquares, numberOfPlayers);
Console.WriteLine("Press any key to roll the die.");
while (!game.IsFinished) {
Console.ReadKey(interceptKey);
var dieRoll = GetDieValue();
game.Roll(dieRoll);
var currentState = (game.IsFinished) ? "You won the game!" : "Now on square " + game.CurrentSquare;
Console.WriteLine("Player " + game.CurrentPlayer + " rolled a " + dieRoll + ". " + currentState);
}
//...
Creating a new game with 20 squares and 2 players. Press any key to roll the die. Player 2 rolled a 1. Now on square 0 Player 1 rolled a 3. Now on square 1 Player 2 rolled a 2. Now on square 3 ...snip... Player 2 rolled a 2. Now on square 18 Player 1 rolled a 2. Now on square 14 Player 2 rolled a 3. You won the game!
Why does player 2 have the first go? And why do they roll a 1 and yet are on square 0? The reason is highlighted in the code snippet above -- the Roll() method updates the Game so that it is the next player's turn. When the demo gets to writing out the current player's position and state, it is actually writing the details for the next player who is about to have their turn.
I believe this is a form of temporal coupling, or coupling in time. The CurrentSquare and CurrentPlayer methods depend on whether they are called before or after Roll. There is almost certainly going to be a problem with IsFinished as well, as that depends on the CurrentPlayer too.
I've been worried about this for a little while actually, especially when thinking about how the GUI will probably want to animate the initial move, then an additional move up or down the snake/ladder/critter type thing. It looks like we'll either have to trigger events from Roll, or break it up and have some kind of coordinator object.
I am quietly confident that we can change our design to support this when we get some tests around the "feature square" or GUI stories, and that these tests will drive us toward a decent design and implementation. Again, let's stay conscious of this coupling and work around it in our demo code. Remember that our demo code is not production code, so we are being a little less precious with it. Next iteration we'll make sure we write tests to cover this and drive a nicer design.
Here's the new main loop in the demo code:
while (!game.IsFinished) {
var currentPlayer = game.CurrentPlayer;
Console.ReadKey(interceptKey);
var dieRoll = GetDieValue();
game.Roll(dieRoll);
var currentState = (game.IsFinished) ? "You won the game!" : "Now on square " + game.GetSquareFor(currentPlayer);
Console.WriteLine("Player " + currentPlayer + " rolled a " + dieRoll + ". " + currentState);
}
Creating a new game with 20 squares and 2 players. Press any key to roll the die. Player 1 rolled a 5. Now on square 5 Player 2 rolled a 3. Now on square 3 (...snip...) Player 2 rolled a 1. Now on square 10 Player 1 rolled a 5. Now on square 23 Player 2 rolled a 5. You won the game!
As predicted, our IsFinished property is having problems because of the coupling issue. We'll fix this, then we're done.
Test-first debugging
Let's write a test to expose the bug with the IsFinished implementation:
[Fact]
public void Game_should_finish_as_soon_as_any_player_reaches_the_end() {
var threePlayerGame = new Game(5, 3);
threePlayerGame.Roll(1);
threePlayerGame.Roll(6);
Assert.True(threePlayerGame.IsFinished);
}
This fails. At the time the assertion is evaluated player 2 has finished, but the current player is player 3, who is yet to finish. This should be straight-forward to pass from here:
public bool IsFinished {
get {
return playerPositions.Any(square => square >= boardSize);
}
}
All the tests now pass. We've now removed the temporal coupling between Roll and IsFinished, and our demo now works as expected:
Creating a new game with 20 squares and 2 players. Press any key to roll the die. Player 1 rolled a 1. Now on square 1 Player 2 rolled a 1. Now on square 1 (...snip...) Player 1 rolled a 2. Now on square 17 Player 2 rolled a 2. Now on square 15 Player 1 rolled a 3. You won the game! Press a key to exit.
That's a wrap!
Not sure if this looks like a lot of work to you, but it actually represents only a couple of minutes of coding time, and a couple of minutes of thinking time (plus lots of time to write it all down in excruciating detail :-)). Point is that each iteration so far has been really quick despite (or because of?) involving lots of small steps.
You can browse or checkout the final code for this iteration from here. Thanks for making it this far! :)
