Expandable, object-oriented tic tac toe module

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

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.

StackExchange Code Review Q#86798, answer score: 3