4
\$\begingroup\$

I implemented an object-oriented version of a Tic-Tac-Toe game in Python 3.

I'd like to have feedback on general code style and possible optimizations.

#! /usr/bin/env python3
#
#    tictactoe - A simple tic-tac-toe game
#
#    (C) 2017 - Richard Neumann <mail at richard dash neumann dot de>
#
#    This program is free software: you can redistribute it and/or modify
#    it under the terms of the GNU General Public License as published by
#    the Free Software Foundation, either version 3 of the License, or
#    (at your option) any later version.
#
#    This program is distributed in the hope that it will be useful,
#    but WITHOUT ANY WARRANTY; without even the implied warranty of
#    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
#    GNU General Public License for more details.
#
#    You should have received a copy of the GNU General Public License
#    along with this program.  If not, see <http://www.gnu.org/licenses/>.
#
"""A simple Tic-Tac-Toe game"""

from itertools import chain


class InvalidInput(Exception):
    """Indicates an invalid user input"""

    def __init__(self, message):
        """Sets the message to the user"""
        super().__init__(message)
        self.message = message


class FieldIsOccupied(Exception):
    """Indicates that the selected field is occupied"""

    pass


class GameEnd(Exception):
    """Indicates the end of the game"""

    def __init__(self, winner):
        """Sets the respective winner"""
        super().__init__(winner)
        self.winner = winner


class Player():
    """Player and field values"""

    CROSS = 'x'
    CIRCLE = 'o'
    NONE = ' '


class TicTacToe():
    """The game class"""

    def __init__(self):
        """Initializes field and last player"""
        self.field = [[Player.NONE] * 3, [Player.NONE] * 3, [Player.NONE] * 3]
        self.last_player = Player.NONE

    def __str__(self):
        """Converts the game field into a string"""
        return '{}║{}║{}\n═╬═╬═\n{}║{}║{}\n═╬═╬═\n{}║{}║{}'.format(
            *chain(*self.field))

    @property
    def win_patterns(self):
        """Yields patterns significant for winning"""
        # Rows
        yield self.field[0]
        yield self.field[1]
        yield self.field[2]
        # Columns
        yield (self.field[0][0], self.field[1][0], self.field[2][0])
        yield (self.field[0][1], self.field[1][1], self.field[2][1])
        yield (self.field[0][2], self.field[1][2], self.field[2][2])
        # Diagonals
        yield (self.field[0][0], self.field[1][1], self.field[2][2])
        yield (self.field[0][2], self.field[1][1], self.field[2][0])

    @property
    def next_player(self):
        """Returns the next player"""
        if self.last_player is Player.CROSS:
            return Player.CIRCLE

        return Player.CROSS

    def check_winner(self):
        """Check if the game has ended"""
        draw = True

        for fields in self.win_patterns:
            if fields[0] in (Player.CROSS, Player.CIRCLE):
                if all(fields[0] == field for field in fields[1:]):
                    raise GameEnd(fields[0])
            elif any(field is Player.NONE for field in fields):
                draw = False

        if draw:
            raise GameEnd(Player.NONE)

    def make_turn(self, player, column, row):
        """Makes a turn"""
        if self.field[row][column] is Player.NONE:
            self.last_player = self.field[row][column] = player
        else:
            raise FieldIsOccupied()

    @property
    def player_input(self):
        """Reads input from player"""
        coords = input('Turn for {}: '.format(self.next_player))

        try:
            column, row = coords.split()
        except ValueError:
            raise InvalidInput('Please enter: "<column> <row>"')
        else:
            try:
                column, row = int(column), int(row)
            except ValueError:
                raise InvalidInput('Coordinates must be integers.')
            else:
                if all(0 <= i <= 2 for i in (column, row)):
                    return column, row

                raise InvalidInput('Coordinates must be 0 <= i <=2.')

    def start(self):
        """Starts the game"""
        while True:
            print(self)

            try:
                self.check_winner()
            except GameEnd as game_end:
                if game_end.winner is Player.CROSS:
                    print('Cross wins.')
                elif game_end.winner is Player.CIRCLE:
                    print('Circle wins.')
                else:
                    print('The game ended in a draw.')

                break

            try:
                column, row = self.player_input
            except KeyboardInterrupt:
                print('\nGame aborted.')
                return False
            except InvalidInput as invalid_input:
                print(invalid_input.message)
            else:
                try:
                    self.make_turn(self.next_player, column, row)
                except FieldIsOccupied:
                    print('The selected field is occupied.')

        try:
            input('Press enter to exit...')
        except KeyboardInterrupt:
            print()


def main():
    """Runs a new game"""

    game = TicTacToe()
    game.start()


if __name__ == '__main__':
    main()
\$\endgroup\$
3
\$\begingroup\$

Great job there - easy to understand class with a nice separation logic around the methods - really like that you've used __str__ to have a string representation of the game board.

Some minor improvements:

  • you can remove extra () from the class definitions:

    class TicTacToe:
    
  • end your docstrings with a dot (see PEP 257 -- Docstring Conventions)

  • you can use the short if/else form in your next_player method:

    return Player.CIRCLE if self.last_player is Player.CROSS else Player.CROSS
    
  • Player can be an Enum class. And, think about a better name for it - it feels more like a Field than Player since you enumerate possible field values there

Note that it is not straightforward to scale the board to a bigger size - not sure how big of a problem this is, but think about adding more generalizations to the class.

\$\endgroup\$
  • \$\begingroup\$ The class Player was actually named FieldValue before I changed it. :D Good point with the if/else shorthand. I will definitely change this. \$\endgroup\$ – Richard Neumann Aug 23 '17 at 13:53
  • \$\begingroup\$ With your proposed one-line if/else I'd exceed the maximum line length of 79 characters. Since breaking up the statement into multiple lines using paratheses or `\` would void its purpose, I'll keep it as it is for now. \$\endgroup\$ – Richard Neumann Aug 23 '17 at 14:07
  • 2
    \$\begingroup\$ You could use booleans for the players, and just use return not last_player to switch turns. \$\endgroup\$ – Solomon Ucko Aug 23 '17 at 14:35
  • 2
    \$\begingroup\$ Also I am not sure whether their usage of message in InvalidInput is just a habit from Python 2 or it was intentional. message attribute was removed in Python 3 and str(exc) is the recommended way. \$\endgroup\$ – Ashwini Chaudhary Aug 23 '17 at 18:03
3
\$\begingroup\$

it's looking very good! I had a few things to talk about.

Don't use is for string comparison, use ==. If the test is for checking equality, using is will give inconsistent results.

See this question https://stackoverflow.com/questions/1504717/why-does-comparing-strings-in-python-using-either-or-is-sometimes-produce

Exception Handling, I think overall the exception handling is very good, but I think you may have gone a little overboard with regards to your GameEnd Exception class. I like FieldIsOccupied Exception, though. An exception is supposed to be raised when an Exceptional thing happens in your program, I would say that someone trying to fill an already filled slot is that. But the end of the game is very much a core part of the flow of your game!

To me, something like this makes more sense.

if self.has_winner():
    winner = self.winner
    ...
else:
    ...

Using exceptions as control flow is considered an anit-pattern.

On top of this, I would also say that a ValueError could be used in place of an InvalidInput

\$\endgroup\$

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.