A game of automated tag

Clash Royale CLAN TAG#URR8PPP
.everyoneloves__top-leaderboard:empty,.everyoneloves__mid-leaderboard:empty margin-bottom:0;
up vote
7
down vote
favorite
This game has 4 people moving randomly, one unit in one of the four directions (up, down, left, right), with a fifth person as a designated tagger. If this fifth person tags someone, the person is out. Game ends when there is one survivor left.
import random
class Player:
def __init__(self, name, status, speed, health, position):
self.name = name
self.status = status
self.speed = speed
self.health = health
self.position = position
def move(self, arena_limits, player_pos):
change = "left": -1, "right": 1, "up": 1, "down": -1
direction = random.choice(list(change.keys()))
if not self.is_stuck(arena_limits, player_pos):
if direction in ("left", "right"):
in_limits = 0 <= self.position[0] + (change[direction] * self.speed) <= arena_limits[0]
if in_limits and [self.position[0] + (change[direction] * self.speed), self.position[1]] not in player_pos:
self.position[0] += change[direction] * self.speed
else:
self.move(arena_limits, player_pos)
if direction in ("up", "down"):
in_limits = 0 <= self.position[1] + (change[direction] * self.speed) <= arena_limits[0]
if in_limits and [self.position[0], self.position[1] + (change[direction] * self.speed)] not in player_pos:
self.position[1] += change[direction] * self.speed
else:
self.move(arena_limits, player_pos)
else:
pass
def is_stuck(self, arena_limits, player_pos):
can_go_left = ([self.position[0] + (-1 * self.speed), self.position[1]] not in player_pos) and
0 <= self.position[0] + (-1 * self.speed) <= arena_limits[0]
can_go_right = ([self.position[0] + (1 * self.speed), self.position[1]] not in player_pos) and
0 <= self.position[0] + (1 * self.speed) <= arena_limits[0]
can_go_up = ([self.position[0], self.position[1] + (1 * self.speed)] not in player_pos) and
0 <= self.position[1] + (1 * self.speed) <= arena_limits[1]
can_go_down = ([self.position[0], self.position[1] + (-1 * self.speed)] not in player_pos) and
0 <= self.position[1] + (-1 * self.speed) <= arena_limits[1]
if not (can_go_up or can_go_down or can_go_left or can_go_right):
return True
else:
return False
def tag(self, player_list):
if self.status == "IT":
done = False
turns = 0
while not done:
turns += 1
player_pos = [user.position for user in player_list] + [self.position]
for player in player_list:
player.move([8, 8], player_pos)
above_or_under = player.position[0] in (self.position[0] + 1, self.position[0] - 1)
left_or_right = player.position[1] in (self.position[1] + 1, self.position[1] - 1)
if above_or_under or left_or_right:
del player_list[player_list.index(player)]
print(player.name + " has been eliminated after " + str(turns) + " turn(s)!")
if len(player_list) == 1:
print(player_list[0].name + " is the winner!")
print("-" * 20)
done = True
self.move([8, 8], player_pos)
tagger = Player("TAGGER", "IT", 1, 0, [8, 8])
for b in range(10000):
victims =
for i in range(4):
victims.append(Player("Player" + str(i), "", 1, 0, [random.randint(0, 6), random.randint(0, 6)]))
tagger.tag(victims)
Some quick comments about the code:
- Comments will be added to my code soon
self.healthis currently not used but is planned on being used- The field is
8x8and each person can move once in one of the four directions. If they are stuckis_stuck(), they will simply not move - A player is out if they are directly above, next to, or below the tagger. Diagonally does not count
Main question here, can I shorten the code and can I make it more Pythonic?
python object-oriented python-3.x simulation
add a comment |Â
up vote
7
down vote
favorite
This game has 4 people moving randomly, one unit in one of the four directions (up, down, left, right), with a fifth person as a designated tagger. If this fifth person tags someone, the person is out. Game ends when there is one survivor left.
import random
class Player:
def __init__(self, name, status, speed, health, position):
self.name = name
self.status = status
self.speed = speed
self.health = health
self.position = position
def move(self, arena_limits, player_pos):
change = "left": -1, "right": 1, "up": 1, "down": -1
direction = random.choice(list(change.keys()))
if not self.is_stuck(arena_limits, player_pos):
if direction in ("left", "right"):
in_limits = 0 <= self.position[0] + (change[direction] * self.speed) <= arena_limits[0]
if in_limits and [self.position[0] + (change[direction] * self.speed), self.position[1]] not in player_pos:
self.position[0] += change[direction] * self.speed
else:
self.move(arena_limits, player_pos)
if direction in ("up", "down"):
in_limits = 0 <= self.position[1] + (change[direction] * self.speed) <= arena_limits[0]
if in_limits and [self.position[0], self.position[1] + (change[direction] * self.speed)] not in player_pos:
self.position[1] += change[direction] * self.speed
else:
self.move(arena_limits, player_pos)
else:
pass
def is_stuck(self, arena_limits, player_pos):
can_go_left = ([self.position[0] + (-1 * self.speed), self.position[1]] not in player_pos) and
0 <= self.position[0] + (-1 * self.speed) <= arena_limits[0]
can_go_right = ([self.position[0] + (1 * self.speed), self.position[1]] not in player_pos) and
0 <= self.position[0] + (1 * self.speed) <= arena_limits[0]
can_go_up = ([self.position[0], self.position[1] + (1 * self.speed)] not in player_pos) and
0 <= self.position[1] + (1 * self.speed) <= arena_limits[1]
can_go_down = ([self.position[0], self.position[1] + (-1 * self.speed)] not in player_pos) and
0 <= self.position[1] + (-1 * self.speed) <= arena_limits[1]
if not (can_go_up or can_go_down or can_go_left or can_go_right):
return True
else:
return False
def tag(self, player_list):
if self.status == "IT":
done = False
turns = 0
while not done:
turns += 1
player_pos = [user.position for user in player_list] + [self.position]
for player in player_list:
player.move([8, 8], player_pos)
above_or_under = player.position[0] in (self.position[0] + 1, self.position[0] - 1)
left_or_right = player.position[1] in (self.position[1] + 1, self.position[1] - 1)
if above_or_under or left_or_right:
del player_list[player_list.index(player)]
print(player.name + " has been eliminated after " + str(turns) + " turn(s)!")
if len(player_list) == 1:
print(player_list[0].name + " is the winner!")
print("-" * 20)
done = True
self.move([8, 8], player_pos)
tagger = Player("TAGGER", "IT", 1, 0, [8, 8])
for b in range(10000):
victims =
for i in range(4):
victims.append(Player("Player" + str(i), "", 1, 0, [random.randint(0, 6), random.randint(0, 6)]))
tagger.tag(victims)
Some quick comments about the code:
- Comments will be added to my code soon
self.healthis currently not used but is planned on being used- The field is
8x8and each person can move once in one of the four directions. If they are stuckis_stuck(), they will simply not move - A player is out if they are directly above, next to, or below the tagger. Diagonally does not count
Main question here, can I shorten the code and can I make it more Pythonic?
python object-oriented python-3.x simulation
You said thathealthis reserved for future improvements, but there is no real point in havingspeedeither. Do you plan to use different values in the future too?
â Mathias Ettinger
Aug 3 at 7:05
@MathiasEttingerspeedis used in themoveandis_stuckfunctions
â Anthony Pham
Aug 3 at 15:30
Yes, but speed is always 1. So, unless you plan to have it varying in the future, I see no point in keeping it around.
â Mathias Ettinger
Aug 3 at 15:48
Yes I am going to vary it, I currently have it at one since it is more simple to test
â Anthony Pham
Aug 3 at 16:47
add a comment |Â
up vote
7
down vote
favorite
up vote
7
down vote
favorite
This game has 4 people moving randomly, one unit in one of the four directions (up, down, left, right), with a fifth person as a designated tagger. If this fifth person tags someone, the person is out. Game ends when there is one survivor left.
import random
class Player:
def __init__(self, name, status, speed, health, position):
self.name = name
self.status = status
self.speed = speed
self.health = health
self.position = position
def move(self, arena_limits, player_pos):
change = "left": -1, "right": 1, "up": 1, "down": -1
direction = random.choice(list(change.keys()))
if not self.is_stuck(arena_limits, player_pos):
if direction in ("left", "right"):
in_limits = 0 <= self.position[0] + (change[direction] * self.speed) <= arena_limits[0]
if in_limits and [self.position[0] + (change[direction] * self.speed), self.position[1]] not in player_pos:
self.position[0] += change[direction] * self.speed
else:
self.move(arena_limits, player_pos)
if direction in ("up", "down"):
in_limits = 0 <= self.position[1] + (change[direction] * self.speed) <= arena_limits[0]
if in_limits and [self.position[0], self.position[1] + (change[direction] * self.speed)] not in player_pos:
self.position[1] += change[direction] * self.speed
else:
self.move(arena_limits, player_pos)
else:
pass
def is_stuck(self, arena_limits, player_pos):
can_go_left = ([self.position[0] + (-1 * self.speed), self.position[1]] not in player_pos) and
0 <= self.position[0] + (-1 * self.speed) <= arena_limits[0]
can_go_right = ([self.position[0] + (1 * self.speed), self.position[1]] not in player_pos) and
0 <= self.position[0] + (1 * self.speed) <= arena_limits[0]
can_go_up = ([self.position[0], self.position[1] + (1 * self.speed)] not in player_pos) and
0 <= self.position[1] + (1 * self.speed) <= arena_limits[1]
can_go_down = ([self.position[0], self.position[1] + (-1 * self.speed)] not in player_pos) and
0 <= self.position[1] + (-1 * self.speed) <= arena_limits[1]
if not (can_go_up or can_go_down or can_go_left or can_go_right):
return True
else:
return False
def tag(self, player_list):
if self.status == "IT":
done = False
turns = 0
while not done:
turns += 1
player_pos = [user.position for user in player_list] + [self.position]
for player in player_list:
player.move([8, 8], player_pos)
above_or_under = player.position[0] in (self.position[0] + 1, self.position[0] - 1)
left_or_right = player.position[1] in (self.position[1] + 1, self.position[1] - 1)
if above_or_under or left_or_right:
del player_list[player_list.index(player)]
print(player.name + " has been eliminated after " + str(turns) + " turn(s)!")
if len(player_list) == 1:
print(player_list[0].name + " is the winner!")
print("-" * 20)
done = True
self.move([8, 8], player_pos)
tagger = Player("TAGGER", "IT", 1, 0, [8, 8])
for b in range(10000):
victims =
for i in range(4):
victims.append(Player("Player" + str(i), "", 1, 0, [random.randint(0, 6), random.randint(0, 6)]))
tagger.tag(victims)
Some quick comments about the code:
- Comments will be added to my code soon
self.healthis currently not used but is planned on being used- The field is
8x8and each person can move once in one of the four directions. If they are stuckis_stuck(), they will simply not move - A player is out if they are directly above, next to, or below the tagger. Diagonally does not count
Main question here, can I shorten the code and can I make it more Pythonic?
python object-oriented python-3.x simulation
This game has 4 people moving randomly, one unit in one of the four directions (up, down, left, right), with a fifth person as a designated tagger. If this fifth person tags someone, the person is out. Game ends when there is one survivor left.
import random
class Player:
def __init__(self, name, status, speed, health, position):
self.name = name
self.status = status
self.speed = speed
self.health = health
self.position = position
def move(self, arena_limits, player_pos):
change = "left": -1, "right": 1, "up": 1, "down": -1
direction = random.choice(list(change.keys()))
if not self.is_stuck(arena_limits, player_pos):
if direction in ("left", "right"):
in_limits = 0 <= self.position[0] + (change[direction] * self.speed) <= arena_limits[0]
if in_limits and [self.position[0] + (change[direction] * self.speed), self.position[1]] not in player_pos:
self.position[0] += change[direction] * self.speed
else:
self.move(arena_limits, player_pos)
if direction in ("up", "down"):
in_limits = 0 <= self.position[1] + (change[direction] * self.speed) <= arena_limits[0]
if in_limits and [self.position[0], self.position[1] + (change[direction] * self.speed)] not in player_pos:
self.position[1] += change[direction] * self.speed
else:
self.move(arena_limits, player_pos)
else:
pass
def is_stuck(self, arena_limits, player_pos):
can_go_left = ([self.position[0] + (-1 * self.speed), self.position[1]] not in player_pos) and
0 <= self.position[0] + (-1 * self.speed) <= arena_limits[0]
can_go_right = ([self.position[0] + (1 * self.speed), self.position[1]] not in player_pos) and
0 <= self.position[0] + (1 * self.speed) <= arena_limits[0]
can_go_up = ([self.position[0], self.position[1] + (1 * self.speed)] not in player_pos) and
0 <= self.position[1] + (1 * self.speed) <= arena_limits[1]
can_go_down = ([self.position[0], self.position[1] + (-1 * self.speed)] not in player_pos) and
0 <= self.position[1] + (-1 * self.speed) <= arena_limits[1]
if not (can_go_up or can_go_down or can_go_left or can_go_right):
return True
else:
return False
def tag(self, player_list):
if self.status == "IT":
done = False
turns = 0
while not done:
turns += 1
player_pos = [user.position for user in player_list] + [self.position]
for player in player_list:
player.move([8, 8], player_pos)
above_or_under = player.position[0] in (self.position[0] + 1, self.position[0] - 1)
left_or_right = player.position[1] in (self.position[1] + 1, self.position[1] - 1)
if above_or_under or left_or_right:
del player_list[player_list.index(player)]
print(player.name + " has been eliminated after " + str(turns) + " turn(s)!")
if len(player_list) == 1:
print(player_list[0].name + " is the winner!")
print("-" * 20)
done = True
self.move([8, 8], player_pos)
tagger = Player("TAGGER", "IT", 1, 0, [8, 8])
for b in range(10000):
victims =
for i in range(4):
victims.append(Player("Player" + str(i), "", 1, 0, [random.randint(0, 6), random.randint(0, 6)]))
tagger.tag(victims)
Some quick comments about the code:
- Comments will be added to my code soon
self.healthis currently not used but is planned on being used- The field is
8x8and each person can move once in one of the four directions. If they are stuckis_stuck(), they will simply not move - A player is out if they are directly above, next to, or below the tagger. Diagonally does not count
Main question here, can I shorten the code and can I make it more Pythonic?
python object-oriented python-3.x simulation
edited Aug 3 at 3:22
200_success
123k14143398
123k14143398
asked Aug 2 at 21:29
Anthony Pham
4501620
4501620
You said thathealthis reserved for future improvements, but there is no real point in havingspeedeither. Do you plan to use different values in the future too?
â Mathias Ettinger
Aug 3 at 7:05
@MathiasEttingerspeedis used in themoveandis_stuckfunctions
â Anthony Pham
Aug 3 at 15:30
Yes, but speed is always 1. So, unless you plan to have it varying in the future, I see no point in keeping it around.
â Mathias Ettinger
Aug 3 at 15:48
Yes I am going to vary it, I currently have it at one since it is more simple to test
â Anthony Pham
Aug 3 at 16:47
add a comment |Â
You said thathealthis reserved for future improvements, but there is no real point in havingspeedeither. Do you plan to use different values in the future too?
â Mathias Ettinger
Aug 3 at 7:05
@MathiasEttingerspeedis used in themoveandis_stuckfunctions
â Anthony Pham
Aug 3 at 15:30
Yes, but speed is always 1. So, unless you plan to have it varying in the future, I see no point in keeping it around.
â Mathias Ettinger
Aug 3 at 15:48
Yes I am going to vary it, I currently have it at one since it is more simple to test
â Anthony Pham
Aug 3 at 16:47
You said that
health is reserved for future improvements, but there is no real point in having speed either. Do you plan to use different values in the future too?â Mathias Ettinger
Aug 3 at 7:05
You said that
health is reserved for future improvements, but there is no real point in having speed either. Do you plan to use different values in the future too?â Mathias Ettinger
Aug 3 at 7:05
@MathiasEttinger
speed is used in the move and is_stuck functionsâ Anthony Pham
Aug 3 at 15:30
@MathiasEttinger
speed is used in the move and is_stuck functionsâ Anthony Pham
Aug 3 at 15:30
Yes, but speed is always 1. So, unless you plan to have it varying in the future, I see no point in keeping it around.
â Mathias Ettinger
Aug 3 at 15:48
Yes, but speed is always 1. So, unless you plan to have it varying in the future, I see no point in keeping it around.
â Mathias Ettinger
Aug 3 at 15:48
Yes I am going to vary it, I currently have it at one since it is more simple to test
â Anthony Pham
Aug 3 at 16:47
Yes I am going to vary it, I currently have it at one since it is more simple to test
â Anthony Pham
Aug 3 at 16:47
add a comment |Â
2 Answers
2
active
oldest
votes
up vote
8
down vote
accepted
Buggy game mechanics
Your program reports "Playerp has been eliminated after t turn(s)!", but without any visual confirmation, so you're blindly trusting that the logic is correct. But is it?
If I do add a visualization, then I see bizarre things like this happening (where T represents the tagger, and the numbers represent the pursued players):
â®
Turn 5:
---------
| |
| 3 |
| |
| 2 |
| |
| 0 |
| |
| |
| T |
---------
Turn 6:
---------
| |
| 3 |
| |
|2 |
| |
| |
| 0 |
| |
| T |
---------
Player0 has been eliminated after 7 turn(s)!
Player3 has been eliminated after 7 turn(s)!
Player2 is the winner!
Let's examine the tagging logic:
above_or_under = player.position[0] in (self.position[0] + 1, self.position[0] - 1)
left_or_right = player.position[1] in (self.position[1] + 1, self.position[1] - 1)
if above_or_under or left_or_right:
del player_list[player_list.index(player)]
â¦
Basically, this isn't a regular game of tag ("A player is out if they are directly above, next to, or below the tagger. Diagonally does not count"), but a game of laser tag with eight laser beams. Stepping on any of these rays would put a player in the kill zone:
â âÂÂ
â âÂÂ
â âÂÂ
âÂÂâÂÂâÂÂâÂÂâÂÂâÂÂâÂÂâ¼âÂÂâ¼âÂÂâÂÂâÂÂâÂÂâÂÂâÂÂâÂÂ
âÂÂTâÂÂ
âÂÂâÂÂâÂÂâÂÂâÂÂâÂÂâÂÂâ¼âÂÂâ¼âÂÂâÂÂâÂÂâÂÂâÂÂâÂÂâÂÂ
â âÂÂ
â âÂÂ
â âÂÂ
The moral of the story: verify what is happening visually. Furthermore, examine the progress at each time step. (Here's a similarly buggy simulation that could have benefitted from the same advice.)
As a consequence, I recommend that you split up your tag() method, which runs an entire game, to define a one_turn() method that just advances the game by one turn, to make it easier to examine the progress.
Design
The logic for dealing with illegal moves is cumbersome. It's repetitive, with the in_limits tests within move() being similar to the tests within is_stuck(). In move(), you make a choice of which direction to go, then awkwardly recurse to retry if that choice turns out to be illegal. It would be better to weed out the illegal moves first, then make a choice that is guaranteed to succeed.
Furthermore, move() has two branches of code to handle displacements along the two axes; it would be nice to handle displacements in any direction using the same code.
To help solve these issues, I would define a Coords class to help you figure out what the neighboring positions are. I would make it a namedtuple, because position.x and position.y would look less cluttered than position[0] and position[1].
The status member of the Player class is superfluous, as is the if self.status == "IT" check in the tag() method.
The fact that the move(), is_stuck(), and tag() methods need so many and such similar parameters passed to them is a hint that your OOP modeling is underdeveloped. In particular, the tag() method does not belong in the Player class. There really ought to be a TagGame class that defines the arena size, keeps track of its players, and encodes the rules of the game.
Style
When you say that the arena limits are [8, 8], you mean that it's a 9ÃÂ9 board, which is unconventional. Your legal coordinates range from [0, 0] to [8, 8], inclusive. I would expect that when you specify [8, 8] as the arena limits, you would get an 8ÃÂ8 board (either ranging from [0, 0] to [7, 7] inclusive, or from [1, 1] to [8, 8] inclusive).
When you get around to commenting your code, start by writing docstrings first.
Instead of composing strings by concatenation with +, use one of the several string-formatting options. Personally, I prefer str.format(), which is quite powerful and is supported by many versions of Python.
List comprehensions and generator expressions are your friend for writing compact Pythonic code!
Flag variables, such as done, are almost always a sign that the code could be improved by eliminating them.
else: pass is obviously superfluous.
del player_list[player_list.index(player)] is simply player_list.remove(player).
Suggested solution
To activate the visualization, uncomment the print() statements in TagGame.one_turn().
from collections import namedtuple
from random import choice, randint
class Coords(namedtuple('Coords', 'x y')):
def is_in_bounds(self, bounds):
return (0 < self.x <= bounds.x) and (0 < self.y <= bounds.y)
def neighbors(self, bounds, steps=1):
"""
Return a list of Coords that are the specified number of steps away in
each cardinal direction and that are in bounds.
"""
candidate_coords = [
Coords(self.x, self.y - steps),
Coords(self.x - steps, self.y), Coords(self.x + steps, self.y),
Coords(self.x, self.y + steps),
]
return [c for c in candidate_coords if c.is_in_bounds(bounds)]
class Player:
def __init__(self, name, position, speed=1):
self.name = name
self.position = position
self.speed = speed
def __str__(self):
return self.name
def move(self, game):
"""
Make a move in a random cardinal direction (unless it is impossible to
do so without running out of bounds or stepping on another player).
"""
player_positions = [p.position for p in game.players + [game.it]]
destinations = [
c for c in self.position.neighbors(game.bounds, self.speed)
if c not in player_positions
]
if destinations:
self.position = choice(destinations)
class TagGame:
def __init__(self, num_players):
"""
Create a game with the specified number of players (plus the
"it" player).
"""
self.bounds = Coords(8, 8)
self.it = Player("IT", Coords(8, 8))
self.players = [
Player("Player0".format(i), Coords(randint(1, 6), randint(1, 6)))
for i in range(num_players)
]
self.turns = 0
def one_turn(self):
"""
Run one turn of the game, yielding the players who are eliminated.
"""
kill_zone = self.it.position.neighbors(self.bounds)
for player in self.players:
player.move(self)
if player.position in kill_zone:
self.players.remove(player)
yield player
if len(self.players) == 1:
# print(self)
return
# print(self)
self.it.move(self)
def run(self):
"""
Run the game until one player remains, and return that winning player.
"""
while True:
self.turns += 1
for loser in self.one_turn():
print("0 has been eliminated after 1 turns!".format(
loser, self.turns
))
if len(self.players) == 1:
print("0 is the winner!".format(self.players[0]))
return self.players[0]
def __str__(self):
arena = (
' ' + '-' * self.bounds.x + ' n' +
'n'.join(
'|' + ''.join(
next((
p.name[-1] for p in [self.it] + self.players
if p.position == Coords(x, y)
), ' ')
for x in range(1, self.bounds.x + 1)
) + "|"
for y in range(1, self.bounds.y + 1)
) +
'n ' + '-' * self.bounds.x + ' '
)
return "Turn 0:n1".format(self.turns, arena)
# Your buggy laser tag games tend to finish in a handful of turns.
# A proper game of tag could sometimes take thousands of turns, so I
# wouldn't run 10000 games.
for simulation in range(2):
TagGame(num_players=4).run()
print("-" * 20)
Very nice organization. I was getting there, but I got held up with a mutability-related bug. I realize now I should have refactored to better abstractions (Coords,TagGame) much earlier than I did in my process: lessons for the future.
â Graham
Aug 3 at 7:33
Amazing answer here. I will be sure to look at the new changes and how they are better. Thanks for the extremely in-depth review!
â Anthony Pham
Aug 3 at 15:30
But for some reason, the game fails if I change the coordinate size like to5,5
â Anthony Pham
Aug 3 at 18:53
If you changebounds, then you would also have to change the initial placements of the players accordingly.
â 200_success
Aug 3 at 19:07
Another question, I dont understand how my tagging logic creates a laser tag scenario? If I put the Tagger's +1 and -1 for its X and Y, how does it extend all the way?
â Anthony Pham
2 days ago
 |Â
show 1 more comment
up vote
5
down vote
If you are interested in seeing my (somewhat OOP underdeveloped) though process, you can look below. However, 200_success has presented a fleshed out solution which appears to have optimally tuned parts, so I would recommend their solution as a final product. I have no final code at the end of mine.
Here's some suggestions. This answer is an iterative process of explanation.
Initialization
Use if __name__ == '__main__':
In addition to having all of the procedural initialization code at the bottom, prefix it with an if __name__ == '__main__'. This allows other people to import parts of your module (the file) without running the code at the bottom.
(See also: What does if __name__ == âÂÂ__main__âÂÂ: do?)
Python's string formatting options are awesome.
Instead of "Player" + str(i)", do f"Player i". Prefixing a string with f makes the string a formatted string literal, which makes it easier to concatenate string and other variables by just putting the variable in curly brackets. This is new in Python 3.6.
Utilize list comprehensions
Instead of
victims =
for i in range(4):
victims.append(Player(f"Player i", "", 1, 0, [random.randint(0, 6) for i in range(2)]))
just do:
victims = [Player(f"Player i", "", 1, 0, [random.randint(0, 6), random.randint(0, 6)]) for i in range(4)]
DRYer is sometimes only negligibly better
Instead of repeating random.randint(0, 6) twice in [random.randint(0, 6), random.randint(0, 6)], one could just do a list comprehension [random.randint(0, 6) for i in range(2)] In this example, it makes almost no difference, but as you scale up or generalize, it can be useful to consider.
But this is only a taste of DRYer code; more of this idea will be developed later.
Player.move() and Player.is_stuck() methods
Define often-used "constants" as class variables, not local method variables...
Instead of this:
class Player:
# ...
def move(self, arena_limits, player_pos):
change = "left": -1, "right": 1, "up": 1, "down": -1
do this:
class Player:
# ...
change = "left": -1, "right": 1, "up": 1, "down": -1
def move(self, arena_limits, player_pos):
# ...
There's no reason to redefine this each time the method is called.
...and utilize enums
Enums can be incredibly useful in any programming language, because they define a restricted set of values. Instead of defining a dictionary of strings, you can define some enums.
For more information about the why of enums, I would recommend reading this Stack Overflow answer.
Use DRY code
When I see something like the below, I immediately know the code can be made more compact, because the entire control flow is repeated.
if direction in ("left", "right"):
in_limits = 0 <= self.position[0] + (change[direction] * self.speed) <= arena_limits[0]
if in_limits and [self.position[0] + (change[direction] * self.speed), self.position[1]] not in player_pos:
self.position[0] += change[direction] * self.speed
else:
self.move(arena_limits, player_pos)
if direction in ("up", "down"):
in_limits = 0 <= self.position[1] + (change[direction] * self.speed) <= arena_limits[0]
if in_limits and [self.position[0], self.position[1] + (change[direction] * self.speed)] not in player_pos:
self.position[1] += change[direction] * self.speed
else:
self.move(arena_limits, player_pos)
Also you use (change[direction] * self.speed) all over the place in this section, when you could just assign it to a variable (I chose to call it displacement.)
Now, it can be tricky, and require other constructs, and sometimes it's more efficient to repeat code. But generally, 99% of the time, it's a big no-no.
Now, the example I gave is nothing compared to the (apologies) horrendous repetition in this block:
can_go_left = ([self.position[0] + (-1 * self.speed), self.position[1]] not in player_pos) and
0 <= self.position[0] + (-1 * self.speed) <= arena_limits[0]
can_go_right = ([self.position[0] + (1 * self.speed), self.position[1]] not in player_pos) and
0 <= self.position[0] + (1 * self.speed) <= arena_limits[0]
can_go_up = ([self.position[0], self.position[1] + (1 * self.speed)] not in player_pos) and
0 <= self.position[1] + (1 * self.speed) <= arena_limits[1]
can_go_down = ([self.position[0], self.position[1] + (-1 * self.speed)] not in player_pos) and
0 <= self.position[1] + (-1 * self.speed) <= arena_limits[1]
if not (can_go_up or can_go_down or can_go_left or can_go_right):
... but more on that later.
Avoid unnecessary recursion...
...like this (consider it in context):
def move(self, arena_limits, player_pos):
# ...
self.move(arena_limits, player_pos)
This is a bit debatable, but generally it's better practice to program procedurally when it's equal effort. Getting a bit technical, recursion adds variables to the stack when it goes one level deeper, taking up more memory than necessary. This is a bit inefficient, and on a large scale can result in stack overflow (or "RecursionError", because Python catches stack overflow.)
move() refactored I
from enum import Enum
# ...
class Movement
# (direction, magnitude)
LEFT = (0, -1)
RIGHT = (0, 1)
UP = (1, 1)
DOWN = (1, -1)
def move(self, arena_limits, player_pos):
if not self.is_stuck(arena_limits, player_pos):
while True:
movement = random.choice(list(self.Movement))
direction, magnitude = move[0], move[1]
displacement = magnitude * self.speed
in_limits = 0 <= self.position[direction] + displacement <= arena_limits[0]
destination = self.position.copy()
destination[direction] += displacement
if in_limits and destination not in player_pos:
self.position[direction] += displacement
break
But wait, there's more!
Player.is_stuck() does most of the same logic that is repeated continually in Player.move(). By being clever in refactoring and combining code, one can almost like magic eliminate most of the logic, just letting DRY and efficient code take care of the work. Basically, we just developed a general method to find accessible location in move(), so now we can apply it to the starting logic, and get that logic out the way before ever choosing an individual direction:
move() refactored II
def move(self, arena_limits, player_pos):
accessible = self.accessible_positions(arena_limits, player_pos)
if accessible:
movement = random.choice(accessible)
self.position[movement[0]] += movement[1] * self.speed
def accessible_positions(self, arena_limits, player_pos):
accessible =
for m in self.Movement:
move = m.value
direction, magnitude = move
displacement = magnitude * self.speed
destination = self.position.copy()
destination[direction] += displacement
if 0 <= self.position[direction] + displacement <= arena_limits[0] and destination not in player_pos:
accessible.append(move)
return accessible
Use more standard loop flow
Declaring a boolean to serve as a loop check variable is generally unnecessary and can lead to problems (e.g. the loop continuing to do things before it checks arrives at the condition check). while True: ... if whatever: break is generally a more recognizable, more understandable, and more concise idiom.
Conclusion
This answer will stay unfinished, but hopefully the thought process can be useful.
add a comment |Â
2 Answers
2
active
oldest
votes
2 Answers
2
active
oldest
votes
active
oldest
votes
active
oldest
votes
up vote
8
down vote
accepted
Buggy game mechanics
Your program reports "Playerp has been eliminated after t turn(s)!", but without any visual confirmation, so you're blindly trusting that the logic is correct. But is it?
If I do add a visualization, then I see bizarre things like this happening (where T represents the tagger, and the numbers represent the pursued players):
â®
Turn 5:
---------
| |
| 3 |
| |
| 2 |
| |
| 0 |
| |
| |
| T |
---------
Turn 6:
---------
| |
| 3 |
| |
|2 |
| |
| |
| 0 |
| |
| T |
---------
Player0 has been eliminated after 7 turn(s)!
Player3 has been eliminated after 7 turn(s)!
Player2 is the winner!
Let's examine the tagging logic:
above_or_under = player.position[0] in (self.position[0] + 1, self.position[0] - 1)
left_or_right = player.position[1] in (self.position[1] + 1, self.position[1] - 1)
if above_or_under or left_or_right:
del player_list[player_list.index(player)]
â¦
Basically, this isn't a regular game of tag ("A player is out if they are directly above, next to, or below the tagger. Diagonally does not count"), but a game of laser tag with eight laser beams. Stepping on any of these rays would put a player in the kill zone:
â âÂÂ
â âÂÂ
â âÂÂ
âÂÂâÂÂâÂÂâÂÂâÂÂâÂÂâÂÂâ¼âÂÂâ¼âÂÂâÂÂâÂÂâÂÂâÂÂâÂÂâÂÂ
âÂÂTâÂÂ
âÂÂâÂÂâÂÂâÂÂâÂÂâÂÂâÂÂâ¼âÂÂâ¼âÂÂâÂÂâÂÂâÂÂâÂÂâÂÂâÂÂ
â âÂÂ
â âÂÂ
â âÂÂ
The moral of the story: verify what is happening visually. Furthermore, examine the progress at each time step. (Here's a similarly buggy simulation that could have benefitted from the same advice.)
As a consequence, I recommend that you split up your tag() method, which runs an entire game, to define a one_turn() method that just advances the game by one turn, to make it easier to examine the progress.
Design
The logic for dealing with illegal moves is cumbersome. It's repetitive, with the in_limits tests within move() being similar to the tests within is_stuck(). In move(), you make a choice of which direction to go, then awkwardly recurse to retry if that choice turns out to be illegal. It would be better to weed out the illegal moves first, then make a choice that is guaranteed to succeed.
Furthermore, move() has two branches of code to handle displacements along the two axes; it would be nice to handle displacements in any direction using the same code.
To help solve these issues, I would define a Coords class to help you figure out what the neighboring positions are. I would make it a namedtuple, because position.x and position.y would look less cluttered than position[0] and position[1].
The status member of the Player class is superfluous, as is the if self.status == "IT" check in the tag() method.
The fact that the move(), is_stuck(), and tag() methods need so many and such similar parameters passed to them is a hint that your OOP modeling is underdeveloped. In particular, the tag() method does not belong in the Player class. There really ought to be a TagGame class that defines the arena size, keeps track of its players, and encodes the rules of the game.
Style
When you say that the arena limits are [8, 8], you mean that it's a 9ÃÂ9 board, which is unconventional. Your legal coordinates range from [0, 0] to [8, 8], inclusive. I would expect that when you specify [8, 8] as the arena limits, you would get an 8ÃÂ8 board (either ranging from [0, 0] to [7, 7] inclusive, or from [1, 1] to [8, 8] inclusive).
When you get around to commenting your code, start by writing docstrings first.
Instead of composing strings by concatenation with +, use one of the several string-formatting options. Personally, I prefer str.format(), which is quite powerful and is supported by many versions of Python.
List comprehensions and generator expressions are your friend for writing compact Pythonic code!
Flag variables, such as done, are almost always a sign that the code could be improved by eliminating them.
else: pass is obviously superfluous.
del player_list[player_list.index(player)] is simply player_list.remove(player).
Suggested solution
To activate the visualization, uncomment the print() statements in TagGame.one_turn().
from collections import namedtuple
from random import choice, randint
class Coords(namedtuple('Coords', 'x y')):
def is_in_bounds(self, bounds):
return (0 < self.x <= bounds.x) and (0 < self.y <= bounds.y)
def neighbors(self, bounds, steps=1):
"""
Return a list of Coords that are the specified number of steps away in
each cardinal direction and that are in bounds.
"""
candidate_coords = [
Coords(self.x, self.y - steps),
Coords(self.x - steps, self.y), Coords(self.x + steps, self.y),
Coords(self.x, self.y + steps),
]
return [c for c in candidate_coords if c.is_in_bounds(bounds)]
class Player:
def __init__(self, name, position, speed=1):
self.name = name
self.position = position
self.speed = speed
def __str__(self):
return self.name
def move(self, game):
"""
Make a move in a random cardinal direction (unless it is impossible to
do so without running out of bounds or stepping on another player).
"""
player_positions = [p.position for p in game.players + [game.it]]
destinations = [
c for c in self.position.neighbors(game.bounds, self.speed)
if c not in player_positions
]
if destinations:
self.position = choice(destinations)
class TagGame:
def __init__(self, num_players):
"""
Create a game with the specified number of players (plus the
"it" player).
"""
self.bounds = Coords(8, 8)
self.it = Player("IT", Coords(8, 8))
self.players = [
Player("Player0".format(i), Coords(randint(1, 6), randint(1, 6)))
for i in range(num_players)
]
self.turns = 0
def one_turn(self):
"""
Run one turn of the game, yielding the players who are eliminated.
"""
kill_zone = self.it.position.neighbors(self.bounds)
for player in self.players:
player.move(self)
if player.position in kill_zone:
self.players.remove(player)
yield player
if len(self.players) == 1:
# print(self)
return
# print(self)
self.it.move(self)
def run(self):
"""
Run the game until one player remains, and return that winning player.
"""
while True:
self.turns += 1
for loser in self.one_turn():
print("0 has been eliminated after 1 turns!".format(
loser, self.turns
))
if len(self.players) == 1:
print("0 is the winner!".format(self.players[0]))
return self.players[0]
def __str__(self):
arena = (
' ' + '-' * self.bounds.x + ' n' +
'n'.join(
'|' + ''.join(
next((
p.name[-1] for p in [self.it] + self.players
if p.position == Coords(x, y)
), ' ')
for x in range(1, self.bounds.x + 1)
) + "|"
for y in range(1, self.bounds.y + 1)
) +
'n ' + '-' * self.bounds.x + ' '
)
return "Turn 0:n1".format(self.turns, arena)
# Your buggy laser tag games tend to finish in a handful of turns.
# A proper game of tag could sometimes take thousands of turns, so I
# wouldn't run 10000 games.
for simulation in range(2):
TagGame(num_players=4).run()
print("-" * 20)
Very nice organization. I was getting there, but I got held up with a mutability-related bug. I realize now I should have refactored to better abstractions (Coords,TagGame) much earlier than I did in my process: lessons for the future.
â Graham
Aug 3 at 7:33
Amazing answer here. I will be sure to look at the new changes and how they are better. Thanks for the extremely in-depth review!
â Anthony Pham
Aug 3 at 15:30
But for some reason, the game fails if I change the coordinate size like to5,5
â Anthony Pham
Aug 3 at 18:53
If you changebounds, then you would also have to change the initial placements of the players accordingly.
â 200_success
Aug 3 at 19:07
Another question, I dont understand how my tagging logic creates a laser tag scenario? If I put the Tagger's +1 and -1 for its X and Y, how does it extend all the way?
â Anthony Pham
2 days ago
 |Â
show 1 more comment
up vote
8
down vote
accepted
Buggy game mechanics
Your program reports "Playerp has been eliminated after t turn(s)!", but without any visual confirmation, so you're blindly trusting that the logic is correct. But is it?
If I do add a visualization, then I see bizarre things like this happening (where T represents the tagger, and the numbers represent the pursued players):
â®
Turn 5:
---------
| |
| 3 |
| |
| 2 |
| |
| 0 |
| |
| |
| T |
---------
Turn 6:
---------
| |
| 3 |
| |
|2 |
| |
| |
| 0 |
| |
| T |
---------
Player0 has been eliminated after 7 turn(s)!
Player3 has been eliminated after 7 turn(s)!
Player2 is the winner!
Let's examine the tagging logic:
above_or_under = player.position[0] in (self.position[0] + 1, self.position[0] - 1)
left_or_right = player.position[1] in (self.position[1] + 1, self.position[1] - 1)
if above_or_under or left_or_right:
del player_list[player_list.index(player)]
â¦
Basically, this isn't a regular game of tag ("A player is out if they are directly above, next to, or below the tagger. Diagonally does not count"), but a game of laser tag with eight laser beams. Stepping on any of these rays would put a player in the kill zone:
â âÂÂ
â âÂÂ
â âÂÂ
âÂÂâÂÂâÂÂâÂÂâÂÂâÂÂâÂÂâ¼âÂÂâ¼âÂÂâÂÂâÂÂâÂÂâÂÂâÂÂâÂÂ
âÂÂTâÂÂ
âÂÂâÂÂâÂÂâÂÂâÂÂâÂÂâÂÂâ¼âÂÂâ¼âÂÂâÂÂâÂÂâÂÂâÂÂâÂÂâÂÂ
â âÂÂ
â âÂÂ
â âÂÂ
The moral of the story: verify what is happening visually. Furthermore, examine the progress at each time step. (Here's a similarly buggy simulation that could have benefitted from the same advice.)
As a consequence, I recommend that you split up your tag() method, which runs an entire game, to define a one_turn() method that just advances the game by one turn, to make it easier to examine the progress.
Design
The logic for dealing with illegal moves is cumbersome. It's repetitive, with the in_limits tests within move() being similar to the tests within is_stuck(). In move(), you make a choice of which direction to go, then awkwardly recurse to retry if that choice turns out to be illegal. It would be better to weed out the illegal moves first, then make a choice that is guaranteed to succeed.
Furthermore, move() has two branches of code to handle displacements along the two axes; it would be nice to handle displacements in any direction using the same code.
To help solve these issues, I would define a Coords class to help you figure out what the neighboring positions are. I would make it a namedtuple, because position.x and position.y would look less cluttered than position[0] and position[1].
The status member of the Player class is superfluous, as is the if self.status == "IT" check in the tag() method.
The fact that the move(), is_stuck(), and tag() methods need so many and such similar parameters passed to them is a hint that your OOP modeling is underdeveloped. In particular, the tag() method does not belong in the Player class. There really ought to be a TagGame class that defines the arena size, keeps track of its players, and encodes the rules of the game.
Style
When you say that the arena limits are [8, 8], you mean that it's a 9ÃÂ9 board, which is unconventional. Your legal coordinates range from [0, 0] to [8, 8], inclusive. I would expect that when you specify [8, 8] as the arena limits, you would get an 8ÃÂ8 board (either ranging from [0, 0] to [7, 7] inclusive, or from [1, 1] to [8, 8] inclusive).
When you get around to commenting your code, start by writing docstrings first.
Instead of composing strings by concatenation with +, use one of the several string-formatting options. Personally, I prefer str.format(), which is quite powerful and is supported by many versions of Python.
List comprehensions and generator expressions are your friend for writing compact Pythonic code!
Flag variables, such as done, are almost always a sign that the code could be improved by eliminating them.
else: pass is obviously superfluous.
del player_list[player_list.index(player)] is simply player_list.remove(player).
Suggested solution
To activate the visualization, uncomment the print() statements in TagGame.one_turn().
from collections import namedtuple
from random import choice, randint
class Coords(namedtuple('Coords', 'x y')):
def is_in_bounds(self, bounds):
return (0 < self.x <= bounds.x) and (0 < self.y <= bounds.y)
def neighbors(self, bounds, steps=1):
"""
Return a list of Coords that are the specified number of steps away in
each cardinal direction and that are in bounds.
"""
candidate_coords = [
Coords(self.x, self.y - steps),
Coords(self.x - steps, self.y), Coords(self.x + steps, self.y),
Coords(self.x, self.y + steps),
]
return [c for c in candidate_coords if c.is_in_bounds(bounds)]
class Player:
def __init__(self, name, position, speed=1):
self.name = name
self.position = position
self.speed = speed
def __str__(self):
return self.name
def move(self, game):
"""
Make a move in a random cardinal direction (unless it is impossible to
do so without running out of bounds or stepping on another player).
"""
player_positions = [p.position for p in game.players + [game.it]]
destinations = [
c for c in self.position.neighbors(game.bounds, self.speed)
if c not in player_positions
]
if destinations:
self.position = choice(destinations)
class TagGame:
def __init__(self, num_players):
"""
Create a game with the specified number of players (plus the
"it" player).
"""
self.bounds = Coords(8, 8)
self.it = Player("IT", Coords(8, 8))
self.players = [
Player("Player0".format(i), Coords(randint(1, 6), randint(1, 6)))
for i in range(num_players)
]
self.turns = 0
def one_turn(self):
"""
Run one turn of the game, yielding the players who are eliminated.
"""
kill_zone = self.it.position.neighbors(self.bounds)
for player in self.players:
player.move(self)
if player.position in kill_zone:
self.players.remove(player)
yield player
if len(self.players) == 1:
# print(self)
return
# print(self)
self.it.move(self)
def run(self):
"""
Run the game until one player remains, and return that winning player.
"""
while True:
self.turns += 1
for loser in self.one_turn():
print("0 has been eliminated after 1 turns!".format(
loser, self.turns
))
if len(self.players) == 1:
print("0 is the winner!".format(self.players[0]))
return self.players[0]
def __str__(self):
arena = (
' ' + '-' * self.bounds.x + ' n' +
'n'.join(
'|' + ''.join(
next((
p.name[-1] for p in [self.it] + self.players
if p.position == Coords(x, y)
), ' ')
for x in range(1, self.bounds.x + 1)
) + "|"
for y in range(1, self.bounds.y + 1)
) +
'n ' + '-' * self.bounds.x + ' '
)
return "Turn 0:n1".format(self.turns, arena)
# Your buggy laser tag games tend to finish in a handful of turns.
# A proper game of tag could sometimes take thousands of turns, so I
# wouldn't run 10000 games.
for simulation in range(2):
TagGame(num_players=4).run()
print("-" * 20)
Very nice organization. I was getting there, but I got held up with a mutability-related bug. I realize now I should have refactored to better abstractions (Coords,TagGame) much earlier than I did in my process: lessons for the future.
â Graham
Aug 3 at 7:33
Amazing answer here. I will be sure to look at the new changes and how they are better. Thanks for the extremely in-depth review!
â Anthony Pham
Aug 3 at 15:30
But for some reason, the game fails if I change the coordinate size like to5,5
â Anthony Pham
Aug 3 at 18:53
If you changebounds, then you would also have to change the initial placements of the players accordingly.
â 200_success
Aug 3 at 19:07
Another question, I dont understand how my tagging logic creates a laser tag scenario? If I put the Tagger's +1 and -1 for its X and Y, how does it extend all the way?
â Anthony Pham
2 days ago
 |Â
show 1 more comment
up vote
8
down vote
accepted
up vote
8
down vote
accepted
Buggy game mechanics
Your program reports "Playerp has been eliminated after t turn(s)!", but without any visual confirmation, so you're blindly trusting that the logic is correct. But is it?
If I do add a visualization, then I see bizarre things like this happening (where T represents the tagger, and the numbers represent the pursued players):
â®
Turn 5:
---------
| |
| 3 |
| |
| 2 |
| |
| 0 |
| |
| |
| T |
---------
Turn 6:
---------
| |
| 3 |
| |
|2 |
| |
| |
| 0 |
| |
| T |
---------
Player0 has been eliminated after 7 turn(s)!
Player3 has been eliminated after 7 turn(s)!
Player2 is the winner!
Let's examine the tagging logic:
above_or_under = player.position[0] in (self.position[0] + 1, self.position[0] - 1)
left_or_right = player.position[1] in (self.position[1] + 1, self.position[1] - 1)
if above_or_under or left_or_right:
del player_list[player_list.index(player)]
â¦
Basically, this isn't a regular game of tag ("A player is out if they are directly above, next to, or below the tagger. Diagonally does not count"), but a game of laser tag with eight laser beams. Stepping on any of these rays would put a player in the kill zone:
â âÂÂ
â âÂÂ
â âÂÂ
âÂÂâÂÂâÂÂâÂÂâÂÂâÂÂâÂÂâ¼âÂÂâ¼âÂÂâÂÂâÂÂâÂÂâÂÂâÂÂâÂÂ
âÂÂTâÂÂ
âÂÂâÂÂâÂÂâÂÂâÂÂâÂÂâÂÂâ¼âÂÂâ¼âÂÂâÂÂâÂÂâÂÂâÂÂâÂÂâÂÂ
â âÂÂ
â âÂÂ
â âÂÂ
The moral of the story: verify what is happening visually. Furthermore, examine the progress at each time step. (Here's a similarly buggy simulation that could have benefitted from the same advice.)
As a consequence, I recommend that you split up your tag() method, which runs an entire game, to define a one_turn() method that just advances the game by one turn, to make it easier to examine the progress.
Design
The logic for dealing with illegal moves is cumbersome. It's repetitive, with the in_limits tests within move() being similar to the tests within is_stuck(). In move(), you make a choice of which direction to go, then awkwardly recurse to retry if that choice turns out to be illegal. It would be better to weed out the illegal moves first, then make a choice that is guaranteed to succeed.
Furthermore, move() has two branches of code to handle displacements along the two axes; it would be nice to handle displacements in any direction using the same code.
To help solve these issues, I would define a Coords class to help you figure out what the neighboring positions are. I would make it a namedtuple, because position.x and position.y would look less cluttered than position[0] and position[1].
The status member of the Player class is superfluous, as is the if self.status == "IT" check in the tag() method.
The fact that the move(), is_stuck(), and tag() methods need so many and such similar parameters passed to them is a hint that your OOP modeling is underdeveloped. In particular, the tag() method does not belong in the Player class. There really ought to be a TagGame class that defines the arena size, keeps track of its players, and encodes the rules of the game.
Style
When you say that the arena limits are [8, 8], you mean that it's a 9ÃÂ9 board, which is unconventional. Your legal coordinates range from [0, 0] to [8, 8], inclusive. I would expect that when you specify [8, 8] as the arena limits, you would get an 8ÃÂ8 board (either ranging from [0, 0] to [7, 7] inclusive, or from [1, 1] to [8, 8] inclusive).
When you get around to commenting your code, start by writing docstrings first.
Instead of composing strings by concatenation with +, use one of the several string-formatting options. Personally, I prefer str.format(), which is quite powerful and is supported by many versions of Python.
List comprehensions and generator expressions are your friend for writing compact Pythonic code!
Flag variables, such as done, are almost always a sign that the code could be improved by eliminating them.
else: pass is obviously superfluous.
del player_list[player_list.index(player)] is simply player_list.remove(player).
Suggested solution
To activate the visualization, uncomment the print() statements in TagGame.one_turn().
from collections import namedtuple
from random import choice, randint
class Coords(namedtuple('Coords', 'x y')):
def is_in_bounds(self, bounds):
return (0 < self.x <= bounds.x) and (0 < self.y <= bounds.y)
def neighbors(self, bounds, steps=1):
"""
Return a list of Coords that are the specified number of steps away in
each cardinal direction and that are in bounds.
"""
candidate_coords = [
Coords(self.x, self.y - steps),
Coords(self.x - steps, self.y), Coords(self.x + steps, self.y),
Coords(self.x, self.y + steps),
]
return [c for c in candidate_coords if c.is_in_bounds(bounds)]
class Player:
def __init__(self, name, position, speed=1):
self.name = name
self.position = position
self.speed = speed
def __str__(self):
return self.name
def move(self, game):
"""
Make a move in a random cardinal direction (unless it is impossible to
do so without running out of bounds or stepping on another player).
"""
player_positions = [p.position for p in game.players + [game.it]]
destinations = [
c for c in self.position.neighbors(game.bounds, self.speed)
if c not in player_positions
]
if destinations:
self.position = choice(destinations)
class TagGame:
def __init__(self, num_players):
"""
Create a game with the specified number of players (plus the
"it" player).
"""
self.bounds = Coords(8, 8)
self.it = Player("IT", Coords(8, 8))
self.players = [
Player("Player0".format(i), Coords(randint(1, 6), randint(1, 6)))
for i in range(num_players)
]
self.turns = 0
def one_turn(self):
"""
Run one turn of the game, yielding the players who are eliminated.
"""
kill_zone = self.it.position.neighbors(self.bounds)
for player in self.players:
player.move(self)
if player.position in kill_zone:
self.players.remove(player)
yield player
if len(self.players) == 1:
# print(self)
return
# print(self)
self.it.move(self)
def run(self):
"""
Run the game until one player remains, and return that winning player.
"""
while True:
self.turns += 1
for loser in self.one_turn():
print("0 has been eliminated after 1 turns!".format(
loser, self.turns
))
if len(self.players) == 1:
print("0 is the winner!".format(self.players[0]))
return self.players[0]
def __str__(self):
arena = (
' ' + '-' * self.bounds.x + ' n' +
'n'.join(
'|' + ''.join(
next((
p.name[-1] for p in [self.it] + self.players
if p.position == Coords(x, y)
), ' ')
for x in range(1, self.bounds.x + 1)
) + "|"
for y in range(1, self.bounds.y + 1)
) +
'n ' + '-' * self.bounds.x + ' '
)
return "Turn 0:n1".format(self.turns, arena)
# Your buggy laser tag games tend to finish in a handful of turns.
# A proper game of tag could sometimes take thousands of turns, so I
# wouldn't run 10000 games.
for simulation in range(2):
TagGame(num_players=4).run()
print("-" * 20)
Buggy game mechanics
Your program reports "Playerp has been eliminated after t turn(s)!", but without any visual confirmation, so you're blindly trusting that the logic is correct. But is it?
If I do add a visualization, then I see bizarre things like this happening (where T represents the tagger, and the numbers represent the pursued players):
â®
Turn 5:
---------
| |
| 3 |
| |
| 2 |
| |
| 0 |
| |
| |
| T |
---------
Turn 6:
---------
| |
| 3 |
| |
|2 |
| |
| |
| 0 |
| |
| T |
---------
Player0 has been eliminated after 7 turn(s)!
Player3 has been eliminated after 7 turn(s)!
Player2 is the winner!
Let's examine the tagging logic:
above_or_under = player.position[0] in (self.position[0] + 1, self.position[0] - 1)
left_or_right = player.position[1] in (self.position[1] + 1, self.position[1] - 1)
if above_or_under or left_or_right:
del player_list[player_list.index(player)]
â¦
Basically, this isn't a regular game of tag ("A player is out if they are directly above, next to, or below the tagger. Diagonally does not count"), but a game of laser tag with eight laser beams. Stepping on any of these rays would put a player in the kill zone:
â âÂÂ
â âÂÂ
â âÂÂ
âÂÂâÂÂâÂÂâÂÂâÂÂâÂÂâÂÂâ¼âÂÂâ¼âÂÂâÂÂâÂÂâÂÂâÂÂâÂÂâÂÂ
âÂÂTâÂÂ
âÂÂâÂÂâÂÂâÂÂâÂÂâÂÂâÂÂâ¼âÂÂâ¼âÂÂâÂÂâÂÂâÂÂâÂÂâÂÂâÂÂ
â âÂÂ
â âÂÂ
â âÂÂ
The moral of the story: verify what is happening visually. Furthermore, examine the progress at each time step. (Here's a similarly buggy simulation that could have benefitted from the same advice.)
As a consequence, I recommend that you split up your tag() method, which runs an entire game, to define a one_turn() method that just advances the game by one turn, to make it easier to examine the progress.
Design
The logic for dealing with illegal moves is cumbersome. It's repetitive, with the in_limits tests within move() being similar to the tests within is_stuck(). In move(), you make a choice of which direction to go, then awkwardly recurse to retry if that choice turns out to be illegal. It would be better to weed out the illegal moves first, then make a choice that is guaranteed to succeed.
Furthermore, move() has two branches of code to handle displacements along the two axes; it would be nice to handle displacements in any direction using the same code.
To help solve these issues, I would define a Coords class to help you figure out what the neighboring positions are. I would make it a namedtuple, because position.x and position.y would look less cluttered than position[0] and position[1].
The status member of the Player class is superfluous, as is the if self.status == "IT" check in the tag() method.
The fact that the move(), is_stuck(), and tag() methods need so many and such similar parameters passed to them is a hint that your OOP modeling is underdeveloped. In particular, the tag() method does not belong in the Player class. There really ought to be a TagGame class that defines the arena size, keeps track of its players, and encodes the rules of the game.
Style
When you say that the arena limits are [8, 8], you mean that it's a 9ÃÂ9 board, which is unconventional. Your legal coordinates range from [0, 0] to [8, 8], inclusive. I would expect that when you specify [8, 8] as the arena limits, you would get an 8ÃÂ8 board (either ranging from [0, 0] to [7, 7] inclusive, or from [1, 1] to [8, 8] inclusive).
When you get around to commenting your code, start by writing docstrings first.
Instead of composing strings by concatenation with +, use one of the several string-formatting options. Personally, I prefer str.format(), which is quite powerful and is supported by many versions of Python.
List comprehensions and generator expressions are your friend for writing compact Pythonic code!
Flag variables, such as done, are almost always a sign that the code could be improved by eliminating them.
else: pass is obviously superfluous.
del player_list[player_list.index(player)] is simply player_list.remove(player).
Suggested solution
To activate the visualization, uncomment the print() statements in TagGame.one_turn().
from collections import namedtuple
from random import choice, randint
class Coords(namedtuple('Coords', 'x y')):
def is_in_bounds(self, bounds):
return (0 < self.x <= bounds.x) and (0 < self.y <= bounds.y)
def neighbors(self, bounds, steps=1):
"""
Return a list of Coords that are the specified number of steps away in
each cardinal direction and that are in bounds.
"""
candidate_coords = [
Coords(self.x, self.y - steps),
Coords(self.x - steps, self.y), Coords(self.x + steps, self.y),
Coords(self.x, self.y + steps),
]
return [c for c in candidate_coords if c.is_in_bounds(bounds)]
class Player:
def __init__(self, name, position, speed=1):
self.name = name
self.position = position
self.speed = speed
def __str__(self):
return self.name
def move(self, game):
"""
Make a move in a random cardinal direction (unless it is impossible to
do so without running out of bounds or stepping on another player).
"""
player_positions = [p.position for p in game.players + [game.it]]
destinations = [
c for c in self.position.neighbors(game.bounds, self.speed)
if c not in player_positions
]
if destinations:
self.position = choice(destinations)
class TagGame:
def __init__(self, num_players):
"""
Create a game with the specified number of players (plus the
"it" player).
"""
self.bounds = Coords(8, 8)
self.it = Player("IT", Coords(8, 8))
self.players = [
Player("Player0".format(i), Coords(randint(1, 6), randint(1, 6)))
for i in range(num_players)
]
self.turns = 0
def one_turn(self):
"""
Run one turn of the game, yielding the players who are eliminated.
"""
kill_zone = self.it.position.neighbors(self.bounds)
for player in self.players:
player.move(self)
if player.position in kill_zone:
self.players.remove(player)
yield player
if len(self.players) == 1:
# print(self)
return
# print(self)
self.it.move(self)
def run(self):
"""
Run the game until one player remains, and return that winning player.
"""
while True:
self.turns += 1
for loser in self.one_turn():
print("0 has been eliminated after 1 turns!".format(
loser, self.turns
))
if len(self.players) == 1:
print("0 is the winner!".format(self.players[0]))
return self.players[0]
def __str__(self):
arena = (
' ' + '-' * self.bounds.x + ' n' +
'n'.join(
'|' + ''.join(
next((
p.name[-1] for p in [self.it] + self.players
if p.position == Coords(x, y)
), ' ')
for x in range(1, self.bounds.x + 1)
) + "|"
for y in range(1, self.bounds.y + 1)
) +
'n ' + '-' * self.bounds.x + ' '
)
return "Turn 0:n1".format(self.turns, arena)
# Your buggy laser tag games tend to finish in a handful of turns.
# A proper game of tag could sometimes take thousands of turns, so I
# wouldn't run 10000 games.
for simulation in range(2):
TagGame(num_players=4).run()
print("-" * 20)
edited Aug 3 at 10:00
mkrieger1
1,3591723
1,3591723
answered Aug 3 at 7:14
200_success
123k14143398
123k14143398
Very nice organization. I was getting there, but I got held up with a mutability-related bug. I realize now I should have refactored to better abstractions (Coords,TagGame) much earlier than I did in my process: lessons for the future.
â Graham
Aug 3 at 7:33
Amazing answer here. I will be sure to look at the new changes and how they are better. Thanks for the extremely in-depth review!
â Anthony Pham
Aug 3 at 15:30
But for some reason, the game fails if I change the coordinate size like to5,5
â Anthony Pham
Aug 3 at 18:53
If you changebounds, then you would also have to change the initial placements of the players accordingly.
â 200_success
Aug 3 at 19:07
Another question, I dont understand how my tagging logic creates a laser tag scenario? If I put the Tagger's +1 and -1 for its X and Y, how does it extend all the way?
â Anthony Pham
2 days ago
 |Â
show 1 more comment
Very nice organization. I was getting there, but I got held up with a mutability-related bug. I realize now I should have refactored to better abstractions (Coords,TagGame) much earlier than I did in my process: lessons for the future.
â Graham
Aug 3 at 7:33
Amazing answer here. I will be sure to look at the new changes and how they are better. Thanks for the extremely in-depth review!
â Anthony Pham
Aug 3 at 15:30
But for some reason, the game fails if I change the coordinate size like to5,5
â Anthony Pham
Aug 3 at 18:53
If you changebounds, then you would also have to change the initial placements of the players accordingly.
â 200_success
Aug 3 at 19:07
Another question, I dont understand how my tagging logic creates a laser tag scenario? If I put the Tagger's +1 and -1 for its X and Y, how does it extend all the way?
â Anthony Pham
2 days ago
Very nice organization. I was getting there, but I got held up with a mutability-related bug. I realize now I should have refactored to better abstractions (
Coords, TagGame) much earlier than I did in my process: lessons for the future.â Graham
Aug 3 at 7:33
Very nice organization. I was getting there, but I got held up with a mutability-related bug. I realize now I should have refactored to better abstractions (
Coords, TagGame) much earlier than I did in my process: lessons for the future.â Graham
Aug 3 at 7:33
Amazing answer here. I will be sure to look at the new changes and how they are better. Thanks for the extremely in-depth review!
â Anthony Pham
Aug 3 at 15:30
Amazing answer here. I will be sure to look at the new changes and how they are better. Thanks for the extremely in-depth review!
â Anthony Pham
Aug 3 at 15:30
But for some reason, the game fails if I change the coordinate size like to
5,5â Anthony Pham
Aug 3 at 18:53
But for some reason, the game fails if I change the coordinate size like to
5,5â Anthony Pham
Aug 3 at 18:53
If you change
bounds, then you would also have to change the initial placements of the players accordingly.â 200_success
Aug 3 at 19:07
If you change
bounds, then you would also have to change the initial placements of the players accordingly.â 200_success
Aug 3 at 19:07
Another question, I dont understand how my tagging logic creates a laser tag scenario? If I put the Tagger's +1 and -1 for its X and Y, how does it extend all the way?
â Anthony Pham
2 days ago
Another question, I dont understand how my tagging logic creates a laser tag scenario? If I put the Tagger's +1 and -1 for its X and Y, how does it extend all the way?
â Anthony Pham
2 days ago
 |Â
show 1 more comment
up vote
5
down vote
If you are interested in seeing my (somewhat OOP underdeveloped) though process, you can look below. However, 200_success has presented a fleshed out solution which appears to have optimally tuned parts, so I would recommend their solution as a final product. I have no final code at the end of mine.
Here's some suggestions. This answer is an iterative process of explanation.
Initialization
Use if __name__ == '__main__':
In addition to having all of the procedural initialization code at the bottom, prefix it with an if __name__ == '__main__'. This allows other people to import parts of your module (the file) without running the code at the bottom.
(See also: What does if __name__ == âÂÂ__main__âÂÂ: do?)
Python's string formatting options are awesome.
Instead of "Player" + str(i)", do f"Player i". Prefixing a string with f makes the string a formatted string literal, which makes it easier to concatenate string and other variables by just putting the variable in curly brackets. This is new in Python 3.6.
Utilize list comprehensions
Instead of
victims =
for i in range(4):
victims.append(Player(f"Player i", "", 1, 0, [random.randint(0, 6) for i in range(2)]))
just do:
victims = [Player(f"Player i", "", 1, 0, [random.randint(0, 6), random.randint(0, 6)]) for i in range(4)]
DRYer is sometimes only negligibly better
Instead of repeating random.randint(0, 6) twice in [random.randint(0, 6), random.randint(0, 6)], one could just do a list comprehension [random.randint(0, 6) for i in range(2)] In this example, it makes almost no difference, but as you scale up or generalize, it can be useful to consider.
But this is only a taste of DRYer code; more of this idea will be developed later.
Player.move() and Player.is_stuck() methods
Define often-used "constants" as class variables, not local method variables...
Instead of this:
class Player:
# ...
def move(self, arena_limits, player_pos):
change = "left": -1, "right": 1, "up": 1, "down": -1
do this:
class Player:
# ...
change = "left": -1, "right": 1, "up": 1, "down": -1
def move(self, arena_limits, player_pos):
# ...
There's no reason to redefine this each time the method is called.
...and utilize enums
Enums can be incredibly useful in any programming language, because they define a restricted set of values. Instead of defining a dictionary of strings, you can define some enums.
For more information about the why of enums, I would recommend reading this Stack Overflow answer.
Use DRY code
When I see something like the below, I immediately know the code can be made more compact, because the entire control flow is repeated.
if direction in ("left", "right"):
in_limits = 0 <= self.position[0] + (change[direction] * self.speed) <= arena_limits[0]
if in_limits and [self.position[0] + (change[direction] * self.speed), self.position[1]] not in player_pos:
self.position[0] += change[direction] * self.speed
else:
self.move(arena_limits, player_pos)
if direction in ("up", "down"):
in_limits = 0 <= self.position[1] + (change[direction] * self.speed) <= arena_limits[0]
if in_limits and [self.position[0], self.position[1] + (change[direction] * self.speed)] not in player_pos:
self.position[1] += change[direction] * self.speed
else:
self.move(arena_limits, player_pos)
Also you use (change[direction] * self.speed) all over the place in this section, when you could just assign it to a variable (I chose to call it displacement.)
Now, it can be tricky, and require other constructs, and sometimes it's more efficient to repeat code. But generally, 99% of the time, it's a big no-no.
Now, the example I gave is nothing compared to the (apologies) horrendous repetition in this block:
can_go_left = ([self.position[0] + (-1 * self.speed), self.position[1]] not in player_pos) and
0 <= self.position[0] + (-1 * self.speed) <= arena_limits[0]
can_go_right = ([self.position[0] + (1 * self.speed), self.position[1]] not in player_pos) and
0 <= self.position[0] + (1 * self.speed) <= arena_limits[0]
can_go_up = ([self.position[0], self.position[1] + (1 * self.speed)] not in player_pos) and
0 <= self.position[1] + (1 * self.speed) <= arena_limits[1]
can_go_down = ([self.position[0], self.position[1] + (-1 * self.speed)] not in player_pos) and
0 <= self.position[1] + (-1 * self.speed) <= arena_limits[1]
if not (can_go_up or can_go_down or can_go_left or can_go_right):
... but more on that later.
Avoid unnecessary recursion...
...like this (consider it in context):
def move(self, arena_limits, player_pos):
# ...
self.move(arena_limits, player_pos)
This is a bit debatable, but generally it's better practice to program procedurally when it's equal effort. Getting a bit technical, recursion adds variables to the stack when it goes one level deeper, taking up more memory than necessary. This is a bit inefficient, and on a large scale can result in stack overflow (or "RecursionError", because Python catches stack overflow.)
move() refactored I
from enum import Enum
# ...
class Movement
# (direction, magnitude)
LEFT = (0, -1)
RIGHT = (0, 1)
UP = (1, 1)
DOWN = (1, -1)
def move(self, arena_limits, player_pos):
if not self.is_stuck(arena_limits, player_pos):
while True:
movement = random.choice(list(self.Movement))
direction, magnitude = move[0], move[1]
displacement = magnitude * self.speed
in_limits = 0 <= self.position[direction] + displacement <= arena_limits[0]
destination = self.position.copy()
destination[direction] += displacement
if in_limits and destination not in player_pos:
self.position[direction] += displacement
break
But wait, there's more!
Player.is_stuck() does most of the same logic that is repeated continually in Player.move(). By being clever in refactoring and combining code, one can almost like magic eliminate most of the logic, just letting DRY and efficient code take care of the work. Basically, we just developed a general method to find accessible location in move(), so now we can apply it to the starting logic, and get that logic out the way before ever choosing an individual direction:
move() refactored II
def move(self, arena_limits, player_pos):
accessible = self.accessible_positions(arena_limits, player_pos)
if accessible:
movement = random.choice(accessible)
self.position[movement[0]] += movement[1] * self.speed
def accessible_positions(self, arena_limits, player_pos):
accessible =
for m in self.Movement:
move = m.value
direction, magnitude = move
displacement = magnitude * self.speed
destination = self.position.copy()
destination[direction] += displacement
if 0 <= self.position[direction] + displacement <= arena_limits[0] and destination not in player_pos:
accessible.append(move)
return accessible
Use more standard loop flow
Declaring a boolean to serve as a loop check variable is generally unnecessary and can lead to problems (e.g. the loop continuing to do things before it checks arrives at the condition check). while True: ... if whatever: break is generally a more recognizable, more understandable, and more concise idiom.
Conclusion
This answer will stay unfinished, but hopefully the thought process can be useful.
add a comment |Â
up vote
5
down vote
If you are interested in seeing my (somewhat OOP underdeveloped) though process, you can look below. However, 200_success has presented a fleshed out solution which appears to have optimally tuned parts, so I would recommend their solution as a final product. I have no final code at the end of mine.
Here's some suggestions. This answer is an iterative process of explanation.
Initialization
Use if __name__ == '__main__':
In addition to having all of the procedural initialization code at the bottom, prefix it with an if __name__ == '__main__'. This allows other people to import parts of your module (the file) without running the code at the bottom.
(See also: What does if __name__ == âÂÂ__main__âÂÂ: do?)
Python's string formatting options are awesome.
Instead of "Player" + str(i)", do f"Player i". Prefixing a string with f makes the string a formatted string literal, which makes it easier to concatenate string and other variables by just putting the variable in curly brackets. This is new in Python 3.6.
Utilize list comprehensions
Instead of
victims =
for i in range(4):
victims.append(Player(f"Player i", "", 1, 0, [random.randint(0, 6) for i in range(2)]))
just do:
victims = [Player(f"Player i", "", 1, 0, [random.randint(0, 6), random.randint(0, 6)]) for i in range(4)]
DRYer is sometimes only negligibly better
Instead of repeating random.randint(0, 6) twice in [random.randint(0, 6), random.randint(0, 6)], one could just do a list comprehension [random.randint(0, 6) for i in range(2)] In this example, it makes almost no difference, but as you scale up or generalize, it can be useful to consider.
But this is only a taste of DRYer code; more of this idea will be developed later.
Player.move() and Player.is_stuck() methods
Define often-used "constants" as class variables, not local method variables...
Instead of this:
class Player:
# ...
def move(self, arena_limits, player_pos):
change = "left": -1, "right": 1, "up": 1, "down": -1
do this:
class Player:
# ...
change = "left": -1, "right": 1, "up": 1, "down": -1
def move(self, arena_limits, player_pos):
# ...
There's no reason to redefine this each time the method is called.
...and utilize enums
Enums can be incredibly useful in any programming language, because they define a restricted set of values. Instead of defining a dictionary of strings, you can define some enums.
For more information about the why of enums, I would recommend reading this Stack Overflow answer.
Use DRY code
When I see something like the below, I immediately know the code can be made more compact, because the entire control flow is repeated.
if direction in ("left", "right"):
in_limits = 0 <= self.position[0] + (change[direction] * self.speed) <= arena_limits[0]
if in_limits and [self.position[0] + (change[direction] * self.speed), self.position[1]] not in player_pos:
self.position[0] += change[direction] * self.speed
else:
self.move(arena_limits, player_pos)
if direction in ("up", "down"):
in_limits = 0 <= self.position[1] + (change[direction] * self.speed) <= arena_limits[0]
if in_limits and [self.position[0], self.position[1] + (change[direction] * self.speed)] not in player_pos:
self.position[1] += change[direction] * self.speed
else:
self.move(arena_limits, player_pos)
Also you use (change[direction] * self.speed) all over the place in this section, when you could just assign it to a variable (I chose to call it displacement.)
Now, it can be tricky, and require other constructs, and sometimes it's more efficient to repeat code. But generally, 99% of the time, it's a big no-no.
Now, the example I gave is nothing compared to the (apologies) horrendous repetition in this block:
can_go_left = ([self.position[0] + (-1 * self.speed), self.position[1]] not in player_pos) and
0 <= self.position[0] + (-1 * self.speed) <= arena_limits[0]
can_go_right = ([self.position[0] + (1 * self.speed), self.position[1]] not in player_pos) and
0 <= self.position[0] + (1 * self.speed) <= arena_limits[0]
can_go_up = ([self.position[0], self.position[1] + (1 * self.speed)] not in player_pos) and
0 <= self.position[1] + (1 * self.speed) <= arena_limits[1]
can_go_down = ([self.position[0], self.position[1] + (-1 * self.speed)] not in player_pos) and
0 <= self.position[1] + (-1 * self.speed) <= arena_limits[1]
if not (can_go_up or can_go_down or can_go_left or can_go_right):
... but more on that later.
Avoid unnecessary recursion...
...like this (consider it in context):
def move(self, arena_limits, player_pos):
# ...
self.move(arena_limits, player_pos)
This is a bit debatable, but generally it's better practice to program procedurally when it's equal effort. Getting a bit technical, recursion adds variables to the stack when it goes one level deeper, taking up more memory than necessary. This is a bit inefficient, and on a large scale can result in stack overflow (or "RecursionError", because Python catches stack overflow.)
move() refactored I
from enum import Enum
# ...
class Movement
# (direction, magnitude)
LEFT = (0, -1)
RIGHT = (0, 1)
UP = (1, 1)
DOWN = (1, -1)
def move(self, arena_limits, player_pos):
if not self.is_stuck(arena_limits, player_pos):
while True:
movement = random.choice(list(self.Movement))
direction, magnitude = move[0], move[1]
displacement = magnitude * self.speed
in_limits = 0 <= self.position[direction] + displacement <= arena_limits[0]
destination = self.position.copy()
destination[direction] += displacement
if in_limits and destination not in player_pos:
self.position[direction] += displacement
break
But wait, there's more!
Player.is_stuck() does most of the same logic that is repeated continually in Player.move(). By being clever in refactoring and combining code, one can almost like magic eliminate most of the logic, just letting DRY and efficient code take care of the work. Basically, we just developed a general method to find accessible location in move(), so now we can apply it to the starting logic, and get that logic out the way before ever choosing an individual direction:
move() refactored II
def move(self, arena_limits, player_pos):
accessible = self.accessible_positions(arena_limits, player_pos)
if accessible:
movement = random.choice(accessible)
self.position[movement[0]] += movement[1] * self.speed
def accessible_positions(self, arena_limits, player_pos):
accessible =
for m in self.Movement:
move = m.value
direction, magnitude = move
displacement = magnitude * self.speed
destination = self.position.copy()
destination[direction] += displacement
if 0 <= self.position[direction] + displacement <= arena_limits[0] and destination not in player_pos:
accessible.append(move)
return accessible
Use more standard loop flow
Declaring a boolean to serve as a loop check variable is generally unnecessary and can lead to problems (e.g. the loop continuing to do things before it checks arrives at the condition check). while True: ... if whatever: break is generally a more recognizable, more understandable, and more concise idiom.
Conclusion
This answer will stay unfinished, but hopefully the thought process can be useful.
add a comment |Â
up vote
5
down vote
up vote
5
down vote
If you are interested in seeing my (somewhat OOP underdeveloped) though process, you can look below. However, 200_success has presented a fleshed out solution which appears to have optimally tuned parts, so I would recommend their solution as a final product. I have no final code at the end of mine.
Here's some suggestions. This answer is an iterative process of explanation.
Initialization
Use if __name__ == '__main__':
In addition to having all of the procedural initialization code at the bottom, prefix it with an if __name__ == '__main__'. This allows other people to import parts of your module (the file) without running the code at the bottom.
(See also: What does if __name__ == âÂÂ__main__âÂÂ: do?)
Python's string formatting options are awesome.
Instead of "Player" + str(i)", do f"Player i". Prefixing a string with f makes the string a formatted string literal, which makes it easier to concatenate string and other variables by just putting the variable in curly brackets. This is new in Python 3.6.
Utilize list comprehensions
Instead of
victims =
for i in range(4):
victims.append(Player(f"Player i", "", 1, 0, [random.randint(0, 6) for i in range(2)]))
just do:
victims = [Player(f"Player i", "", 1, 0, [random.randint(0, 6), random.randint(0, 6)]) for i in range(4)]
DRYer is sometimes only negligibly better
Instead of repeating random.randint(0, 6) twice in [random.randint(0, 6), random.randint(0, 6)], one could just do a list comprehension [random.randint(0, 6) for i in range(2)] In this example, it makes almost no difference, but as you scale up or generalize, it can be useful to consider.
But this is only a taste of DRYer code; more of this idea will be developed later.
Player.move() and Player.is_stuck() methods
Define often-used "constants" as class variables, not local method variables...
Instead of this:
class Player:
# ...
def move(self, arena_limits, player_pos):
change = "left": -1, "right": 1, "up": 1, "down": -1
do this:
class Player:
# ...
change = "left": -1, "right": 1, "up": 1, "down": -1
def move(self, arena_limits, player_pos):
# ...
There's no reason to redefine this each time the method is called.
...and utilize enums
Enums can be incredibly useful in any programming language, because they define a restricted set of values. Instead of defining a dictionary of strings, you can define some enums.
For more information about the why of enums, I would recommend reading this Stack Overflow answer.
Use DRY code
When I see something like the below, I immediately know the code can be made more compact, because the entire control flow is repeated.
if direction in ("left", "right"):
in_limits = 0 <= self.position[0] + (change[direction] * self.speed) <= arena_limits[0]
if in_limits and [self.position[0] + (change[direction] * self.speed), self.position[1]] not in player_pos:
self.position[0] += change[direction] * self.speed
else:
self.move(arena_limits, player_pos)
if direction in ("up", "down"):
in_limits = 0 <= self.position[1] + (change[direction] * self.speed) <= arena_limits[0]
if in_limits and [self.position[0], self.position[1] + (change[direction] * self.speed)] not in player_pos:
self.position[1] += change[direction] * self.speed
else:
self.move(arena_limits, player_pos)
Also you use (change[direction] * self.speed) all over the place in this section, when you could just assign it to a variable (I chose to call it displacement.)
Now, it can be tricky, and require other constructs, and sometimes it's more efficient to repeat code. But generally, 99% of the time, it's a big no-no.
Now, the example I gave is nothing compared to the (apologies) horrendous repetition in this block:
can_go_left = ([self.position[0] + (-1 * self.speed), self.position[1]] not in player_pos) and
0 <= self.position[0] + (-1 * self.speed) <= arena_limits[0]
can_go_right = ([self.position[0] + (1 * self.speed), self.position[1]] not in player_pos) and
0 <= self.position[0] + (1 * self.speed) <= arena_limits[0]
can_go_up = ([self.position[0], self.position[1] + (1 * self.speed)] not in player_pos) and
0 <= self.position[1] + (1 * self.speed) <= arena_limits[1]
can_go_down = ([self.position[0], self.position[1] + (-1 * self.speed)] not in player_pos) and
0 <= self.position[1] + (-1 * self.speed) <= arena_limits[1]
if not (can_go_up or can_go_down or can_go_left or can_go_right):
... but more on that later.
Avoid unnecessary recursion...
...like this (consider it in context):
def move(self, arena_limits, player_pos):
# ...
self.move(arena_limits, player_pos)
This is a bit debatable, but generally it's better practice to program procedurally when it's equal effort. Getting a bit technical, recursion adds variables to the stack when it goes one level deeper, taking up more memory than necessary. This is a bit inefficient, and on a large scale can result in stack overflow (or "RecursionError", because Python catches stack overflow.)
move() refactored I
from enum import Enum
# ...
class Movement
# (direction, magnitude)
LEFT = (0, -1)
RIGHT = (0, 1)
UP = (1, 1)
DOWN = (1, -1)
def move(self, arena_limits, player_pos):
if not self.is_stuck(arena_limits, player_pos):
while True:
movement = random.choice(list(self.Movement))
direction, magnitude = move[0], move[1]
displacement = magnitude * self.speed
in_limits = 0 <= self.position[direction] + displacement <= arena_limits[0]
destination = self.position.copy()
destination[direction] += displacement
if in_limits and destination not in player_pos:
self.position[direction] += displacement
break
But wait, there's more!
Player.is_stuck() does most of the same logic that is repeated continually in Player.move(). By being clever in refactoring and combining code, one can almost like magic eliminate most of the logic, just letting DRY and efficient code take care of the work. Basically, we just developed a general method to find accessible location in move(), so now we can apply it to the starting logic, and get that logic out the way before ever choosing an individual direction:
move() refactored II
def move(self, arena_limits, player_pos):
accessible = self.accessible_positions(arena_limits, player_pos)
if accessible:
movement = random.choice(accessible)
self.position[movement[0]] += movement[1] * self.speed
def accessible_positions(self, arena_limits, player_pos):
accessible =
for m in self.Movement:
move = m.value
direction, magnitude = move
displacement = magnitude * self.speed
destination = self.position.copy()
destination[direction] += displacement
if 0 <= self.position[direction] + displacement <= arena_limits[0] and destination not in player_pos:
accessible.append(move)
return accessible
Use more standard loop flow
Declaring a boolean to serve as a loop check variable is generally unnecessary and can lead to problems (e.g. the loop continuing to do things before it checks arrives at the condition check). while True: ... if whatever: break is generally a more recognizable, more understandable, and more concise idiom.
Conclusion
This answer will stay unfinished, but hopefully the thought process can be useful.
If you are interested in seeing my (somewhat OOP underdeveloped) though process, you can look below. However, 200_success has presented a fleshed out solution which appears to have optimally tuned parts, so I would recommend their solution as a final product. I have no final code at the end of mine.
Here's some suggestions. This answer is an iterative process of explanation.
Initialization
Use if __name__ == '__main__':
In addition to having all of the procedural initialization code at the bottom, prefix it with an if __name__ == '__main__'. This allows other people to import parts of your module (the file) without running the code at the bottom.
(See also: What does if __name__ == âÂÂ__main__âÂÂ: do?)
Python's string formatting options are awesome.
Instead of "Player" + str(i)", do f"Player i". Prefixing a string with f makes the string a formatted string literal, which makes it easier to concatenate string and other variables by just putting the variable in curly brackets. This is new in Python 3.6.
Utilize list comprehensions
Instead of
victims =
for i in range(4):
victims.append(Player(f"Player i", "", 1, 0, [random.randint(0, 6) for i in range(2)]))
just do:
victims = [Player(f"Player i", "", 1, 0, [random.randint(0, 6), random.randint(0, 6)]) for i in range(4)]
DRYer is sometimes only negligibly better
Instead of repeating random.randint(0, 6) twice in [random.randint(0, 6), random.randint(0, 6)], one could just do a list comprehension [random.randint(0, 6) for i in range(2)] In this example, it makes almost no difference, but as you scale up or generalize, it can be useful to consider.
But this is only a taste of DRYer code; more of this idea will be developed later.
Player.move() and Player.is_stuck() methods
Define often-used "constants" as class variables, not local method variables...
Instead of this:
class Player:
# ...
def move(self, arena_limits, player_pos):
change = "left": -1, "right": 1, "up": 1, "down": -1
do this:
class Player:
# ...
change = "left": -1, "right": 1, "up": 1, "down": -1
def move(self, arena_limits, player_pos):
# ...
There's no reason to redefine this each time the method is called.
...and utilize enums
Enums can be incredibly useful in any programming language, because they define a restricted set of values. Instead of defining a dictionary of strings, you can define some enums.
For more information about the why of enums, I would recommend reading this Stack Overflow answer.
Use DRY code
When I see something like the below, I immediately know the code can be made more compact, because the entire control flow is repeated.
if direction in ("left", "right"):
in_limits = 0 <= self.position[0] + (change[direction] * self.speed) <= arena_limits[0]
if in_limits and [self.position[0] + (change[direction] * self.speed), self.position[1]] not in player_pos:
self.position[0] += change[direction] * self.speed
else:
self.move(arena_limits, player_pos)
if direction in ("up", "down"):
in_limits = 0 <= self.position[1] + (change[direction] * self.speed) <= arena_limits[0]
if in_limits and [self.position[0], self.position[1] + (change[direction] * self.speed)] not in player_pos:
self.position[1] += change[direction] * self.speed
else:
self.move(arena_limits, player_pos)
Also you use (change[direction] * self.speed) all over the place in this section, when you could just assign it to a variable (I chose to call it displacement.)
Now, it can be tricky, and require other constructs, and sometimes it's more efficient to repeat code. But generally, 99% of the time, it's a big no-no.
Now, the example I gave is nothing compared to the (apologies) horrendous repetition in this block:
can_go_left = ([self.position[0] + (-1 * self.speed), self.position[1]] not in player_pos) and
0 <= self.position[0] + (-1 * self.speed) <= arena_limits[0]
can_go_right = ([self.position[0] + (1 * self.speed), self.position[1]] not in player_pos) and
0 <= self.position[0] + (1 * self.speed) <= arena_limits[0]
can_go_up = ([self.position[0], self.position[1] + (1 * self.speed)] not in player_pos) and
0 <= self.position[1] + (1 * self.speed) <= arena_limits[1]
can_go_down = ([self.position[0], self.position[1] + (-1 * self.speed)] not in player_pos) and
0 <= self.position[1] + (-1 * self.speed) <= arena_limits[1]
if not (can_go_up or can_go_down or can_go_left or can_go_right):
... but more on that later.
Avoid unnecessary recursion...
...like this (consider it in context):
def move(self, arena_limits, player_pos):
# ...
self.move(arena_limits, player_pos)
This is a bit debatable, but generally it's better practice to program procedurally when it's equal effort. Getting a bit technical, recursion adds variables to the stack when it goes one level deeper, taking up more memory than necessary. This is a bit inefficient, and on a large scale can result in stack overflow (or "RecursionError", because Python catches stack overflow.)
move() refactored I
from enum import Enum
# ...
class Movement
# (direction, magnitude)
LEFT = (0, -1)
RIGHT = (0, 1)
UP = (1, 1)
DOWN = (1, -1)
def move(self, arena_limits, player_pos):
if not self.is_stuck(arena_limits, player_pos):
while True:
movement = random.choice(list(self.Movement))
direction, magnitude = move[0], move[1]
displacement = magnitude * self.speed
in_limits = 0 <= self.position[direction] + displacement <= arena_limits[0]
destination = self.position.copy()
destination[direction] += displacement
if in_limits and destination not in player_pos:
self.position[direction] += displacement
break
But wait, there's more!
Player.is_stuck() does most of the same logic that is repeated continually in Player.move(). By being clever in refactoring and combining code, one can almost like magic eliminate most of the logic, just letting DRY and efficient code take care of the work. Basically, we just developed a general method to find accessible location in move(), so now we can apply it to the starting logic, and get that logic out the way before ever choosing an individual direction:
move() refactored II
def move(self, arena_limits, player_pos):
accessible = self.accessible_positions(arena_limits, player_pos)
if accessible:
movement = random.choice(accessible)
self.position[movement[0]] += movement[1] * self.speed
def accessible_positions(self, arena_limits, player_pos):
accessible =
for m in self.Movement:
move = m.value
direction, magnitude = move
displacement = magnitude * self.speed
destination = self.position.copy()
destination[direction] += displacement
if 0 <= self.position[direction] + displacement <= arena_limits[0] and destination not in player_pos:
accessible.append(move)
return accessible
Use more standard loop flow
Declaring a boolean to serve as a loop check variable is generally unnecessary and can lead to problems (e.g. the loop continuing to do things before it checks arrives at the condition check). while True: ... if whatever: break is generally a more recognizable, more understandable, and more concise idiom.
Conclusion
This answer will stay unfinished, but hopefully the thought process can be useful.
edited Aug 3 at 7:35
answered Aug 3 at 7:29
Graham
1639
1639
add a comment |Â
add a comment |Â
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
StackExchange.ready(
function ()
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f200857%2fa-game-of-automated-tag%23new-answer', 'question_page');
);
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
You said that
healthis reserved for future improvements, but there is no real point in havingspeedeither. Do you plan to use different values in the future too?â Mathias Ettinger
Aug 3 at 7:05
@MathiasEttinger
speedis used in themoveandis_stuckfunctionsâ Anthony Pham
Aug 3 at 15:30
Yes, but speed is always 1. So, unless you plan to have it varying in the future, I see no point in keeping it around.
â Mathias Ettinger
Aug 3 at 15:48
Yes I am going to vary it, I currently have it at one since it is more simple to test
â Anthony Pham
Aug 3 at 16:47