5
\$\begingroup\$

I created a console-based TicTacToe game in Python. So far, everything works as expected. I decided to keep the class static, because I figured there would be no use for multiple game states at once. Is there anything I can improve on?

class TicTacToe:
    """Plays a 2-player TicTacToe Game."""

    __curr_player = False  # simple bool player switch
    __game_matrix = [  # representation of game state
        [0, 0, 0],
        [0, 0, 0],
        [0, 0, 0],
    ]

    @classmethod
    def play(cls):
        """Play a single round."""
        while not cls.__game_finished():

            # set current player
            player = 2 if cls.__curr_player else 1

            # get player move
            while 1:
                try:
                    print(f"Player {player}'s turn.")
                    x = int(input("X: "))
                    y = int(input("Y: "))
                    if cls.__is_valid_field(x, y):
                        break
                    else:
                        print("Invalid field.")
                except ValueError:
                    print("Invalid Input.")

            cls.__update(x, y, player)
            print("Current game state: ")
            cls.__display()

            # change player
            cls.__curr_player = not cls.__curr_player

        print("Game finished: ")
        cls.__display()

        # reset state
        cls.__reset()

    @classmethod
    def __game_finished(cls):
        """Returns True if one player has won or all fields are filled."""
        # all filled?
        if all([value for line in cls.__game_matrix for value in line]):
            return True
        # horizontal win?
        if any(all([x == line[0] and x for x in line]) for line in cls.__game_matrix):
            return True
        # vertical win?
        if any(all([line[column] == cls.__game_matrix[0][column] and line[column] for line in cls.__game_matrix]) for column in range(3)):
            return True
        # diagonal win?
        if (cls.__game_matrix[0][0] == cls.__game_matrix[1][1] == cls.__game_matrix[2][2] != 0) or \
           (cls.__game_matrix[0][2] == cls.__game_matrix[1][1] == cls.__game_matrix[2][0] != 0):
            return True
        return False

    @classmethod
    def __is_valid_field(cls, x, y):
        """Checks if field is already filled or out of range."""
        if x > 3 or x < 1 or y > 3 or y < 1:
            return False
        return not cls.__game_matrix[y - 1][x - 1]

    @classmethod
    def __update(cls, x, y, player):
        """Update game state."""
        cls.__game_matrix[y - 1][x - 1] = player

    @classmethod
    def __display(cls):
        """Displays current game state."""
        for line in cls.__game_matrix:
            print(line)

    @classmethod
    def __reset(cls):
        """Reset game state."""
        cls.__curr_player = False
        cls.__game_matrix = [
            [0, 0, 0],
            [0, 0, 0],
            [0, 0, 0],
        ]


def main():
    TicTacToe.play()


if __name__ == "__main__":
    main()
\$\endgroup\$
3
+50
\$\begingroup\$
  • Consider representing the matrix not as integers, but as references to players
  • Consider representing players as an Enum, and putting some string utilities such as symbol text in there
  • The standard way of showing this game's grid is as "X" and "O" characters
  • Drop the reset method, and certainly drop the mandatory call to reset at the end of play. If you really need a fresh game, make a new instance.
  • Convert this from a class of static methods - which might as well be a module with global functions - into an actual class whose instance is meaningful
  • Do not use double-underscores the way you are; this is reserved for name mangling - and anyway, there's not much of a point in attempting to make private members in Python. If you insist on private members, use a single underscore.
  • You're missing logic to show a stalemate.
  • Consider pre-calculating a sequence of line coordinates to simplify your end-condition check code.

Also, this is somewhat dangerous:

cls.__game_matrix[1][1] == cls.__game_matrix[2][2] != 0

It does do what you intended, which is an implied

cls.__game_matrix[1][1] == cls.__game_matrix[2][2] and
cls.__game_matrix[2][2] != 0

However, given that False == 0 in Python, this risks being difficult-to-interpret by programmers that might fear that this evaluates to

(cls.__game_matrix[1][1] == cls.__game_matrix[2][2]) is not False

even though it doesn't. Better to be explicit.

Suggested

from dataclasses import dataclass
from enum import Enum
from typing import Optional, Tuple, List

N = 3


