A game of automated tag

The name of the pictureThe name of the pictureThe name of the pictureClash 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.health is currently not used but is planned on being used

  • The field is 8x8 and each person can move once in one of the four directions. If they are stuck is_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?







share|improve this question





















  • 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










  • @MathiasEttingerspeed 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 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
















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.health is currently not used but is planned on being used

  • The field is 8x8 and each person can move once in one of the four directions. If they are stuck is_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?







share|improve this question





















  • 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










  • @MathiasEttingerspeed 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 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












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.health is currently not used but is planned on being used

  • The field is 8x8 and each person can move once in one of the four directions. If they are stuck is_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?







share|improve this question













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.health is currently not used but is planned on being used

  • The field is 8x8 and each person can move once in one of the four directions. If they are stuck is_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?









share|improve this question












share|improve this question




share|improve this question








edited Aug 3 at 3:22









200_success

123k14143398




123k14143398









asked Aug 2 at 21:29









Anthony Pham

4501620




4501620











  • 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










  • @MathiasEttingerspeed 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 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










  • @MathiasEttingerspeed 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 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












@MathiasEttingerspeed is used in the move and is_stuck functions
– Anthony Pham
Aug 3 at 15:30




@MathiasEttingerspeed 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










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)





share|improve this answer























  • 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 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










  • 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


















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.






share|improve this answer























    Your Answer




    StackExchange.ifUsing("editor", function ()
    return StackExchange.using("mathjaxEditing", function ()
    StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix)
    StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
    );
    );
    , "mathjax-editing");

    StackExchange.ifUsing("editor", function ()
    StackExchange.using("externalEditor", function ()
    StackExchange.using("snippets", function ()
    StackExchange.snippets.init();
    );
    );
    , "code-snippets");

    StackExchange.ready(function()
    var channelOptions =
    tags: "".split(" "),
    id: "196"
    ;
    initTagRenderer("".split(" "), "".split(" "), channelOptions);

    StackExchange.using("externalEditor", function()
    // Have to fire editor after snippets, if snippets enabled
    if (StackExchange.settings.snippets.snippetsEnabled)
    StackExchange.using("snippets", function()
    createEditor();
    );

    else
    createEditor();

    );

    function createEditor()
    StackExchange.prepareEditor(
    heartbeatType: 'answer',
    convertImagesToLinks: false,
    noModals: false,
    showLowRepImageUploadWarning: true,
    reputationToPostImages: null,
    bindNavPrevention: true,
    postfix: "",
    onDemand: true,
    discardSelector: ".discard-answer"
    ,immediatelyShowMarkdownHelp:true
    );



    );








     

    draft saved


    draft discarded


















    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






























    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)





    share|improve this answer























    • 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 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










    • 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















    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)





    share|improve this answer























    • 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 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










    • 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













    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)





    share|improve this answer















    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)






    share|improve this answer















    share|improve this answer



    share|improve this answer








    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 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










    • 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










    • 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










    • 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
















    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













    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.






    share|improve this answer



























      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.






      share|improve this answer

























        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.






        share|improve this answer















        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.







        share|improve this answer















        share|improve this answer



        share|improve this answer








        edited Aug 3 at 7:35


























        answered Aug 3 at 7:29









        Graham

        1639




        1639






















             

            draft saved


            draft discarded


























             


            draft saved


            draft discarded














            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













































































            Popular posts from this blog

            Python Lists

            Aion

            JavaScript Array Iteration Methods