Game:l50-53 shows an example of Data Clump: these are the players related data, and each index relates to a single player. Such indices need to be consistent across the arrays, which can eventually lead to bugs.
This commit introduces a new feature: now players can leave a game while running.
As outlined in the tests from this commit, removing a player from the Game::players array will mismatch the remaining players with the recorded data.
While this particular bug could be fixed by setting back the correct data in the Game::remove() method, now any changes on these data, or inclusion of a new type of data (for example, players may be assigned a color), would then need to be performed both in the Game::add() and Game::remove() methods. This might not be obvious to a new programmer joining the project.
Introducing a data holding class
The solution would be to group all the data referring to a same player in a dedicated data holding instance, representing this player.
To perform this operation safely, a first step is to encapsulate fields for Game::players, Game::places, Game::purses and Game::inPenaltyBox.
With all the data access encapsulated, we can nowmove these fields to the Player class. The Game::players will now hold references to Player instances in order to access this data.
Fixing the remove method
This refactor could have been stopped there, but the following commits outlines two new bugs that may occur when implementing the Game::remove() method:
We can benefits from our players being instances here to use a reference to the current player, instead of an integer referring to its position in an array.
Cleaning up
This could also bring an end to this refactoring, although, the encapsulation of these change of the current player offers us a good opportunity to enforce separation of concerns, by moving all the code related to selecting the current player in a dedicated collection class
This in turn gives us the opportunity to level down all of the written tests to the PlayersList class scope, where the player removal feature belongs.
Player Data Clump
The issue
Game:l50-53shows an example of Data Clump: these are the players related data, and each index relates to a single player. Such indices need to be consistent across the arrays, which can eventually lead to bugs.This commit introduces a new feature: now players can leave a game while running.
As outlined in the tests from this commit, removing a player from the
Game::playersarray will mismatch the remaining players with the recorded data.While this particular bug could be fixed by setting back the correct data in the
Game::remove()method, now any changes on these data, or inclusion of a new type of data (for example, players may be assigned a color), would then need to be performed both in theGame::add()andGame::remove()methods. This might not be obvious to a new programmer joining the project.Introducing a data holding class
The solution would be to group all the data referring to a same player in a dedicated data holding instance, representing this player.
To perform this operation safely, a first step is to encapsulate fields for
Game::players,Game::places,Game::pursesandGame::inPenaltyBox.With all the data access encapsulated, we can nowmove these fields to the
Playerclass. TheGame::playerswill now hold references toPlayerinstances in order to access this data.Fixing the remove method
This refactor could have been stopped there, but the following commits outlines two new bugs that may occur when implementing the
Game::remove()method:-If a player leaves, the current player may be changed
We can benefits from our players being instances here to use a reference to the current player, instead of an integer referring to its position in an array.
Cleaning up
This could also bring an end to this refactoring, although, the encapsulation of these change of the current player offers us a good opportunity to enforce separation of concerns, by moving all the code related to selecting the current player in a dedicated collection class
This in turn gives us the opportunity to level down all of the written tests to the
PlayersListclass scope, where the player removal feature belongs.