class Player(Enum):
    ONE = False
    TWO = True

    def __str__(self):
        return 'two' if self.value else 'one'

    @staticmethod
    def symbol(player: Optional['Player']) -> str:
        if player is None:
            return ' '
        return 'X' if player.value else 'O'

    @property
    def other(self) -> 'Player':
        return Player(not self.value)


@dataclass
class Coord:
    x: int
    y: int

    @property
    def is_valid(self) -> bool:
        return 0 <= self.x < N and 0 <= self.y < N

    @classmethod
    def from_stdin(cls) -> 'Coord':
        while True:
            try:
                coord = cls(int(input('X: ')) - 1, int(input('Y: ')) - 1)
                if coord.is_valid:
                    return coord
            except ValueError:
                pass

            print('Invalid input.')


class TicTacToe:
    """Plays a 2-player TicTacToe Game."""

    def __init__(self):
        self.curr_player = Player.ONE
        self.matrix: List[List[Optional[Player]]] = [
            [None]*N for _ in range(N)
        ]
        self.line_coords = self.make_lines()

    @staticmethod
    def make_lines() -> Tuple[List[Coord]]:
        return (
            *(  # Horizontal
                [Coord(x, y) for x in range(N)]
                for y in range(N)
            ),
            *(  # Vertical
                [Coord(x, y) for y in range(N)]
                for x in range(N)
            ),
            # First diag
            [Coord(y, y) for y in range(N)],
            # Second diag
            [Coord(x, N - x - 1) for x in range(N)],
        )

    def play(self):
        """Play a single round."""
        while True:
            print(f"Player {self.curr_player}'s turn.")

            # get player move
            while True:
                coord = Coord.from_stdin()
                if self.matrix[coord.y][coord.x] is None:
                    break
                print('That cell is already taken.')

            self.update(coord)
            print('Current game state:')
            self.display()

            is_finished, winner = self.get_winner()
            if is_finished:
                break

            # change player
            self.curr_player = self.curr_player.other

        winner_text = 'stalemate' if winner is None else f'player {winner} won'
        print(f'Game finished; {winner_text}:')
        self.display()

    def get_winner(self) -> Tuple[bool, Optional[Player]]:
        """Returns whether the game is done, and the winner."""

        for line in self.line_coords:
            first = self.matrix[line[0].y][line[0].x]
            if first is not None:
                others = {
                    self.matrix[coord.y][coord.x] for coord in line[1:]
                }
                if others == {first}:
                    return True, first

        return all(
            all(cell is not None for cell in line) for line in self.matrix
        ), None

    def update(self, coord: Coord):
        """Update game state."""
        self.matrix[coord.y][coord.x] = self.curr_player

    def display(self):
        """Displays current game state."""
        for line in self.matrix:
            print(' '.join(Player.symbol(cell) for cell in line))


def main():
    TicTacToe().play()


if __name__ == "__main__":
    main()
\$\endgroup\$
3
\$\begingroup\$
  1. When i started the game, i found it pretty confusing as to what what coordinates should we input. I added an instructions method:
    @classmethod
    def __instructions(cls):
        print(
            'Welcome to TicTacToe\n'
            'Here is the board:\n'
            )
        cls.__display()
        print(
            'The coordinates start at top left\n'
            '(1, 1) (2, 1) (3, 1)\n'
            '(1, 2) (2, 2) (3, 2)\n'
            '(1, 3) (2, 3)  (3, 3)\n'
            )
  1. I found having 0, 1, 2 on the board is pretty confusing. I replaced empty cells with a symbol, in this case -
    @classmethod
    def __display(cls):
        """Displays current game state."""
        for line in cls.__game_matrix:
            symbols = []
            for elem in line:
                if elem == 0:
                    symbols.append('-')
                else:
                    symbols.append(str(elem))
            print(' '.join(symbols))
  1. I rearraged private methods before

  2. I added a win annoncement to know who won:

print("Game finished: Player {} won!".format(player))

  1. If you used a normal class you would not have needed
@classmethod
def __game_finished(cls):

but would have used the normal

def __game_finished(self):

less writing


Here is my code:

