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.