A Python based Tic-Tac-Toe game

Question

As a basic project, I've programmed the game of noughts and crosses (tic-tac-toe) using classes. I made three classes: Board, Player and LazyRobot, as well as the main game file.

I would like to know any improvements to code or any best practices I have neglected. I know there is some repetition within the code, but I would like to hear your thoughts.

Below is the main game.py file:

from copy import deepcopy
from board import Board, PositionError
from player import Player
from lazyrobot import LazyRobot
def game(player1, player2):
 board = Board()
 game_winner = None
 turn = player1
 print('Starting new game.')
 print('{} is playing noughts.'.format(player1.getname()))
 print('{} is playing crosses.'.format(player2.getname()))
 board.display_board()
 while len(board.available_positions()) != 0:
 if turn == player1:
 nought_turn(player1, board)
 if board.is_winner() is True:
 game_winner = player1
 break
 else:
 turn = player2
 else:
 cross_turn(player2, board)
 if board.is_winner() is True:
 game_winner = player2
 break
 else:
 turn = player1
 if game_winner is None:
 print('Game is tied.')
 else:
 print('{} has won the game!'.format(game_winner.getname()))
 print('End of game.')
def nought_turn(player, board):
 print('It is now {}\'s turn. (o)'.format(player.getname()))
 while True:
 try:
 move = player.getmove(deepcopy(board))
 board.add_nought(move)
 break
 except PositionError:
 print('Sorry, the space has been taken. '
 'Please try another space.')
 board.display_board()
def cross_turn(player, board):
 print('It is now {}\'s turn. (x)'.format(player.getname()))
 while True:
 try:
 move = player.getmove(deepcopy(board))
 board.add_cross(move)
 break
 except PositionError:
 print('Sorry, the space has been taken. '
 'Please try another space.')
 board.display_board()

Here is the Board class:

class PositionError(Exception):
 pass
class Board(object):
 def __init__(self):
 self.board = {1: ' ', 2: ' ', 3: ' ', 4: ' ',
 5: ' ', 6: ' ', 7: ' ', 8: ' ', 9: ' '}
 self.nought_positions = []
 self.cross_positions = []
 def display_board(self):
 line_1 = ' {} | {} | {} '.format(self.board[1],
 self.board[2], self.board[3])
 line_2 = ' {} | {} | {} '.format(self.board[4],
 self.board[5], self.board[6])
 line_3 = ' {} | {} | {} '.format(self.board[7],
 self.board[8], self.board[9])
 print(line_1)
 print('-----------')
 print(line_2)
 print('-----------')
 print(line_3)
 def add_nought(self, position):
 if self.board[position] == ' ':
 self.board[position] = 'o'
 self.nought_positions.append(position)
 else:
 raise PositionError('Space is occupied.')
 def add_cross(self, position):
 if self.board[position] == ' ':
 self.board[position] = 'x'
 self.cross_positions.append(position)
 else:
 raise PositionError('Space is occupied.')
 def is_winner(self):
 winning_positions = [{1,2,3}, {4,5,6},
 {7,8,9}, {1,4,7},
 {2,5,8}, {3,6,9},
 {1,5,9}, {3,5,7}]
 nought_set = set(self.nought_positions)
 cross_set = set(self.cross_positions)
 for position in winning_positions:
 if nought_set & position == position:
 return True
 elif cross_set & position == position:
 return True
 return False
 def available_positions(self):
 nought_set = set(self.nought_positions)
 cross_set = set(self.cross_positions)
 all_positions = {1,2,3,4,5,6,7,8,9}
 available_positions = (all_positions - nought_set) - cross_set
 return list(available_positions)

Here is the Player class:

class PositionError(Exception):
 pass
class NumberError(Exception):
 pass
class Player(object):
 def __init__(self, name):
 self.name = name
 def getname(self):
 return self.name
 def getmove(self, board):
 available_positions = board.available_positions()
 print('{}, make your next move.'.format(self.getname()))
 while True:
 try:
 move = int(input('Number: '))
 if move > 9 or move < 1:
 raise NumberError
 if move not in available_positions:
 raise PositionError
 return move
 except ValueError:
 print('Sorry, please type a number.')
 except NumberError:
 print('Sorry, please type a number between 1 and 9.')
 except PositionError:
 print('Sorry, the space has been taken. '
 'Please try another space.')

And here is the LazyRobot class: (I'm not too worried about this, as I will make a robot with better strategy.)

from random import randint
class LazyRobot(object):
 def __init__(self, name):
 self.name = name
 def getname(self):
 return self.name
 def getmove(self, board):
 available_positions = board.available_positions()
 index = randint(0, len(available_positions)-1)
 return available_positions[index]

Answer 1

PEP 8

Overall, your code is very clean. However, there are a few minor PEP-8 deviations:

1. .getname() should be .get_name(), as it is two words.

2. In several places, where a statement continues over multiple lines, the indentation is not PEP-8 compliant, such as:

    self.board = {1: ' ', 2: ' ', 3: ' ', 4: ' ',
    5: ' ', 6: ' ', 7: ' ', 8: ' ', 9: ' '}
   

should be:

 self.board = {1: ' ', 2: ' ', 3: ' ', 4: ' ',
 5: ' ', 6: ' ', 7: ' ', 8: ' ', 9: ' '}

(But perhaps this was an artifact of copying the code to the Code Review site.)

3. Commas should be followed by one space. Eg)

    all_positions = {1,2,3,4,5,6,7,8,9}
   

   should be:

    all_positions = {1, 2, 3, 4, 5, 6, 7, 8, 9}
   

