8
\$\begingroup\$

I'm starting to improve my python skills by learning good code practices, as commenting the code, encapsulating object attributes, using typings, and so on... but I'm a newbie on all of this, as you will see on my code below

To do this, I'm developing my test subject, as a basic game board engine, that has to have the ability to be extended to multiple board games, and types of players.

So, for example, I start it with a classic TicTacToe game with a human player. And I'm asking for recommendations on how to improve the code from any point of view to do it

thanks in advance

here is the code:

games.py

import itertools
from typing import Sequence, Dict, Tuple, List

from players import Player


class Board:
    """Basic class for some classic boards games"""

    def __init__(self, rows: int, cols: int) -> None:
        """Init the game board

        :param rows: Amount of rows for the board
        :type rows: int

        :param cols: Amount of columns for the board
        :type cols: int
        """
        # Generate board as list of list with size equal to rows x cols
        self._board: List[List[str]]
        self._board = [['' for _ in range(cols)] for _ in range(rows)]

    def __str__(self) -> str:
        """Board representations

        :rtype: str
        """

        str_board: List[str] = ["    " + "   ".join([str(i) for i, _ in enumerate(self._board[0])])]
        str_line: str = '  ' + '-' * (len(self._board) * 4 + 1)

        for row_n, row in enumerate(self._board):
            str_row = " | ".join([str(val if val else ' ') for val in row])
            str_board.append(str_line)
            str_board.append(f"{row_n} | {str_row} | ")

        str_board.append(str_line)
        return '\n'.join(str_board)

    def __gettitem__(self, cell: Sequence) -> str:

        return self.board_get_position(cell)

    def board_empty_cells(self) -> list:
        """Get the empty cells in the board

        :return: Empty cells in the board as list of (row,col)
        :rtype: list
        """
        cells: List[Tuple[int, int]]
        cells = [(i, j) for i, row in enumerate(self._board)
                 for j, col in enumerate(row)
                 if not self._board[i][j]]

        return cells

    def board_rows(self) -> List[List[str]]:
        """Get the rows of the board as list of lists

        :return: The rows of the board as list of lists
        :rtype: list
        """

        return self._board

    def board_cols(self) -> List[List[str]]:
        """Get the columns of the board

        :return: The columns of the board as list of lists
        :rtype: List[List[str]]
        """
        cols: List[List[str]]
        cols = [list(col) for col in zip(*self._board)]
        return cols

    def board_diags(self) -> List[List[str]]:
        """Get the diagonals of the board

        :return: The diagonals of the board as list of lists
        :rtype: List[List[str]]

        :raise NotImplementedError: For boards with rows != cols
        """

        if len(self._board) == len(self._board[0]):
            d1 = [row[i] for i, row in enumerate(self._board)]
            d2 = [row[~i] for i, row in enumerate(self._board)]
            return [d1, d2]
        else:
            raise NotImplementedError()

    def board_set_position(self, cell: Sequence, value: str, replace: bool = True) -> bool:
        """Set board cell value

        :param cell: The cell in the board as (row,col)
        :type cell: Sequence

        :param value: The value to put in cell
        :type value: str

        :param replace: (True|False) to replace the cell value if the cell is not empty
        :type replace: bool

        :return: True if the cell was modified or False if not
        :rtype: bool
        """

        row, col = cell
        if replace or not self._board[row][col]:
            self._board[row][col] = str(value)
            return True
        else:
            return False

    def board_get_position(self, cell: Sequence) -> str:
        """Get board value in cell

        :param cell: Iterable of size 2 that represent the board cell position as (row,col)
        :type cell: Sequence

        :return: The value of the board on the cell
        :rtype: str
        """

        if len(cell) == 2:
            row, col = cell
            return self._board[row][col]
        else:
            raise ValueError('Cell value is defined as (row,col)')


