I'm learning Python after a 30+ year break from programming, got bored with the usual TicTacToe exercise, so went a bit wild adding functionality to it to make something that's a bit more fun to play than losing to the computer every time.
Now reached the stage of aimlessly noodling around with it, which suggests to me that fresh, and experienced, eyes would be handy. I would very much welcome a review of any/all aspects of the code. Be brutal - I can take it!
A few points:
It is written as a module, as I'm rubbish at UIs and I want to use this to practise with. Would it help if I also posted something that used the module? I have a fairly scraggy tkinter thing that I am using as a testbed if that would help.
It works. Rather nicely I think - in fact I am quite proud of it. This of course is a Big Red Flag that a code review is needed ;)
Nevertheless, it is possible that I have grossly abused something-or-other, but I've tried hard to get PEP8/PEP257 stuff right, and to comment lightly and selectively. So it shouldn't be too hard to work out what I am driving at. I hope.
It's a bit big - runs to 8 pages printed, sorry about that.
"""Module to support playable versions of TicTacToe (Noughts and Crosses)
by phisheep 2017
VARIABLES (all should be treated as read-only)
- USER, COMPUTER, EMPTY - Player objects
- cells[] - list of Cell objects representing the playing board
- lines[] - list of Line objects ditto
- moves[] - list of Cell objects representing moves made in current game
- game_over - boolean
- winning_line - Line object, or None
- winning_player - Player object, or None
CLASSES
- Player() - One for each player, and EMPTY
player.symbol - arbitrary symbols for each player, default 'X', 'O' and ' '
player.opponent - the other (non-EMPTY) player
- Cell() - One for each cell on the board
cell.lines[] - list of Lines this cell is in
cell.set(player) - sets a cell and checks for win/loss/draw
- Line() - One for each row, column and diagonal on the board
line.cells[] - list of Cells in this line
line._value - SUM of the player values in this line
PUBLIC FUNCTIONS
- reset_board() - clears the board for a new game
- available_cells() - returns list of available Cells
- board_list_symbols() - returns list of values on the board
- board_XY_symbols() - return 3x3 list of symbols on the board
- undo() - undoes moves to the previous USER move
- hints(player) - returns list of Cells that a good opponent
might want to move in
- play(difficulty,player) - returns the Cell computer has moved in
(after making the move!)
INTERNAL FUNCTIONS
These fall into three groups:
- _play_... one for each difficulty setting (returning a completed Cell, or None),
which call...
- _make_... to make a particular sort of move (returning a completed Cell, or None),
which may call ...
- _list_possible_... to list candidate moves (returning a list of Cells)
ACKNOWLEDGEMENTS
Harvey, B (1997), Computer Science Logo Style: Volume 1 Symbolic Computing (2nd Edn),
MIT, Cambridge Mass
ISBN 0–262–58151–5 (set of 3 volumes)
ISBN 0–262–58148–5 (volume 1 only)
Chapter 6 at this link
https://people.eecs.berkeley.edu/~bh/pdf/v1ch06.pdf
... for detailed discussion of tactics
"""
import random
_CENTRE = _EDGES = _CORNERS = tuple()
cells = []
lines = []
moves = []
game_over = False
winning_line = None
winning_player = None
class Player:
""" One instance for each of the two players USER and COMPUTER, also NULL """
def __init__(self, value, symbol):
self._value = value
self.symbol = symbol
@property
def opponent(self):
if self == USER:
return COMPUTER
return USER
# These assignments are here (rather than in __init__()) as they are used as default
# arguments in function definitions further down
USER = Player(10, "X")
COMPUTER = Player(1, "O")
EMPTY = Player(0, " ")
class Cell:
""" One instance for each cell in the TicTacToe board.
ATTRIBUTES
- player - a Player object: USER, COMPUTER or EMPTY
- index - cell's position in cells[] list
- xpos, ypos - cell's position in x,y co-ordinates
- lines[] - Line objects, populated by Line() instance
FUNCTIONS
- set(value) - sets a cell value and end-game variables
"""
def __init__(self, i):
self.index = i
self.player = EMPTY
self.xpos = self.index % 3
self.ypos = self.index // 3
self.lines = []
def set(self, player=USER):
""" Return a filled-in cell, having checked for end of game """
global game_over, winning_line, winning_player
if game_over:
raise RuntimeError("Attempt to make a move when the game is over")
if player not in [USER, COMPUTER]:
raise ValueError("Cell must be set to either USER or COMPUTER")
self.player = player
moves.append(player) # recorded in case of undo
for line in self.lines:
if line._value == 3 * self.player._value:
game_over = True
winning_line = line
winning_player = self.player
return self
if not available_cells():
game_over = True
return self
def _reset(self):
self.player = EMPTY
pass
class Line:
""" One instance for each row, column and diagonal
ATTRIBUTES
- cells[] - list of Cells in this line
- value - (read-only) SUM of player values in cells in this line
(used for testing line contents)
"""
def __init__(self, *args):
self.cells = [cells[i] for i in args]
for cell in self.cells:
cell.lines.append(self)
@property
def _value(self):
""" Return total value of cells in this line. """
return sum([cell.player._value for cell in self.cells])
def __init__():
""" Initialise classes and module variables """
# Warning: contains 'magic numbers' specific to this TicTacToe
# implementation. All gathered here so they don't clutter up
# the rest of the code.
#
# (... Except the Player objects, which had to be moved out
# because of getting default arguments right)
global _CENTRE, _EDGES, _CORNERS
for i in range(9): # cells
cells.append(Cell(i))
for i in range(0, 9, 3): # rows
lines.append(Line(i, i + 1, i + 2))
for i in range(3): # columns
lines.append(Line(i, i + 3, i + 6))
lines.append(Line(0, 4, 8)) # diagonals
lines.append(Line(2, 4, 6))
_CENTRE = (cells[4],)
_EDGES = (cells[1], cells[3], cells[5], cells[7])
_CORNERS = (cells[0], cells[2], cells[6], cells[8])
# PUBLIC FUNCTIONS
def reset_board():
""" Reset board to starting position. """
global game_over, winning_line, winning_player
for cell in cells:
cell._reset()
moves.clear()
game_over = False
winning_line = None
winning_player = None
def available_cells():
""" Return list of empty cells. """
return [cell for cell in cells if cell.player == EMPTY]
def board_list_symbols():
""" Return list of all cell symbols. """
return [cell.player.symbol for cell in cells]
def board_XY_symbols():
""" Return 3x3 list of all cell symbols. """
output = [] * 3
for cell in cells:
output[cell.ypos].append(cell.player.symbol)
return (output)
def undo():
""" Undo moves back to last *user* move. """
global game_over, winning_line, winning_player
if game_over: # because it's not over any more!
game_over = False
winning_line = None
winning_player = None
if len(moves) != 0:
last_move = moves[-1]
if last_move.player == COMPUTER:
last_move._reset()
moves.pop()
if len(moves) != 0:
last_user_move = moves[-1]
last_user_move._reset()
moves.pop()
def hints(player=USER):
""" Return list of possible winning moves by opponent. """
return (_list_possible_wins_blocks(player.opponent) or
_list_possible_wins_blocks(player) or
_list_possible_forks(player.opponent) or
_list_possible_forcing_moves(player.opponent))
def play(difficulty, player=COMPUTER):
""" Execute a playing strategy, returning the cell moved in. """
# (This is a little scrappy and could do with tidying up)
#
move = [_play_loser, _play_random, _play_easy, _play_hard,
_play_handicap_2, _play_handicap_1, _play_expert]
if difficulty not in range(len(move)):
raise ValueError("No such difficulty setting - " + str(difficulty))
return move[difficulty](player)
# PLAYING STRATEGIES
def _play_loser(player=COMPUTER):
""" Deliberately play to help opponent win. """
#
# We use the hints() function to identify moves that a sane
# opponent would play, subtract those from the available
# cells, and move in any of the cells that remain. If
# none, take edges for preference.
#
candidates = set(available_cells())
candidates.difference_update(set(hints(player)))
if len(candidates) > 0:
cell = candidates.pop()
return cell.set(player)
return (_make_random_move(_EDGES, player) or
_play_random(player))
def _play_random(player=COMPUTER):
""" Play in random available cell. """
return _make_random_move(cells, player)
def _play_easy(player=COMPUTER):
""" Complete winning line if possible, else play at random. """
return (_make_win_or_block(player, player) or
_play_random(player))
def _play_hard(player=COMPUTER):
""" Complete or block winning lines, prefer centre then corners. """
return (_make_win_or_block(player, player) or
_make_win_or_block(player.opponent, player) or
_make_random_move(_CENTRE, player) or
_make_random_move(_CORNERS, player) or
_play_random(player))
def _play_handicap_2(player=COMPUTER):
""" First two moves random, then expert """
if len(moves) < 5:
return _play_random(player)
return _play_expert(player)
def _play_handicap_1(player=COMPUTER):
""" First move to edge, then play as expert """
if len(moves) < 3:
return _make_random_move(_EDGES, player)
return _play_expert(player)
def _play_expert(player=COMPUTER):
""" Play expertly to not lose, the traditional 'perfect' TicTacToe """
return (_make_win_or_block(player, player) or
_make_win_or_block(player.opponent, player) or
_make_forked_move(player) or
_make_forcing_move(player) or
_make_random_move(_CENTRE, player) or
_make_random_move(_CORNERS, player) or
_play_random(player))
# Possible extensions:
# - add play_cunning() to maximise winning chances against naive opponent
# - add progressive play to adjust difficulty to user's skill level
# FUNCTIONS - UTILITIES FOR COMPUTER MOVE STRATEGIES
def _make_random_move(target_cells, player=COMPUTER):
""" Return completed cell chosen from target_cells if available. """
cells_list = []
for cell in target_cells:
if cell.player == EMPTY:
cells_list.append(cell)
if len(cells_list) != 0:
cell = random.choice(cells_list)
return cell.set(player)
return None
def _list_possible_wins_blocks(look_for_player):
""" Return list of cells with a winning move available. """
possible_cells = []
for cell in available_cells():
for line in cell.lines:
if line._value == 2 * look_for_player._value + 0:
possible_cells.append(cell)
return possible_cells
def _make_win_or_block(look_for_player, fill_with_player):
""" Return a completed cell that wins or blocks a line, or None """
possible_cells = _list_possible_wins_blocks(look_for_player)
if len(possible_cells) != 0:
cell = random.choice(possible_cells)
return cell.set(fill_with_player)
return None
def _list_possible_forks(player=COMPUTER):
""" Return list of available forking moves. """
#
# We're looking for two lines, each containing just one entry for
# 'player', that intersect in an empty cell. That's the cell we
# want to play in.
#
possible_forks = []
for cell in available_cells():
forks = 0
for line in cell.lines:
if line._value == player._value + 0 + 0:
forks += 1
if forks > 1:
possible_forks.append(cell)
return possible_forks
def _make_forked_move(player=COMPUTER):
""" Return completed forking move, or None. """
my_forks = _list_possible_forks(player)
if len(my_forks) == 0:
return None
cell = random.choice(my_forks)
return cell.set(player)
def _list_possible_forcing_moves(player=COMPUTER):
""" Find a forcing move that does not make opponent fork us. """
#
# This one is a bit tricky. We are looking to force the opponent to
# block us, by having two cells in a line - if he doesn't block, we
# get to win next turn. BUT we must avoid 'forcing' him into a move
# that wins the game for him.
#
# So first we find what possible forking moves he has, then we look
# for our candidate lines and extract the two blank cells. If one
# of those cells is *not* a possible forking move for the opponent,
# we have to move in the *other* one.
#
# Got that? Good. Took me ages.
#
opponent_forks = _list_possible_forks(player.opponent)
my_moves = set()
for line in lines:
if line._value == player._value + 0 + 0:
temp = [cell for cell in line.cells if cell.player == EMPTY]
for i, cell in enumerate(temp):
if temp[i] not in opponent_forks:
# move in the *other* empty cell on the line
my_moves.add(temp[(i + 1) % 2])
return list(my_moves)
def _make_forcing_move(player=COMPUTER):
""" Return completed forcing move, or None. """
cell_list = _list_possible_forcing_moves(player)
if len(cell_list) == 0:
return None
cell = random.choice(cell_list)
return cell.set(player)
#
# And, right down here, is the call to initialise the playing board.
# As an old COBOL hand I can't get used to programming upside-down.
#
if __name__ != '__main__':
__init__()
1 Answer 1
You wrap your public variables and functions into another class, called Game or TicTacToe. This way you can instantiate your game by just importing the class, then doing
game = TicTacToe()
game.start()
and having that class control the game.
Right now you are basically treating your global namespace like a class, with global functions as classes, and __init__ as the constructor, that you call excplicitly. Functions/methods with leading and trailing double underscores should normally never be called explicitly.
The constructor of your Player class takes a parameter value. What is that value? Better use a name that tells you what kind of value it is (e. g. level, age, etc.).
The Player class has this property:
@property
def opponent(self):
if self == USER:
return COMPUTER
return USER
It returns the USER instance of Player in any case where it is not the USER itself. But what if you ask the EMPTY player that you are using for empty fields what its opponent is? Maybe you could instead use a variable to save the opponent and set it where you instantiate the players. It is not good practice to have the internal behaviour of classes depend on global state.
Having classes for Line and Cell seems a bit too much. Rather have a class Board which saves the positions of the players' symbols and checks when the winning-condition is fulfilled. If you want to use the Line and Cell classes to better organize your code, make them private classes of the Board, so that no user (even you) of your code creates instances of them where they are not needed.
This is hard to read and seems a bit like a math puzzle:
for i in range(0, 9, 3): # rows
lines.append(Line(i, i + 1, i + 2))
for i in range(3): # columns
lines.append(Line(i, i + 3, i + 6))
Rather write 6 lines (of code) instead of these 4 and write the values excplicitly. Using for loops here hardly gives you any value, neither concerning readability nor performance.
In reset_board you call this:
for cell in cells:
cell._reset()
But remember that method names starting with underscores are used for private methods, and thus should not be called from outside of the class. Using a Board class enables you to add a single reset() method to it that resets all cells and lines.
Explore related questions
See similar questions with these tags.