There comes a time in every codebase’s lifetime where you’re forced to admit you screwed up.
Maybe not in a fatal way, like having critical bugs or failing to fulfill a contractual requirement.
Sometimes you can do the right thing but in all the wrong ways – like running a marathon backwards. Sure, you can do that, but it’s not the most efficient or comfortable way. That’s where refactoring comes in.
Download the source code to follow along.
As I began writing the tests, and thus becoming a consumer of my own code, it became apparent that I had made some design decisions that were causing me a bit of pain during implementation.
Such as:
- PlayerTurn being the name of the player instead of just a reference to the Player
- Winner being the name of the player instead of just a reference to the Player
- The Board squares all being individual properties meant that I couldn’t use an array and an index for faster and more concise access
I’ve dealt with it enough to know that this is a real problem, and the design should be fixed.
But how can we guarantee that the code will still work after we refactor it?
This is where the beauty of having known successful tests really shines.
The process we followed in previous installment is called Red-Green-Refactor:
- Write a test for unimplemented code that fails (RED)
- Write the minimum amount of new code that makes the test pass (GREEN)
- Clean up the implementation mess and confirm the test still passes (REFACTOR)
Further reading from James Shore and Uncle Bob.
The tests were all green, so let’s refactor some of the code and then run the test again.
Here’s the current messy implementation:
public class Game
{
/// <summary>
/// The X player
/// </summary>
public Player Player1 { get; set; }
/// <summary>
/// The O player
/// </summary>
public Player Player2 { get; set; }
/// <summary>
/// The name of the player whose move it is
/// </summary>
public string PlayerTurn { get; set; }
...
And here’s one of the tests:
[TestMethod]
public void GameManager_CreateNew_should_return_a_new_game()
{
// Arrange
var gameManager = new GameManager();
// Act
var game = gameManager.CreateNew();
// Assert
Assert.IsNotNull(game);
Assert.IsNotNull(game.Player1, "game.Player1");
Assert.AreEqual('X', game.Player1.Mark, "game.Player1.Mark");
Assert.AreEqual("Player 1", game.Player1.Name, "game.Player1.Name");
Assert.IsNotNull(game.Player2, "game.Player2");
Assert.AreEqual('O', game.Player2.Mark, "game.Player2.Mark");
Assert.AreEqual("Player 2", game.Player2.Name, "game.Player2.Name");
Assert.AreEqual(game.Player1.Name, game.PlayerTurn, "game.PlayerTurn");
...
So let’s change the PlayerTurn property from a string into a Player and see what happens:
public class Game
{
/// <summary>
/// The X player
/// </summary>
public Player Player1 { get; set; }
/// <summary>
/// The O player
/// </summary>
public Player Player2 { get; set; }
/// <summary>
/// The player whose move it is
/// </summary>
public Player PlayerTurn { get; set; }
...
We have build errors now! But don’t fret, that was to be expected:
Let’s fix those (I’ll leave this as an exercise for you):
GameManager.CreateNew()– updateGame.GetCurrentPlayer()– removeGame.MakeMove()– updateGame.FinishAsVictory()– updateGame.SwitchPlayerTurn()– update
Now it compiles, so let’s run those tests again:
It looks like our change had unexpected side effects! Now to figure out what’s wrong – is it the code that we just changed, or is it the test?
Here’s the line that failed:
[TestMethod]
public void GameManager_CreateNew_should_return_a_new_game()
{
// Arrange
var gameManager = new GameManager();
// Act
var game = gameManager.CreateNew();
// Assert
Assert.IsNotNull(game);
Assert.IsNotNull(game.Player1, "game.Player1");
Assert.AreEqual('X', game.Player1.Mark, "game.Player1.Mark");
Assert.AreEqual("Player 1", game.Player1.Name, "game.Player1.Name");
Assert.IsNotNull(game.Player2, "game.Player2");
Assert.AreEqual('O', game.Player2.Mark, "game.Player2.Mark");
Assert.AreEqual("Player 2", game.Player2.Name, "game.Player2.Name");
Assert.AreEqual(game.Player1.Name, game.PlayerTurn, "game.PlayerTurn");
...
Ah, that makes sense. We are comparing the player name against a Player object now. The compiler didn’t complain because the Assert methods take in objects, not any specific type. So we’ll need to update the test:
Assert.AreEqual(game.Player1, game.PlayerTurn, "game.PlayerTurn");
And this other test in when_using_GameManager.cs while we’re at it:
[TestMethod]
public void then_PlayerTurn_should_be_Player_1()
{
Assert.AreEqual(game.Player1, game.PlayerTurn, "game.PlayerTurn");
}
And run all the tests again:
And voilà! All green again.
We’ve just completed a full Red-Green-Refactor cycle! Easy, wasn’t it?
Go ahead and refactor the other 2 issues I identified at the beginning of the post, and come back here when you’re finished.
Compare your refactoring to mine, or just download the updated source code if want to skip it.
When you’re embracing TDD, you must also embrace the idea that your test code should be treated as real code.
That is, your test code should also be of high quality! That’s not to say you should follow the exact same practices, such as DRY or SOLID – the difference is that in test code, you must value clarity over all, so some repetition is needed. The most important thing for tests is that they must clearly communicate their intent! Try to minimize the unessential aspects of the test in order to elevate the essential details.
Consider these tests:
[TestMethod]
public void Game_MakeMove_with_empty_board_should_allow_move_for_square_1()
{
var moveSuccess = game.MakeMove(1);
Assert.IsTrue(moveSuccess, nameof(moveSuccess));
Assert.AreEqual('X', game.Board[1]);
}
[TestMethod]
public void Game_MakeMove_with_empty_board_should_allow_move_for_square_2()
{
var moveSuccess = game.MakeMove(2);
Assert.IsTrue(moveSuccess, nameof(moveSuccess));
Assert.AreEqual('X', game.Board[2]);
}
[TestMethod]
public void Game_MakeMove_with_empty_board_should_allow_move_for_square_3()
{
var moveSuccess = game.MakeMove(3);
Assert.IsTrue(moveSuccess, nameof(moveSuccess));
Assert.AreEqual('X', game.Board[3]);
}
Repetition is okay, to an extent – but consider that in order to comprehensively test this functionality, we will have to repeat the test 9 times and the only thing that changes is the square number. These tests are ripe for refactoring, because think about what happens if we introduce a change that breaks these tests? We’ll have to update all 9 tests, and with so much repetition it is easy to make mistakes. So let’s refactor these 9 tests into a single, condensed test.
Here’s the optimized version:
[TestMethod]
public void Game_MakeMove_with_empty_board_should_allow_move_for_any_square()
{
var failures = new List<AssertFailedException>();
for (var square = 1; square <= 9; square++)
{
try
{
game = gameMgr.CreateNew();
var moveSuccess = game.MakeMove(square);
Assert.IsTrue(moveSuccess, nameof(moveSuccess));
Assert.AreEqual('X', game.Board[square], $"Failed for square {square}");
}
catch (AssertFailedException ex)
{
failures.Add(ex);
}
}
Assert.IsFalse(failures.Any(), $"These test cases failed:\n{string.Join("\n", failures.Select(ex => ex.Message))}");
}
Note that the highlighted portion is the exact same test as before, just with a variable instead of a hard-coded square number. All I did was add some scaffolding that allowed us to execute the test against multiple test values.
Couple other things I want to point out here:
- when iterating over a bunch of test cases, I like to be able to execute them all and collect the results, instead of stopping at the first failure – otherwise you may fix one failure only to run into another failure immediately after, and so on.
- the failure descriptions are very detailed, and tell us exactly which test case failed
For test code, even more so than “real” code, clarity and detail is key.
Go ahead and update the Game_MakeMove_with_empty_board_should_deny_move_for_square_X tests as well. (Oops looks like a typo in the method name due to a bad-copy paste! Rename it to something like Game_MakeMove_with_filled_square_should_deny_move_for_same_square).
Now consider the test Game_MakeMove_after_winning_move_should_update_fields:
[TestMethod]
public void Game_MakeMove_after_winning_move_should_update_fields()
{
// handles top across as X
var moves = new[] {1, 4, 2, 5, 3};
// iterate over the moves and test unfinished status
for (var moveIdx = 0; moveIdx < moves.Length; moveIdx++)
{
var move = moves[moveIdx];
var moveSuccess = game.MakeMove(move);
Assert.IsTrue(moveSuccess, nameof(moveSuccess));
if (moveIdx + 1 != moves.Length)
{
// not the final move
Assert.IsFalse(game.IsFinished, "game.IsFinished");
Assert.IsFalse(game.IsDraw, "game.IsDraw");
Assert.IsNotNull(game.PlayerTurn, "game.PlayerTurn");
Assert.IsNull(game.Winner, "game.Winner");
}
}
// test finished status
Assert.IsTrue(game.IsFinished, "game.IsFinished");
Assert.IsFalse(game.IsDraw, "game.IsDraw");
Assert.IsNull(game.PlayerTurn, "game.PlayerTurn");
Assert.AreEqual(game.Player1, game.Winner, "game.Winner");
}
Currently it is only testing the winning moves that result in Xs across the top row. But other than changing the move sequence and the expected winner, the test is going to be exactly the same for any other winning move. And in order to have complete logic coverage, we’ll have to do 16 of these tests (top row, middle row, bottom row, left column, middle column, right column, downright diagonal, downleft diagonal times X and O). Again, the unnecessary duplication introduces risks.
So let’s rewrite this test to handle all 8 winning cases, for both Xs and Os:
[TestMethod]
public void Game_MakeMove_after_winning_move_should_update_fields()
{
// different win scenarios: (top across, middle across, bottom across, left down, center down, right down, diagonal right, diagonal left) x (X, O) = 16 cases
var testCases = new[]
{
new Tuple<IList<int>, char?>(new[] { 1, 4, 2, 5, 3 }, 'X'),
new Tuple<IList<int>, char?>(new[] { 4, 1, 5, 2, 6 }, 'X'),
new Tuple<IList<int>, char?>(new[] { 7, 1, 8, 2, 9 }, 'X'),
new Tuple<IList<int>, char?>(new[] { 1, 2, 4, 5, 7 }, 'X'),
new Tuple<IList<int>, char?>(new[] { 2, 1, 5, 4, 8 }, 'X'),
new Tuple<IList<int>, char?>(new[] { 3, 1, 6, 4, 9 }, 'X'),
new Tuple<IList<int>, char?>(new[] { 1, 2, 5, 3, 9 }, 'X'),
new Tuple<IList<int>, char?>(new[] { 3, 2, 5, 1, 7 }, 'X'),
new Tuple<IList<int>, char?>(new[] { 7, 1, 4, 2, 5, 3 }, 'O'),
new Tuple<IList<int>, char?>(new[] { 7, 4, 1, 5, 2, 6 }, 'O'),
new Tuple<IList<int>, char?>(new[] { 4, 7, 1, 8, 2, 9 }, 'O'),
new Tuple<IList<int>, char?>(new[] { 9, 1, 2, 4, 5, 7 }, 'O'),
new Tuple<IList<int>, char?>(new[] { 9, 2, 1, 5, 4, 8 }, 'O'),
new Tuple<IList<int>, char?>(new[] { 8, 3, 1, 6, 4, 9 }, 'O'),
new Tuple<IList<int>, char?>(new[] { 4, 1, 2, 5, 3, 9 }, 'O'),
new Tuple<IList<int>, char?>(new[] { 4, 3, 2, 5, 1, 7 }, 'O'),
};
var successes = 0;
var failures = new List<AssertFailedException>();
foreach (var testCase in testCases)
{
game = gameMgr.CreateNew();
try
{
// iterate over the moves and test unfinished status
for (var moveIdx = 0; moveIdx < testCase.Item1.Count; moveIdx++)
{
var move = testCase.Item1[moveIdx];
var moveSuccess = game.MakeMove(move);
Assert.IsTrue(moveSuccess, nameof(moveSuccess));
if (moveIdx + 1 != testCase.Item1.Count)
{
// not the final move
Assert.IsFalse(game.IsFinished, "game.IsFinished");
Assert.IsFalse(game.IsDraw, "game.IsDraw");
Assert.IsNotNull(game.PlayerTurn, "game.PlayerTurn");
Assert.IsNull(game.Winner, "game.Winner");
}
}
// test finished status
Assert.IsTrue(game.IsFinished, "game.IsFinished");
Assert.IsFalse(game.IsDraw, "game.IsDraw");
Assert.IsNull(game.PlayerTurn, "game.PlayerTurn");
var player = game.Player1.Mark == testCase.Item2 ? game.Player1 : game.Player2;
Assert.AreEqual(player, game.Winner, "game.Winner");
successes++;
}
catch (AssertFailedException ex)
{
failures.Add(new AssertFailedException($"Failure \"{ex.Message}\" on test case:\n{RenderStringGameBoard(testCase.Item1)}\n", ex));
}
}
Assert.IsFalse(failures.Any(), $"{successes}/{testCases.Length} test casses passed.\nThese test cases failed:\n{string.Join("\n", failures.Select(ex => ex.Message))}");
}
Wow that just got a lot more complicated. But let’s do a quick comparison here.
The main logic of the test, lines 124-146 are pretty much the same as before. I just added the additional testcases above, and then the scaffolding around it to capture all the failures.
The RenderStringGameBoard method just does a text representation of the game board after all the moves – seeing it helps us quickly grasp the test case:
private string RenderStringGameBoard(IEnumerable<int> gameMoves)
{
// using 'V' to represent null squares because the test output is not in a fixed-width font
var filler = 'V';
var board = new[]
{
filler, filler, filler,
filler, filler, filler,
filler, filler, filler,
};
var currentMark = 'X';
foreach (var move in gameMoves)
{
board[move - 1] = currentMark;
currentMark = currentMark == 'X' ? 'O' : 'X';
}
return $"{board[0]}|{board[1]}|{board[2]}\n" +
$"{board[3]}|{board[4]}|{board[5]}\n" +
$"{board[6]}|{board[7]}|{board[8]}\n";
}
When I run the test I can see that only 2/16 cases passed, which is to be expected, since the old test only tested top row victory conditions and that’s all was implemented. Now it’s time to go back to the Game code and implement the other victory conditions. Remember to implement them one at a time, rerun the test, and see the new test case pass!
Other refactoring exercises:
Game_MakeMove_after_draw_move_should_update_fields– update it to handle a few non-winning test cases. Don’t bother striving for completeness here, as there are far more non-winning boards than winning boards. Just a few cases should suffice.- The for-loop and the try-catch for the multiple test cases can also be refactored into a handy utility method, so that we’re not unnecessarily repeating it. Create a test case execution harness and update these tests:
Game_MakeMove_with_empty_board_should_allow_move_for_any_squareGame_MakeMove_with_filled_square_should_deny_move_for_same_squareGame_MakeMove_after_winning_move_should_update_fieldsGame_MakeMove_after_draw_move_should_update_fields- This is a little advanced and assumes you have a good grasp of lambdas. If you need a hint, the method signature is
public static void Execute<TTestCase>(IEnumerable<TTestCase> testCases, Action<TTestCase> test, Func<TTestCase, Exception, Exception> exceptionWrapper)
And with that, we’re done! Stay tuned for the next installment.
If you’ve found this tutorial useful, please share it! Got suggestions or feedback? Please leave a comment – thanks!
Pingback: Tic-Tac-Tutorial Series | PhilChuang.com