class Game(Board):
    """Basic class for board games"""

    def __init__(self, name: str, board_size: tuple) -> None:
        """

        :param name: The Name of the Game
        :type name: str

        :param board_size: The size of the board as (row,cols)
        :type board_size: tuple

        :raise ValueError if board_size len != 2
        """

        if len(board_size) == 2:
            super().__init__(*board_size)
        else:
            raise ValueError('Board size have to be defined as (rows, cols)')
        self._name = name
        self._is_over = False
        self._game_status: Dict = {'history': [],
                                   'winner': None,
                                   'status': 'running'}

    @property
    def name(self) -> str:
        """

        :return: Game name
        :rtype: str
        """
        return self._name

    @name.setter
    def name(self, _name: str) -> None:
        """
        Set Game name

        :param _name: The game name
        :type _name: str
        """
        self._name = _name

    def game_status(self) -> dict:
        """

        :return: The game status
        :rtype: dict
        """
        return self._game_status

    def is_over(self) -> bool:
        """

        :return: (True|False) if the game is over or not
        :rtype: bool
        """
        return self._is_over

    def move(self, *args) -> None:
        """

        :raise ValueError: If the game if over, of if an invalid move was played
        """
        if self._is_over:
            raise ValueError("The game is over")


class TicTacToe(Game):
    """Tic Tac Toe Game"""

    BOARD_SIZE: Tuple[int, int] = (3, 3)

    def __init__(self, _name='Tic Tac Toe') -> None:
        super().__init__(name=_name, board_size=self.BOARD_SIZE)
        self._players = ('X', 'O')
        self._game_sequence = itertools.cycle(self._players)
        self._current_player = next(self._game_sequence)
        self._game_status.update({'current_player': self._current_player})

        # encode the board cell positions by cell name as: {'cell name': board(row,col)}
        self._encoded_cells: Dict[str, Tuple[int, int]]
        self._encoded_cells = {str(i): cell for i, cell in
                               enumerate(itertools.product(range(self.BOARD_SIZE[0]),
                                                           range(self.BOARD_SIZE[1])))}

    def __str__(self):

        header = (' ===============\n'
                  f' | {self._name} |\n'
                  ' ===============\n')
        str_board = []
        str_line = '  ' + '-' * (len(self._board) * 4 + 1)

        # reverse de dict of encoded cell to decode
        decoded_cells = dict(zip(self._encoded_cells.values(), self._encoded_cells.keys()))

        for row_n, row in enumerate(self._board):
            str_row = '  '
            for col_n, cell in enumerate(row):
                val = cell if cell else decoded_cells[(row_n, col_n)]
                str_row += f"| {val} "
            str_row += "|"
            str_board.append(str_line)
            str_board.append(str_row)

        str_board.append(str_line)

        return header + '\n'.join(str_board)

    def game_over(self) -> bool:
        """Check for game over and update game status

        :return: (True|False) if the game is over or not
        :rtype: bool
        """

        def winner_check(to_check: list) -> bool:
            """Check if the player repeat 3 times in the current list

            :param to_check: Player
            :type to_check: list

            :return: (True|False) if the Player repeats 3 times
            :rtype: bool
            """
            winner = False
            for row in to_check:
                if row.count(self._current_player) == 3:
                    winner = True
            return winner

        has_winner = winner_check(self.board_cols()) \
                     or winner_check(self.board_rows()) \
                     or winner_check(self.board_diags())

        # update game status
        if has_winner:
            if self._current_player == 'X':
                self._game_status['winner'] = 0
            else:
                self._game_status['winner'] = 1

            self._game_status['status'] = f'Game over {self._current_player} win'
            self._game_status.pop('current_player', None)
            self._is_over = True

        if not self.board_empty_cells():
            self._game_status['status'] = 'draw'
            self._game_status.pop('current_player', None)
            self._is_over = True

        return self._is_over

    def move(self, cell: str) -> None:
        """Make a move

        :param cell: The name of the cell in the board to play
        :type cell: int

        :raise ValueError: If the game if over, of if an invalid move was played
        """
        if self._is_over:
            raise ValueError("The game is over")

        board_position = self._encoded_cells[cell]
        move_ok = self.board_set_position(cell=board_position, value=self._current_player, replace=False)

        if move_ok:
            self._game_status['history'].append((self._current_player, cell))
            if not self.game_over():
                self._current_player = next(self._game_sequence)
        else:
            raise ValueError(
                f"Invalid move, cell: [{cell}] was played by '{self.board_get_position(self._encoded_cells[cell])}'")