class TicTacToe:
    """Plays a 2-player TicTacToe Game."""

    __curr_player = False  # simple bool player switch
    __game_matrix = [  # representation of game state
        [0, 0, 0],
        [0, 0, 0],
        [0, 0, 0],
    ]

    @classmethod
    def __game_finished(cls):
        """Returns True if one player has won or all fields are filled."""
        # all filled?
        if all([value for line in cls.__game_matrix for value in line]):
            return True
        # horizontal win?
        if any(all([x == line[0] and x for x in line]) for line in cls.__game_matrix):
            return True
        # vertical win?
        if any(all([line[column] == cls.__game_matrix[0][column] and line[column] for line in cls.__game_matrix]) for column in range(3)):
            return True
        # diagonal win?
        if (cls.__game_matrix[0][0] == cls.__game_matrix[1][1] == cls.__game_matrix[2][2] != 0) or \
           (cls.__game_matrix[0][2] == cls.__game_matrix[1][1] == cls.__game_matrix[2][0] != 0):
            return True
        return False

    @classmethod
    def __is_valid_field(cls, x, y):
        """Checks if field is already filled or out of range."""
        if x > 3 or x < 1 or y > 3 or y < 1:
            return False
        return not cls.__game_matrix[y - 1][x - 1]

    @classmethod
    def __update(cls, x, y, player):
        """Update game state."""
        cls.__game_matrix[y - 1][x - 1] = player

    @classmethod
    def __display(cls):
        """Displays current game state."""
        for line in cls.__game_matrix:
            symbols = []
            for elem in line:
                if elem == 0:
                    symbols.append('-')
                else:
                    symbols.append(str(elem))
            print(' '.join(symbols))

    @classmethod
    def __reset(cls):
        """Reset game state."""
        cls.__curr_player = False
        cls.__game_matrix = [
            ['-', '-', '-'],
            ['-', '-', '-'],
            ['-', '-', '-'],
        ]

    @classmethod
    def __instructions(cls):
        print(
            'Welcome to TicTacToe\n'
            'Here is the board:\n'
            )
        cls.__display()
        print(
            'The coordinates start at top left\n'
            '(1, 1) (2, 1) (3, 1)\n'
            '(1, 2) (2, 2) (3, 2)\n'
            '(1, 3) (2, 3)  (3, 3)\n'
            )

    @classmethod
    def play(cls):
        cls.__instructions()
        """Play a single round."""
        while not cls.__game_finished():

            # set current player
            player = 2 if cls.__curr_player else 1

            # get player move
            while 1:
                try:
                    print(f"Player {player}'s turn.")
                    x = int(input("X: "))
                    y = int(input("Y: "))
                    if cls.__is_valid_field(x, y):
                        break
                    else:
                        print("Invalid field.")
                except ValueError:
                    print("Invalid Input.")

            cls.__update(x, y, player)
            print("Current game state: ")
            cls.__display()

            # change player
            cls.__curr_player = not cls.__curr_player

        print("Game finished: Player {} won!".format(player))
        cls.__display()

        # reset state
        cls.__reset()

    


def main():
    TicTacToe.play()


if __name__ == "__main__":
    main()

The code was well commented, broken down, easy to follow and clean!

\$\endgroup\$
5
  • \$\begingroup\$ What do you mean by a "normal class"? \$\endgroup\$ – Grajdeanu Alex Dec 18 '20 at 14:39
  • \$\begingroup\$ @GrajdeanuAlex. I think he is talking about a "non-all-static" class. \$\endgroup\$ – Tom Gebel Dec 18 '20 at 15:23
  • \$\begingroup\$ Yes a normal tictactoe = TicTacToe() \$\endgroup\$ – Abdur-Rahmaan Janhangeer Dec 18 '20 at 16:28
  • 1
    \$\begingroup\$ @Abdur-RahmaanJanhangeer The word you're looking for is "non-static", not "normal" \$\endgroup\$ – Parekh Dec 21 '20 at 7:29
  • 1
    \$\begingroup\$ Indeed; you should follow your own recommendation and convert all of those class methods to instance methods in your suggested code. \$\endgroup\$ – Reinderien Dec 23 '20 at 17:29

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.