4. Raise Objects not Classes

    raise PositionError
   

   should be:

    raise PositionError()
   

   or:

    raise PositionError("Your message here")
   

Classes

Deriving from object is implicit:

class Board(object):
 ...

should simply be written as:

class Board:
 ...

Imports

Both board.py and player.py define the class:

class PositionError(Exception):
 pass

player.py should simply import PositionError from board, like was done in game.py:

from board import Board, PositionError

f-strings

Beginning with Python 3.6, formatting strings has become easier. Instead of separating the format code and the format argument be a large distance, like "...{}...{}...{}".format(a, b, c)", f-strings allow you to embed the argument inside the format code: f"...{a}...{b}...{c}". Note the leading f in f"...".

So this code:

 print('{} is playing noughts.'.format(player1.getname()))

can be rewritten like this:

 print(f'{player1.getname()} is playing noughts.')

(Note: I'll be revisiting this below, and make it even cleaner.)

Code Duplication

game() has virtually identical code in both branches of the if ... else inside the while loop. The largest difference is one branch calls nought_turn() while the other calls cross_turn().

nought_turn() and cross_turn() also have virtually identical code. The first difference is one displays a nought (o) while the other displays a cross (x). The second being the former calls .add_nought() and the latter calls .add_cross().

add_nought() and add_cross() also have virtually identical code. The first difference being one stores 'o' in self.board while the other stores 'x'. The second difference being the position is added to either self.nought_positions or self.cross_positions.

The code above is the same, only the data (the symbol) is different. Let's pass the symbol down. Instead of nought_turn(player1, board), we could use player_turn(player1, board, 'o') and instead of cross_turn(player2, board), we could use player_turn(player2, board, 'x'). player_turn() might look like this:

def player_turn(player, board, symbol):
 print(f"It is now {player.getname()}'s turn. ({symbol})")
 
 while True:
 try:
 move = player.getmove(deepcopy(board))
 board.add_symbol(move, symbol)
 break
 except PositionError:
 print('Sorry, the space has been taken. '
 'Please try another space.')
 
 board.display_board()

Tiny changes. We've added {symbol} to the print statement, and changed .add_nought(...) and .add_cross(...) to .add_symbol(..., symbol). But a big change; we've removed an entire duplicate code function.

add_symbol() would replace add_nought() and add_cross():

 def add_symbol(self, position, symbol):
 if self.board[position] == ' ':
 self.board[position] = symbol
 if symbol == 'o':
 self.nought_positions.append(position)
 else:
 self.cross_positions.append(position)
 else:
 raise PositionError('Space is occupied.')

We've had to add an if ... else in the add_symbol() to append the position to the correct positions list, but again we've removed an entire extra duplicate code function, so that's a win.

Let's look at that revised main loop again:

 turn = player1
 while len(board.available_positions()) != 0:
 if turn == player1:
 player_turn(player1, board, 'o')
 if board.is_winner() is True:
 game_winner = player1
 break
 else:
 turn = player2
 else:
 player_turn(player2, board, 'x')
 if board.is_winner() is True:
 game_winner = player2
 break
 else:
 turn = player1

We're passing player1 or player2 into the player_turn() function. But turn is the current player, so we could just pass in turn. Or better: rename the variable to player! Ditto for game_winner = .... Then we just need to determine the correct symbol for the current player.

 player = player1
 while len(board.available_positions()) != 0:
 symbol = 'o' if player == player1 else 'x'
 player_turn(player, board, symbol)
 if board.is_winner() is True:
 game_winner = player
 break
 player = player2 if player == player1 else player1

nought & cross positions

What are these?

 self.nought_positions = []
 self.cross_positions = []

You .append(position) to them when moves are successfully made. But they are only ever used in:

 nought_set = set(self.nought_positions)
 cross_set = set(self.cross_positions)

and nought_set and cross_set are never modified or destroyed.

So you are maintaining a list and generating the corresponding set when used. Why not just maintain sets from the get-go?

 self.nought_set = set()
 self.cross_set = set()

and use .add(position) to add position to the set when a move is successfully made.

Available positions

You are doing too much work with all_positions:

 all_positions = {1,2,3,4,5,6,7,8,9}

The keys of self.board are all of the positions. So the above statement can be replaced with simply:

 all_positions = set(self.board)

If you changed the keys to be 0 through 8, or 'A' through 'I', using set(self.board) would automatically construct the correct set; you wouldn't need to change the hard-coded {1,2,3,4,5,6,7,8,9} set to the correct values, so is a win for program maintenance.

But wait. From all_positions, you subtract nought_set and cross_set. This leaves all the self.board positions which still have the value " ". There is an easier way:

 def available_positions(self):
 return [position for position, value in self.board.items() if value == " "]

No need for the nought_set or cross_set here!

Is Winner

Each call, you are checking if noughts has won or crosses has won. This is double the required work. Nought can only win on nought's turn, and cross can only win on cross's turn.

If you pass in the symbol that just played, you could eliminate half of the checks:

 def is_winner(self, symbol):
 winning_positions = [{1,2,3}, {4,5,6},
 {7,8,9}, {1,4,7},
 {2,5,8}, {3,6,9},
 {1,5,9}, {3,5,7}]
 if symbol == 'o':
 player_set = set(self.nought_positions)
 else:
 player_set = set(self.cross_positions)
 for position in winning_positions:
 if player_set & position == position:
 return True
 return False

player_set & position == position takes two sets, constructs a new set which is the intersection of the two sets, and compares the result with the second set. This is, effectively, an "is position a subset of player_set" test. And .issubset(other) is built-in to Python:

 for position in winning_positions:
 if position.issubset(player_set):
 return True
 return False

The position.issubset(player_set) test can also be written as position <= player_set.

Looping over a collection of items and testing if a condition is True for any of the items is an any() test ... also built into Python:

 return any(position <= player_set for position in winning_positions)

But do we really need the nought/cross _positions/_set? We got rid of them in available_positions().

We want any winning_positions where all positions have the current players symbol. If we move winning_positions out of the function and made it a global constant:

WINNING_POSITIONS = ((1, 2, 3), (4, 5, 6), (7, 8, 9),
 (1, 4, 7), (2, 5, 8), (3, 6, 9),
 (1, 5, 9), (3, 5, 7))

Then,

 def is_winner(self, symbol):
 return any(all(self.board[cell] == symbol for cell in position)
 for position in WINNING_POSITIONS)

Now, there is no more references to nought_positions or cross_set. Those can be removed globally!

Private members

In the Board class, self.board is a private member; no external class should manipulate it. In Python, there are no private/public access modifiers which will enforce a member being private. But there are conventions.

Members with a leading underscore are - by convention - private. Many IDE's will omit those identifiers when providing auto-complete hints. Additionally from some_module import * will not import any items beginning with an underscore, so it is a bit more than convention.

So:

• self.board should be changed to self._board
• self.name should be changed to self._name

When they existed:

• self.nought_positions should have been self._nought_positions
• self.cross_positions should have been self._cross_positions

Properties

In Player, the method getname() exists because player.name is supposed to be a private member, not accessed directly. You wouldn't want external code to accidentally, or maliciously change a player's name:

if game_winner != player1:
 player1.name = "The loser"

But player1.name is so convenient to use. It is the player's name. What could be simpler than typing player1.name?

We can do that. Safely.

class Player:
 def __init__(self, name):
 self._name = name
 @property
 def name(self):
 return self._name

Now we can access a Player objects .name property safely, and simply. We can fetch the value, but we can't change it:

>>> p = Player("Fred")
>>> p.name
'Fred'
>>> p.name = "Bob"
Traceback (most recent call last):
 File "<stdin>", line 1, in <module>
AttributeError: can't set attribute
>>> 

Which leads to even clearer print format statement I hinted about earlier:

 print(f'{player1.name} is playing noughts.')

Deep Copy

 move = player.getmove(deepcopy(board))

Neither Player nor LazyRobot modify board during getmove(board). Why the deep copy?

If it is when you make a better robot player, perhaps the robot player should do the deep copy? Or add a board.copy() method which can return a duplicate Board for the player to scribble on while they try to determine the optimal move.

• Lewis T.: Would I then need to declare WINNING_POSITIONS as a global variable within the Board class: global WINNING_POSITIONS?
• Lewis T.: Also, I'm worried that if the robot was passed board through getmove(board), it might hack the board to a winning position. My LazyRobot uses board.available_positions(), so I think I would need to pass a copy of the board with all of its methods.
• AJNeufeld: You do not need to use the global keyword. Because you have a board.py file, it is fine to have "board specific" constants declared at file level; they are encapsulated in the board.py module scope. If you want to declare it within the class Board scope, that is fine too. You would refer to it as either Board.WINNING_POSITIONS or self.WINNING_POSITIONS inside Board.is_winner(self).
• AJNeufeld: I would add to class Board the method def copy(self): return deepcopy(self), and then use move = player.get_move(board.copy()). This is cleaner & will allow you to change copy() in the future if desired. I wouldn’t be worried that a Robot could hack the board. This is Python; the Robot can do anything, including changing WINNING_POSITIONS to ((2,4,9),). Or intercept sys.stdout and change the game’s output text! Or redefine the game.py’s player_turn() method, to give the Robot two or more turns in a row! But passing a copy to prevent accidental scribbles is reasonable.