class PlayGame:
    """Base class for playing two player games"""

    def __init__(self, player1: Player, player2: Player, game: Game) -> None:
        """

        :param player1: First player
        :type player1: Player

        :param player2: Second player
        :type player2: Player

        :param game: Game to play
        :type game: Game
        """
        self._game = game
        player1.player = 0
        player2.player = 1
        self._players = (player1, player2)
        self._play_sequence = itertools.cycle(self._players)
        self._current_player: Player = next(self._play_sequence)

    def run(self) -> None:
        """Start playing until the games is over"""

        while not self._game.is_over():

            print(self._game, end='\n')
            print(f'Player: {self._current_player}')
            move = self._current_player.play()

            try:
                self._game.move(*move)
            except ValueError as err:
                print(err)
            else:
                status = self._game.game_status()
                self._current_player.update_game_status(status)
                self._current_player = next(self._play_sequence)
        else:
            status = self._game.game_status()
            winner = self._players[status['winner']]
            print(f'Game over: {winner.name} wins')
            print(self._game, end='\n')

players.py

from typing import Dict


class Player:
    """Basic class for game players"""

    def __init__(self, name: str) -> None:
        self._name = name

    @property
    def name(self) -> str:
        """

        :return: Player name
        :rtype: str
        """
        return self._name

    @name.setter
    def name(self, _name: str) -> None:
        """
        Set Player name

        :param _name: The player name
        :type _name: str
        """
        self._name = _name


class TicTacToeHumanPlayer(Player):
    """Human player for Tic Tac Toe Game"""

    BOARD_SIZE = (3, 3)

    def __init__(self, name: str) -> None:
        super().__init__(name)
        self._player: int = 0
        self._game_status: Dict = {}

    def __str__(self) -> str:
        return f"{self._name} with {'O' if self._player else 'X'}"

    @property
    def player(self) -> int:
        """

        :return: Player turn in the game: 0 for First Player or 1 for Second player
        :rtype: int
        """
        return self._player

    @player.setter
    def player(self, _player: int) -> None:
        """

        :param _player: Set the Player turn in the game
        :type _player: int
        """
        self._player = _player

    def update_game_status(self, status: Dict) -> dict:
        """Update the game status for the player

        :param status: Game status
        :type status: dict

        :return: The game status
        :rtype: dict
        """
        self._game_status.update(status)
        return self._game_status

    @staticmethod
    def play() -> str:
        """Ask to the player where to play

        :return: the move
        :rtype; str
        """
        cell = None
        while not cell:
            cell = input('In which cell you want to play: ')

        return cell

run.py

from games import PlayGame, TicTacToe
from players import TicTacToeHumanPlayer

if __name__ == "__main__":
    ticatac = TicTacToe()
    player1 = TicTacToeHumanPlayer(name='Eduardo')
    player2 = TicTacToeHumanPlayer(name='Firvida')

    game = PlayGame(player1, player2, ticatac)
    game.run()

\$\endgroup\$
  • \$\begingroup\$ Minor quibble: it should be "Number of rows/columns", not "Amount of rows/columns". \$\endgroup\$ – Ray Butterworth Dec 21 '19 at 3:39
9
\$\begingroup\$

I think your Player class is more complicated than it needs to be. Your name property getter and setter aren't doing anything special, so there isn't really any difference between what you have and having _name as a public attribute.

Having a public attribute is only a bad thing if you need to maintain some delicate internal state that you don't want your user to mess with. For example, what if a list's size attribute was public? The user could break the list object horribly by stupidly changing the size attribute.

You aren't dealing with any kind of special state that needs to be encapsulated here though. What if the user forcibly changes the name field? Does that break anything?

Your class then can just be distilled down to just a basic structure with a public attribute. Note too that __init__ doesn't need to have its return type specified. It must return None, so -> None is implied.

class Player:
    """Basic class for game players"""

    def __init__(self, name: str):
        self.name = name

This though is exactly the case for a dataclass: when you have just a simple class holding public data.

from dataclasses import dataclass

@dataclass
class Player:
    """Basic class for game players"""
    name: str

This is purely a convenience and readability helper. dataclass creates a constructor for you based on the fields that you specify. It also creates a readable __repr__ implementation, and a few other things.

Some classes hold a potentially delicate internal state (like a data structure that holds a size field among others that must be altered carefully), and some represent just plain data. Your Player class seems to fall into the latter category. I actually saw a good article on this like an hour ago coincidentally. Don't bother with trying to go to elaborate lengths to protect data that has no need to be protected from change.

