Last active
October 31, 2018 05:50
-
-
Save WorryingWonton/5696abf28cac310a589b3a2717673887 to your computer and use it in GitHub Desktop.
New method of selecting the next active player, which does not require any refactoring of the Monopoly code outside of monopoly.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| import unittest | |
| from itertools import cycle | |
| """ | |
| There's a very bad pattern in my code with having to run a filter operation in two separate methods on Game.players | |
| How do I fix this? (See lines 16 and 30) | |
| """ | |
| class Game: | |
| def __init__(self): | |
| self.all_players = [] | |
| self.players = [] | |
| self.active_player = None | |
| def remove_player(self, player): | |
| player.in_game = False | |
| if player != self.active_player: | |
| self.players = list(filter(lambda player: player.in_game, self.all_players)) | |
| def add_player(self, player): | |
| self.all_players.append(player) | |
| def set_active_player(self): | |
| if not self.active_player: | |
| self.active_player = self.all_players[0] | |
| else: | |
| player_iterator = cycle(self.players) | |
| while next(player_iterator) != self.active_player: | |
| pass | |
| self.active_player = next(player_iterator) | |
| self.players = list(filter(lambda player: player.in_game, self.all_players)) | |
| #Based on a suggestion from Dale | |
| # def set_active_player(self): | |
| # if not self.active_player: | |
| # self.active_player = self.all_players[0] | |
| # else: | |
| # index = self.players.index(self.active_player) | |
| # if index == len(self.players) - 1: | |
| # index = 0 | |
| # self.active_player = self.players[index] | |
| # else: | |
| # self.active_player = self.players[index + 1] | |
| # self.players = list(filter(lambda player: player.in_game, self.all_players)) | |
| class Player: | |
| def __init__(self, name): | |
| self.name = name | |
| self.in_game = True | |
| class TestPlayerRemovalCases(unittest.TestCase): | |
| def test_next_player_removal(self): | |
| my_game = Game() | |
| my_game.add_player(Player('David')) | |
| my_game.add_player(Player('Sara')) | |
| my_game.add_player(Player('Alan')) | |
| my_game.add_player(Player('Mark')) | |
| my_game.set_active_player() | |
| self.assertEqual('David', my_game.active_player.name) | |
| my_game.set_active_player() | |
| self.assertEqual('Sara', my_game.active_player.name) | |
| my_game.remove_player(my_game.all_players[2]) | |
| self.assertEqual(3, len(my_game.players)) | |
| my_game.set_active_player() | |
| self.assertEqual(3, len(my_game.players)) | |
| self.assertEqual('Mark', my_game.active_player.name) | |
| my_game.set_active_player() | |
| self.assertEqual('David', my_game.active_player.name) | |
| def test_active_player_removal(self): | |
| my_game = Game() | |
| my_game.add_player(Player('David')) | |
| my_game.add_player(Player('Sara')) | |
| my_game.add_player(Player('Alan')) | |
| my_game.add_player(Player('Mark')) | |
| my_game.set_active_player() | |
| self.assertEqual('David', my_game.active_player.name) | |
| my_game.remove_player(my_game.all_players[0]) | |
| self.assertEqual(4, len(my_game.players)) | |
| my_game.set_active_player() | |
| self.assertEqual(3, len(my_game.players)) | |
| self.assertEqual('Sara', my_game.active_player.name) | |
| my_game.set_active_player() | |
| self.assertEqual('Alan', my_game.active_player.name) | |
| my_game.set_active_player() | |
| self.assertEqual('Mark', my_game.active_player.name) | |
| my_game.set_active_player() | |
| self.assertEqual('Sara', my_game.active_player.name) | |
| def test_player_removal_at_prior_index(self): | |
| my_game = Game() | |
| my_game.add_player(Player('David')) | |
| my_game.add_player(Player('Sara')) | |
| my_game.add_player(Player('Alan')) | |
| my_game.add_player(Player('Mark')) | |
| my_game.set_active_player() | |
| my_game.set_active_player() | |
| my_game.remove_player(my_game.all_players[0]) | |
| #The next active player should be Alan if David is removed, and the current active player is Sara | |
| my_game.set_active_player() | |
| self.assertEqual('Alan', my_game.active_player.name) | |
| def test_player_removal_at_arbitrary_forward_index(self): | |
| my_game = Game() | |
| my_game.add_player(Player('David')) | |
| my_game.add_player(Player('Sara')) | |
| my_game.add_player(Player('Alan')) | |
| my_game.add_player(Player('Mark')) | |
| my_game.add_player(Player('Bill')) | |
| my_game.add_player(Player('Bob')) | |
| my_game.set_active_player() | |
| my_game.remove_player(my_game.players[3]) | |
| self.assertEqual('David', my_game.active_player.name) | |
| my_game.set_active_player() | |
| self.assertEqual('Sara', my_game.active_player.name) | |
| my_game.set_active_player() | |
| self.assertEqual('Alan', my_game.active_player.name) | |
| my_game.set_active_player() | |
| self.assertEqual('Bill', my_game.active_player.name) | |
| my_game.set_active_player() | |
| self.assertEqual('Bob', my_game.active_player.name) | |
| my_game.set_active_player() | |
| def test_player_removal_at_arbitrary_prior_index(self): | |
| my_game = Game() | |
| my_game.add_player(Player('David')) | |
| my_game.add_player(Player('Sara')) | |
| my_game.add_player(Player('Alan')) | |
| my_game.add_player(Player('Mark')) | |
| my_game.add_player(Player('Bill')) | |
| my_game.add_player(Player('Bob')) | |
| my_game.set_active_player() | |
| my_game.set_active_player() | |
| my_game.set_active_player() | |
| self.assertEqual('Alan', my_game.active_player.name) | |
| my_game.remove_player(my_game.all_players[0]) | |
| my_game.set_active_player() | |
| self.assertEqual('Mark', my_game.active_player.name) | |
| my_game.set_active_player() | |
| self.assertEqual('Bill', my_game.active_player.name) | |
| my_game.set_active_player() | |
| self.assertEqual('Bob', my_game.active_player.name) | |
| my_game.set_active_player() | |
| self.assertEqual('Sara', my_game.active_player.name) | |
| def test_player_removal_at_last_index_in_players(self): | |
| my_game = Game() | |
| my_game.add_player(Player('David')) | |
| my_game.add_player(Player('Sara')) | |
| my_game.add_player(Player('Alan')) | |
| my_game.add_player(Player('Mark')) | |
| my_game.set_active_player() | |
| my_game.remove_player(my_game.all_players[-1]) | |
| my_game.set_active_player() | |
| self.assertEqual('Sara', my_game.active_player.name) | |
| my_game.remove_player(my_game.all_players[2]) | |
| my_game.set_active_player() | |
| self.assertEqual('David', my_game.active_player.name) | |
| if __name__ == '__main__': | |
| unittest.main() |
Author
I refactored the while loop per your suggestion and renamed Player.active to Player.in_game to make things more clear.
I gather the self.players = list(filter(lambda player: player.in_game, self.all_players)) in remove_player is to make it so that set_active_player doesn't advance active_player to a player who has been ejected from the game. An alternative would be to add a second loop in set_active_player which advances through the player list until a non-ejected player is found. Two ways of doing it:
A second while loop:
while True:
self.active_player = next(player_iterator)
if self.active_player.in_game:
break
Replace the current while loop with a double-nested for loop:
for p in player_iterator:
# advance to the active player (from last turn)
if p == self.active_player:
for p in player_iterator:
# advance to the next player that's still in-game
if p.in_game:
self.active_player = p
break
break
OR! Replace the entire body of the else with:
self.active_player = next(dropwhile(lambda p: not p.in_game, dropwhile(lambda p: p != self.active_player, cycle(self.players))))
Try adding these test cases:
def test_cycle_three_players(self):
g = Game()
g.add_player(Player('Sim'))
g.add_player(Player('Bob'))
g.add_player(Player('Val'))
# turn 1
g.set_active_player()
self.assertEqual('Sim', g.active_player.name)
# turn 2
g.set_active_player()
self.assertEqual('Bob', g.active_player.name)
# turn 3
g.set_active_player()
self.assertEqual('Val', g.active_player.name)
# turn 4
g.set_active_player()
self.assertEqual('Sim', g.active_player.name)
# turn 5
g.set_active_player()
self.assertEqual('Bob', g.active_player.name)
# turn 6
g.set_active_player()
self.assertEqual('Val', g.active_player.name)
def test_cycle_two_players(self):
g = Game()
g.add_player(Player('Sim'))
g.add_player(Player('Bob'))
# turn 1
g.set_active_player()
self.assertEqual('Sim', g.active_player.name)
# turn 2
g.set_active_player()
self.assertEqual('Bob', g.active_player.name)
# turn 3
g.set_active_player()
self.assertEqual('Sim', g.active_player.name)
# turn 4
g.set_active_player()
self.assertEqual('Bob', g.active_player.name)
def test_two_players_remove_first_on_first_turn(self):
g = Game()
g.add_player(Player('Sim'))
g.add_player(Player('Bob'))
# turn 1
g.set_active_player()
self.assertEqual('Sim', g.active_player.name)
g.remove_player(g.all_players[0])
# turn 2
g.set_active_player()
self.assertEqual('Bob', g.active_player.name)
# turn 3
g.set_active_player()
self.assertEqual('Bob', g.active_player.name)
# turn 4
g.set_active_player()
self.assertEqual('Bob', g.active_player.name)
def test_three_players_remove_first_on_first_turn(self):
g = Game()
g.add_player(Player('Sim'))
g.add_player(Player('Bob'))
g.add_player(Player('Val'))
# turn 1
g.set_active_player()
self.assertEqual('Sim', g.active_player.name)
g.remove_player(g.all_players[0])
# turn 2
g.set_active_player()
self.assertEqual('Bob', g.active_player.name)
# turn 3
g.set_active_player()
self.assertEqual('Val', g.active_player.name)
# turn 4
g.set_active_player()
self.assertEqual('Bob', g.active_player.name)
# turn 5
g.set_active_player()
self.assertEqual('Val', g.active_player.name)
Simplified Dale's with modulo:
index = self.players.index(self.active_player)
self.active_player = self.players[(index + 1) % len(self.players)]
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
I believe you can rewrite/simplify this to: