3
\$\begingroup\$

Just for practice, I am writing a tic tac toe game in Python 3.4. My main goal is to make expandable, for example allowing larger boards and more than two players.

The way my project is arranged is something like:

  • lib.py - All the functions and classes needed for a tictactoe game
  • ai.py - Functions for the artificial intelligence
  • tictactoe.py - The actual game logic
  • ui.py - Any functions or classes for displaying things that will be used in tictactoe.py
  • tests.py - Unit tests for the ui, ai, and lib modules.

The code I am concerned about is in lib.py. The content of lib.py is:

  • class PlayerError - A basic exception class for exceptions encountered in Player class.
  • class BoardError - A basic exception class for exceptions encountered in Board class.
  • class Player - This player class takes a single letter as an argument, and stores it in self.letter. That is all it does atm, but more functions will probably be added.
  • class Board - This is the most important class of this module. It takes the following arguments: size (a tuple containing the width/height of the board), players (a list of player classes), and win_length (the number of adjacent tiles required to win a game).

Here is the code in lib.py:

"""Module for use in a tictactoe game."""

#
## Classes
#

## Exception Classes


class BoardError(Exception):
    """Exception for any error encountered in Board class"""
    pass


class PlayerError(Exception):
    """Exception for any error encountered in Player class"""
    pass

## Normal Classes


class Player(object):
    """Class which represents a player.  This can be used to create more than
    two players.  Letter argument is the letter you want the player to be
    represented by."""
    def __init__(self, letter):
        if (not type(letter) == str or not len(letter) == 1
                or not letter.isalpha()):
            raise PlayerError("""Invalid player letter, must be one character
                                and must be string.""")
        self.letter = letter


class Board(object):
    """This class represents a tictactoe board.  Size is a tuple which
    contains the width/height of the board, turn is a list of player objects,
    and win_length is the amount of adjacent tiles required to win a game."""
    def __init__(self, size, players, win_length):
        if (type(size) != tuple or len(size) != 2 or type(size[0]) != int or
                type(size[1]) != int or size[0] < 1 or size[1] < 1):
            raise BoardError("""Size needs to be a tuple of two integers that
                                are greater than 0""")

        if type(players) != list:
            raise BoardError("Players argument must be a list")
        elif len(players) < 2:
            raise BoardError("Players list msut have at least two items")
        elif False in [type(player) == Player for player in players]:
            raise BoardError("""Players argument must be a list of player
                                objects""")
        player_letters = [player.letter for player in players]
        if any(player_letters.count(letter) > 1 for letter in player_letters):
            raise BoardError("Players can't use the same letter as each other")


        if type(win_length) != int:
            raise BoardError("Winning length must be integer")
        elif win_length > size[0] or win_length > size[1]:
            raise BoardError("""Winning win_length cannot be larger than the
                                board""")
        elif win_length < 1:
            raise BoardError("Winning length cannot be less than 1")

        self.tile_list = [' '] * (size[0] * size[1])
        self.size = size
        self.players = players
        self.opponents = players[1:]
        self.turn = players[0]
        self.win_length = win_length
        self.last_move = ()

    def set_tile(self, value, pos):
        """Set a tile on the board.  Letter is the value to set it to, and
        pos is the position of the tile.  Pos is a tuple containing an x/y
        value."""
        if (type(pos) != tuple or len(pos) != 2 or type(pos[0]) != int or
                type(pos[1]) != int):
            raise BoardError("""Pos must be a tuple which contains two
                                integers""")

        if (pos[0] >= self.size[0] or pos[1] >= self.size[1]
                or pos[0] < 0 or pos[1] < 0):
            raise BoardError("""Pos can't be larger than board size or less
                                than zero""")

        self.tile_list[
            pos[0] + pos[1] * self.size[1]
        ] = value

    def move(self, pos):
        """Set tile at pos to the letter of the player whose turn it currently
        is.  Pos is a tuple containing an x/y value.  Return False if tile is
        already taken, return True if tile is not taken.  Automatically
        switches self.turn to next player if tile isn't taken."""
        if not self.get_tile(pos) == ' ':
            return False
        else:
            self.set_tile(self.turn.letter, pos)
            self.opponents.append(self.turn)
            self.turn = self.opponents.pop(0)
            self.last_move = pos
            return True

    def get_tile(self, pos):
        """Return a tile's value from a given position.  Pos is a tuple
        containing an x/y value."""
        if (type(pos) != tuple or len(pos) != 2 or type(pos[0]) != int or
                type(pos[1]) != int):
            raise BoardError("""Pos must be a tuple which contains two
                                integers""")

        if (pos[0] >= self.size[0] or pos[1] >= self.size[1]
                or pos[0] < 0 or pos[1] < 0):
            raise BoardError("""Pos can't be larger than board size or less
                                than zero""")

        return self.tile_list[
            pos[0] + pos[1] * self.size[1]
        ]

    def index_to_pos(self, index):
        """Take a list index to self.tile_list and convert it to a position
        for use in get_tile and set_tile"""
        if type(index) != int:
            raise BoardError("Index must be integer")

        return (index % self.size[0], int(
            (index - index % self.size[0]) / self.size[1]
        ))

    def is_won(self, player):
        """Determine if the game is won.  Returns false if there is no winner,
        returns true if the given player has won."""
        if type(player) != Player:
            raise BoardError("Player argument must be Player object")

        directions = [
            (1, 0), (0, 1), (1, 1), (-1, 0), (0, -1), (-1, -1), (1, -1), (-1, 1)
        ]
        for index, tile in enumerate(self.tile_list):
            if tile != player.letter:
                continue
            for direction in directions:
                cur_pos = self.index_to_pos(index)
                for adjacent_length in range(1, self.win_length):
                    new_pos = (
                        cur_pos[0] + direction[0], cur_pos[1] + direction[1]
                    )
                    if any(pos < 0 or pos >= self.size[0] or pos >= self.size[1]
                            for pos in new_pos):
                        break
                    elif self.get_tile(new_pos) != player.letter:
                        break
                    elif adjacent_length == self.win_length - 1:
                        return True
                    cur_pos = new_pos
        return False

    def is_over(self):
        """Returns false if the game is not over, returns true if the game is
        over. A tied game is considered over."""
        if not ' ' in self.tile_list:
            return True
        elif any(self.is_won(player) for player in self.players):
            return True
        else:
            return False

    def get_possible_moves(self):
        """Returns a list of boards which contain the possible moves"""
        possible_moves = []
        for index, tile in enumerate(self.tile_list):
            if tile == ' ':
                possible_moves.append(self.index_to_pos(index))

        return possible_moves

    def get_opponents(self, player):
        """Returns a list of opponents to player, regardless of turn"""
        return [p for p in self.players if p != player]

My main concern here is performance, since the ai module will be creating hundreds of thousands of instances of this Board class, and will be calling many of its methods (especially is_won and is_over) hundreds of thousands of times.

Also, I am not sure whether checking the type of every argument and raising an exception if they are not correct is the best decision.

\$\endgroup\$
3
\$\begingroup\$

You do a lot of explicit type-checking; this isn't very Pythonic. For example:

if (type(size) != tuple or len(size) != 2 or type(size[0]) != int or
        type(size[1]) != int or size[0] < 1 or size[1] < 1):
    raise BoardError("""Size needs to be a tuple of two integers that
                        are greater than 0""")

could be much more neatly implemented as:

try:
    width, height = map(int, size)
except ValueError:
    raise BoardError("Couldn't interpret size {!r}.".format(size))

Rather than checking that the argument is exactly what is expected, this checks than it can be used as expected, i.e. the following size arguments would all work: (1, 2), '12', [1, 2], ['1', '2'], ... This is known as "duck typing" and is at the heart of Python's dynamic type system.

Similarly, why does it matter if players is a list specifically, rather than e.g. a tuple? If you are going to check type explicitly, though, note that you should use isinstance rather than type, e.g. if not isinstance(players, (list, tuple)):.


You have a fair bit of duplicated code. To pick on the same example, the type-checking code above appears in Board.__init__, Board.set_tile and Board.get_tile (although not, bizarrely, in Board.move, although that also takes a pos parameter). Therefore, however you decide to implement it, you could abstract it into a convenience method (it doesn't use any class or instance state, so I've made it static):

@staticmethod
def _parse_pos(pos, min_=0):
    ...
    return x, y

This can now easily be called elsewhere:

def get_tile(self, pos):
    """..."""
    x, y = self_parse_pos(pos)
    ...

Parameterising the min_ value lets you use it in Board.__init__, too:

def __init__(self, size, ...):
    """..."""
    width, height = self._parse_pos(size, min_=1)
    ...

Providing docstrings is definitely a good thing. However, you should read the associated PEP; they should have a single-line summary, followed by further details, e.g.:

def index_to_pos(self, index):
    """Take a list index to self.tile_list and convert it to a position
    for use in get_tile and set_tile"""
    ...

could be:

def index_to_pos(self, index):
    """Convert list index to a position (x, y).

    Take an index to tile_list and convert it to a position
    for use in get_tile and set_tile.

    """
    ...

Some of them don't make sense, either; Board.is_won claims that it:

Returns false if there is no winner, returns true if the given player has won.

There are three possible outcomes: no winner; given player won; and another player won. What happens in the third case? Reading the code, it seems like:

Returns whether or not the given player has won the game.

would be more accurate.


In terms of efficiency, note that you create a new list every time you call Board.is_won. Literals like:

directions = [
    (1, 0), (0, 1), (1, 1), (-1, 0), (0, -1), (-1, -1), (1, -1), (-1, 1)
]

appearing in instance code and never getting changed can generally be abstracted out to class attributes, so that they only get created once (at class definition time) rather than every time the method gets called.

class Board(object):
    """..."""

    DIRECTIONS = [
        (-1, -1), (0, -1), (1, -1),
        (-1,  0),          (1,  0),
        (-1,  1), (0,  1), (1,  1)
    ]

    ...

Converting back and forth between index and position also seems inefficient - the internal code should use one representation or the other, converting only if required for the external interface. A list of lists might be a more intuitive structure for development purposes, and works naturally with x, y indices.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ +1 Never thought of arranging a DIRECTIONS literal like that before \$\endgroup\$ – Eric Apr 14 '15 at 17:12

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.