With this last example though, I have to wonder if there's a need for Player at all. What's the difference between a player and the string that represents the player's name? I'd keep Player if there were any chance that you'd add another field of data in the future (score?), but if not, I'd just use a bare string.

And then a lot of the same can be said about Game. Having getters and setters for name is a waste. Just expose it. Having a getter for _game_status does make sense though. Having only a getter and making the field "private" indicates that it shouldn't be touched externally.


Why is Game a Board? I don't think a game of TicTacToe is a board; a game has a board. This seems to be a "violation" of composition over inheritance. I think Game should hold a Board object and manipulate it.

Along the same lines, I don't think you should have a Player class with a TicTacToeHumanPlayer class inheriting from it like you have. I don't see any advantage to having your current Player as a base class that others classes (like presumably a AI class in the future) inherit from. I would collapse TicTacToeHumanPlayer into Player and just make it one class.

To allow the class to make move decisions in varied ways (a human player, and different difficulties of AIs for example), you could use the Strategy Pattern (or something that slightly resembles it). Give the Player class a _move_maker field. Something like:

from typing import Callable

class Player:  # Ignore my dataclass suggestion for the time being

    def __init__(move_making_func: Callable[[Board], str], . . .):
        self.move_maker: Callable[[Board], str] = move_maker_func

move_maker is a function that accepts as an argument the board, and returns the move to make. For a human player, this would involve asking the player for input, doing some error checking (which you're currently missing) and returning the move. For an AI, it would involve it figuring out a move, then returning that.

To use it, change the play used in PlayGame to something like

move = current_player.move_maker(self.board)

This is just a general suggestion; the simple example I showed here won't necessarily work perfectly. The point is, instead of creating different subclasses to produce slightly different behavior, the class can just use a decision making function to change how exactly the player responds. If you switch to using a full UI, you can just create a new decision function that handles the UI. You'll likely need to make minimal changes to Player (except maybe allowing for async responses).

If you wanted separate classes though, you could have an abstract base class that contains a make_move method that inheritors must implement. Then you could have a separate class for each player type, and a central way of using them.


As I hinted at above, your play function is doing very little in the way of error checking of the user input. I admit, I don't fully understand what your "encoded" system is to indicate cells, but surely not every possible user input is valid. Currently you're only checking if the input is empty.

I'd make it something closer to:

@staticmethod
def play() -> str:
    """Ask to the player where to play

    :return: the move
    :rtype; str
    """
    while True:
        cell = input('In which cell you want to play: ')

        if cell_is_valid(cell):  # Define this function somewhere to do more proper checking
            return cell

        else:
            print(f"Cell {cell} isn't valid.")

I think a while True reads better in this case. It more cleanly allows for an error message, and does away with needing to pre-define cell with a dummy value.


winner = False
for row in to_check:
    if row.count(self._current_player) == 3:
        winner = True
return winner

This can be written more succinctly (and more performantly) as

return any(row.count(self._current_player) == 3 for row in to_check)

This is a generator expression inside of a call to any. It will return (and stop searching) once a True match is found.


You write List[List[str]] a few times. You can easily make aliases of type names like this:

Rows =  List[List[str]]

. . .

def board_rows(self) -> Rows:
    . . .

I think that's a lot less noisy. It also makes it easier to adapt if you ever want to change what the underlying board is represented by.


I think your _game_status dictionary would be another good candidate for a dataclass:

from typing import Optional, List

@dataclass
class GameStatus:
    history: List[?] = []  # Replace the ? with the proper type of the history
    winner: Optional[int] = None  # Indicates that it can be None, or an int
    status: str = "running"  # Arguably this should be an Enum instead of a string
    current_player: Optional[str] = "X"

. . .

_game_status: Dict = GameStatus()  # Uses the default values set above

. . .

self._game_status.winner = 0

. . .

# self._game_status.update({'current_player': self._current_player})
self._game_status.current_player = self._current_player

This clearly states what a game state contains, and makes it harder to cause typos using strings.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks for your full review I will fix all the thinks and put it again. Thanks again and happy new year \$\endgroup\$ – efirvida Jan 7 at 18:40

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.