Playing a game of ConHex in Python

Question

Background

ConHex is game by Michail Antonow where two players try to create a connected chain of cells from one side of the board to the other side. They do this by claiming one empty position each turn. If a player first possesses at least half of the positions of a cell, the cell belongs to that player. Cells can't be conquered/taken once owned. More info on the game on Boardgame Geek and on Little Golem.

Goal

I programmed a class in Python (version 3.9) that provides an api to play a game of ConHex. The code consists of two Python modules: one with the actual class and code and one with constants. For illustration purposes, I added a main which randomly plays a game until one player wins. You may ignore the main() code for the review.

I'm an amateur Python programmer and seek to learn if (and how) to improve this code. I plan to add a GUI, publish the game on GitHub and train an Alpha Zero deep learning network to play ConHex.

Review questions

• General review: please let me know if there are any points on which I can improve the working of the code and the code readability.
• Specifically review the way I'm using the Python logger. Is this a Pythonic and "good" way to use logging (e.g.: logger initialising, storing a logger object in a class, points where I call the logger, etc.)?
• I put all constants and general definitions in a separate constants.py package. Is this a common pattern, or would you recommend organising the code in another way?
• Optically, the code seems a bit messy. I tried to adhere to PEP8 and the code passes Flake8. Any suggestions to improve code readability?
• Would you consider the class sufficiently documented? Any places where documentation (or code comments) should be added or removed?
• Algorithmically, I'm not very happy with the game_won() method. Any suggestions to improve approach, performance and code readability? Specifically keeping in mind that this game will be played hundreds of million times by the Alpha Zero deep learning algorithm, any performance gain will be noticeable...

ConHex board with coordinate system

Illustration of the coordinate system used for points (white circles, coordinates top + left coordinate, e.g.: C2) and for the cells (polygons, right + bottom coordinates, e.g.: 5, 5 is the middle tilted square area):

The file conhex_board.py

import constants as ct
import logging


class Conhex_game:
    """Representation of a Conhex Game and its state during a game
    """

    def __init__(self) -> None:
        """Initializes an empty Conhex board

        Returns: None
        """
        self.logger = logging.getLogger(ct.LOGGER)
        self.logger.info(f'Started logger for {self.__class__.__name__}')
        self.current_player = ct.BoardPosValue.PLAYER1
        self._board = {pos: ct.BoardPosValue.EMPTY
                       for pos in ct.POSITIONS}
        self.cells_conquered = {
            ct.BoardPosValue.PLAYER1: set(),
            ct.BoardPosValue.PLAYER2: set(),
            ct.BoardPosValue.EMPTY: set(ct.CELLS)
        }
        self.winner = ct.BoardPosValue.EMPTY
        self.moves = []
        self.player_names = dict(ct.DEFAULT_PLAYER_NAMES)

    def set_player_names(self, player1_name: str, player2_name: str) -> None:
        self.player_names[ct.BoardPosValue.PLAYER1] = player1_name
        self.player_names[ct.BoardPosValue.PLAYER2] = player2_name

    def next_player(self) -> ct.enum.Enum:
        """Makes the next player the current player

        Returns: the new current player
        """
        if self.current_player == ct.BoardPosValue.PLAYER1:
            self.current_player = ct.BoardPosValue.PLAYER2
        else:
            self.current_player = ct.BoardPosValue.PLAYER1

        self.logger.debug(f'Switched player: {self.current_player=}')
        return self.current_player

    def play_move(self, position: str) -> bool:
        """Play the given move on the board

        Args:
            position (str): position - capital letter + numer
                            MUST be one of ct.POSITIONS
                            the position on the board MUST be empty

        Returns:
            bool: True if the game is won; False if it isn't

        Raises:
            ValueError: if position is not one of ct.POSITIONS
            ValueError: if a move is placed at an empty spot
        """
        self.logger.debug(f'Playing move: {position=}')
        if position not in ct.POSITIONS:
            raise ValueError(f'{position} is not a valid position.')

        if self._board[position] != ct.BoardPosValue.EMPTY:
            raise ValueError(f"Can't play {position}; this position is already"
                             f" taken by {str(self.board[position])}")

        self._board[position] = self.current_player
        self.moves.append(position)
        self._update_cells_conquered(position)
        self.next_player()
        return self.game_won()

    def undo_move(self) -> None:
        """Undoes one move
        """
        if not self.moves:
            return

        position = self.moves.pop()
        self.logger.debug(f'Undoing move: {position}')
        self._board[position] = ct.BoardPosValue.EMPTY
        self.winner = ct.BoardPosValue.EMPTY
        self.next_player()
        self._full_update_cells_conquered()

    def reset(self) -> None:
        """Resets the board to its initial (empty) position
        """
        self.logger.debug('Resetting board')
        self.__init__()

    def _update_cells_conquered(self, position: str) -> None:
        """Updates the conquered cells after position is played

        Args:
            position (str): last played position
        """
        self.logger.debug(f'Updating conquered cells after playing {position}')
        # Check all cells
        for cell, cell_poss in ct.CELLS.items():

            # If the move is in the cell and the cell is not taken yet...
            if (position in cell_poss and
                cell not in (self.cells_conquered[ct.BoardPosValue.PLAYER1] |
                             self.cells_conquered[ct.BoardPosValue.PLAYER2])):

                # Count the number of positions for current player
                positions = sum(self._board[pos] == self._board[position]
                                for pos in cell_poss)

                # If this is more than half of the positions, claim it!
                if positions * 2 >= len(cell_poss):
                    self.cells_conquered[self._board[position]].add(cell)
                    self.cells_conquered[ct.BoardPosValue.EMPTY].remove(cell)

                    self.logger.info(
                        f'After {position=}, {cell=} with points '
                        f'{cell_poss} is added for {self.current_player}; '
                        f'player controls {positions=} points of that cell.'
                        f' Conquered cells are now: {self.cells_conquered=}')

    def _full_update_cells_conquered(self) -> None:
        """Makes a full update of the conquered cells by replaying the game
        """
        self.logger.debug('Replaying game to update conquered cells...')

        stored_moves = list(self.moves)
        self.reset()
        for move in stored_moves:
            self.play_move(move)

    def game_won(self) -> bool:
        """ Checks if the game is won by one of the players
            Winning player is stored in self.winner

        Returns:
            bool: True if the game is won; False if it isn't
        """
        if self.winner is not ct.BoardPosValue.EMPTY:
            return True

        for player, cell_dim in [(ct.BoardPosValue.PLAYER1, 1),
                                 (ct.BoardPosValue.PLAYER2, 0)]:
            self.logger.debug(f'Checking if {player=} has won...')
            player_cells = self.cells_conquered[player]

            # Do a quick check to see if player has a cell in exactly 2 sides
            if len({cell[cell_dim] for cell in player_cells
                   if (cell[cell_dim] <= ct.CELL_LOW_DIM
                       or cell[cell_dim] >= ct.CELL_HIGH_DIM)}) == 2:
                self.logger.debug(f'{player} had no cells at both borders')
                continue  # go to the next player in the for loop

            # Loop over each of the player's cell in the top or left row
            for cell in (cell for cell in player_cells
                         if cell[cell_dim] <= ct.CELL_LOW_DIM):
                # Keep adding points to the set of positions that are adjacent
                # to the cell until no more cells can be added. Also keep
                # track of the connected cells.
                connected_cells = set()
                pos_cloud = set(ct.CELLS[cell])
                cell_count = -1

                # Loop while cells are added in the loop
                while len(connected_cells) > cell_count:
                    cell_count = len(connected_cells)

                    # Loop over all the player's cells
                    for other_cell in player_cells:
                        # Loop over all positions of that player's cell
                        for pos in ct.CELLS[other_cell]:
                            # check if other_cell is adjacent to the cells
                            # connected to cell - we check this by matching
                            # positions iteratively between cell and other_cell
                            if pos in pos_cloud:
                                # Add position
                                pos_cloud |= set(ct.CELLS[other_cell])

                                # Add cell
                                connected_cells.add(other_cell)

                                # Break the loop; cell and pos's already added
                                break

                self.logger.debug(f'For {player=}, all points connected to '
                                  f'{cell} are: {connected_cells}')

                # Check if we can reach the other side
                if any(True for cell in connected_cells
                       if cell[cell_dim] == ct.CELL_HIGH_DIM):
                    self.logger.info(f'{player} has won!')
                    self.winner = player
                    return True

        self.logger.info('None of the players has won yet...')
        return False

    def free_positions(self) -> list:
        """Gives a list of free (non-empty) positions

        Returns:
            list: list of positions (capital letter + number)
        """
        return [pos for pos in ct.POSITIONS
                if self._board[pos] == ct.BoardPosValue.EMPTY]

    def __str__(self) -> str:
        """Returns a string representation of the board

        Returns:
            str: string representation of the board
        """
        result = ''

        # Generate board and plot positions
        for segment, pos in zip(ct.BOARD_ASCII_SEGMENTS, ct.POSITIONS):
            result += segment
            if self._board[pos] == ct.BoardPosValue.EMPTY:
                pos_char = 'O'
            else:
                pos_char = str(self._board[pos])[-1]
            result += pos_char

        # Replace cell coordinates with correct string values
        for x, y in ct.CELLS:
            for owner in ct.BoardPosValue:
                if (x, y) in self.cells_conquered[owner]:
                    result = result.replace(f'{x},{y}', ct.ASCII_CELL[owner])
                    break  # break out of inner for loop

        return result + '\n'

    def load(self, filename: str) -> None:
        """Loads a game from a txt file in LittleGolem format

        Args:
            filename (str): file name of the file to be loaded

        Raises:
            ValueError: if the signature is not found in the file
            ValueError: if file is empty
            ValueError: if the player names or moves could not be read
        """

        # Only first (and only) line is relevant
        with open(filename, 'r') as file:
            content = file.readline()
            self.logger.debug(f'Read file {filename}: {content}')

        if not content:
            raise ValueError(f'Could not read file {filename}; no content.')

        if ct.READ_MARKERS['SIGNATURE'] not in content:
            raise ValueError(f"Signature '{ct.READ_MARKERS['SIGNATURE']}' not "
                             f"found in file {filename}")

        # Parse player names
        try:
            # Find indices where player names start
            player_idx = [content.find(key)
                          for key in ct.READ_MARKERS['PLAYERS'].values()]

            # Extract player names
            player_names = [content[idx + 3:content.find(']', idx)]
                            for idx in player_idx]

            self.logger.debug(f'Read player names: {player_names}')

        except Exception:
            raise ValueError(f'Could not read player names from {filename}')

        # Parse moves
        try:
            # Split the content in fields; then check if the turn markers
            # are in a field. If so, extract the move and put it in the list.
            # This gives a list of moves like ['H5', 'I7', 'H7']
            fields = content.split(ct.READ_MARKERS['FIELD_SEPARATOR'])
            moves = [field[2:field.find(']')] for field in fields
                     if field[:2] in ct.READ_MARKERS['TURNS']]
        except Exception:
            raise ValueError(f'Could not read moves from {filename}')

        # File is read. Now set the player names and replay the game
        self.reset()
        self.set_player_names(*player_names)
        for move in moves:
            self.play_move(move)

        self.logger.info(f'Successfully read file {filename}')

    def save(self, filename: str) -> None:
        raise NotImplementedError


def main():
    import random
    b = Conhex_game()
    while not b.game_won():
        pos = random.choice(b.free_positions())
        print(f'Playing {pos=} for {str(b.current_player)}')
        b.play_move(pos)

    print(f'The game is won by {b.winner}')
    print(b)


if __name__ == "__main__":
    main()

The file constants.py

import enum
import logging

#
# User adjustable configuration settings
#
LOG_LEVEL = logging.ERROR


#
# Logging
#
LOGGER = 'conhex'
LOG_FORMAT = ('[%(levelname)s] [%(asctime)s] [%(filename)s:(%(lineno)d] '
              '%(message)s')
logging.basicConfig(format=LOG_FORMAT, level=LOG_LEVEL)


#
# Board layout: cells and positions, board dimensions
#
CELLS = {
    (1, 1): ['A1', 'B3', 'C2'],
    (1, 3): ['B3', 'B4', 'B5'],
    (1, 5): ['B5', 'B6', 'B7'],
    (1, 7): ['B7', 'B8', 'B9'],
    (1, 9): ['A11', 'B9', 'C10'],
    (2, 2): ['B3', 'B4', 'C2', 'C4', 'D2', 'D3'],
    (2, 4): ['B4', 'B5', 'B6', 'C4', 'C5', 'C6'],
    (2, 6): ['B6', 'B7', 'B8', 'C6', 'C7', 'C8'],
    (2, 8): ['B8', 'B9', 'C10', 'C8', 'D10', 'D9'],
    (3, 1): ['C2', 'D2', 'E2'],
    (3, 3): ['C4', 'C5', 'D3', 'D5', 'E3', 'E4'],
    (3, 5): ['C5', 'C6', 'C7', 'D5', 'D6', 'D7'],
    (3, 7): ['C7', 'C8', 'D7', 'D9', 'E8', 'E9'],
    (3, 9): ['C10', 'D10', 'E10'],
    (4, 2): ['D2', 'D3', 'E2', 'E3', 'F2', 'F3'],
    (4, 4): ['D5', 'D6', 'E4', 'E6', 'F4', 'F5'],
    (4, 6): ['F4', 'F5', 'G4', 'G6', 'H5', 'H6'],
    (4, 8): ['D10', 'D9', 'E10', 'E9', 'F10', 'F9'],
    (5, 1): ['E2', 'F2', 'G2'],
    (5, 3): ['E3', 'E4', 'F3', 'F4', 'G3', 'G4'],
    (5, 5): ['E6', 'F5', 'F6', 'F7', 'G6'],
    (5, 7): ['E8', 'E9', 'F8', 'F9', 'G8', 'G9'],
    (5, 9): ['E10', 'F10', 'G10'],
    (6, 2): ['F2', 'F3', 'G2', 'G3', 'H2', 'H3'],
    (6, 4): ['D6', 'D7', 'E6', 'E8', 'F7', 'F8'],
    (6, 6): ['F7', 'F8', 'G6', 'G8', 'H6', 'H7'],
    (6, 8): ['F10', 'F9', 'G10', 'G9', 'H10', 'H9'],
    (7, 1): ['G2', 'H2', 'I2'],
    (7, 3): ['G3', 'G4', 'H3', 'H5', 'I4', 'I5'],
    (7, 5): ['H5', 'H6', 'H7', 'I5', 'I6', 'I7'],
    (7, 7): ['G8', 'G9', 'H7', 'H9', 'I7', 'I8'],
    (7, 9): ['G10', 'H10', 'I10'],
    (8, 2): ['H2', 'H3', 'I2', 'I4', 'J3', 'J4'],
    (8, 4): ['I4', 'I5', 'I6', 'J4', 'J5', 'J6'],
    (8, 6): ['I6', 'I7', 'I8', 'J6', 'J7', 'J8'],
    (8, 8): ['H10', 'H9', 'I10', 'I8', 'J8', 'J9'],
    (9, 1): ['I2', 'J3', 'K1'],
    (9, 3): ['J3', 'J4', 'J5'],
    (9, 5): ['J5', 'J6', 'J7'],
    (9, 7): ['J7', 'J8', 'J9'],
    (9, 9): ['I10', 'J9', 'K11']
}

POSITIONS = sorted({position for cell in CELLS.values() for position in cell},
                   key=lambda p: (int(p[1:]), p[0]))

CELL_LOW_DIM = 2   # Row/column 1 *and* 2 lie at the border
CELL_HIGH_DIM = 8  # Row/column 8 *and* 9 lie at the border


#
# Players, default player names and possible values of the board positions
#
class BoardPosValue(enum.Enum):
    """Enum class for defining the state of a position on the board
    """

    def _generate_next_value_(name: str, *_) -> str:
        return name

    EMPTY = enum.auto()
    PLAYER1 = enum.auto()
    PLAYER2 = enum.auto()


DEFAULT_PLAYER_NAMES = {
    BoardPosValue.PLAYER1: 'Player 1',
    BoardPosValue.PLAYER2: 'Player 2',
}


#
# ASCII representation of the board
#
__ASCII_BOARD__ = \
    """   A     B     C     D     E     F     G     H     I     J     K
 1 #-----------+-----------+-----------+-----------+-----------#
   |           |    3,1    |    5,1    |    7,1    |           |
   |           |           |           |           |           |
 2 |   1,1     #-----#-----#-----#-----#-----#-----#     9,1   |
   |         /       |    4,2    |    6,2    |       \         |
   |       /         |           |           |         \       |
 3 +-----#    2,2    #-----#-----#-----#-----#    8,2    #-----+
   |     |         /       |    5,3    |       \         |     |
   |     |       /         |           |         \       |     |
 4 | 1,3 #-----#    3,3    #-----#-----#    7,3    #-----# 9,3 |
   |     |     |         /       |       \         |     |     |
   |     |     |       /         |         \       |     |     |
 5 +-----# 2,4 #-----#    4,4    #    6,4   #------# 8,4 #-----+
   |     |     |     |         /   \        |      |     |     |
   |     |     |     |       /  5,5  \      |      |     |     |
 6 | 1,5 #-----# 3,5 #------#    #    #-----# 7,5  #-----# 9,5 |
   |     |     |     |       \       /      |      |     |     |
   |     |     |     |         \   /        |      |     |     |
 7 +-----# 2,6 #-----#     4,6   #    6,6   #------# 8,6 #-----+
   |     |     |       \         |         /       |     |     |
   |     |     |         \       |       /         |     |     |
 8 | 1,7 #-----#    3,7    #-----#-----#    7,7    #-----# 9,7 |
   |     |       \         |           |         /       |     |
   |     |         \       |    5,7    |       /         |     |
 9 +-----#    2,8    #-----#-----#-----#-----#    8,8    #-----+
   |       \         |           |           |         /       |
   |         \       |    4,8    |    6,8    |       /         |
10 |   1,9     #-----#-----#-----#-----#-----#-----#    9,9    |
   |           |           |           |           |           |
   |           |    3,9    |    5,9    |    7,9    |           |
11 #-----------+-----------+-----------+-----------+-----------#""" \
# noqa: W605  - ignore escape sequence warming for the ascii board

BOARD_ASCII_SEGMENTS = __ASCII_BOARD__.split('#')

ASCII_CELL = {
    BoardPosValue.PLAYER1: '-1-',
    BoardPosValue.PLAYER2: '-2-',
    BoardPosValue.EMPTY: '   ',
}


#
# String markers for reading and writing files
#
READ_MARKERS = {
    'SIGNATURE': 'FF[CONHEX]VA[CONHEX]EV[conhex.ld.CONHEX]',
    'PLAYERS': {
        BoardPosValue.PLAYER1: 'PW',
        BoardPosValue.PLAYER2: 'PB',
    },
    'TURNS': ('B[', 'R['),
    'FIELD_SEPARATOR': ';',
}

if __name__ == "__main__":
    pass

Answer 1

Overall it's pretty good.

Conventionally Conhex_game would be ConhexGame (UpperCamelCase for classes).

ct is a non-obvious abbreviation for constants, and I don't see a whole lot of value in aliasing it. You can just from constants import ....

The hint -> ct.enum.Enum: doesn't seem right. You're returning a player, which is a BoardPosValue right?

play_move returning a boolean doesn't seem to add a lot of value. It would be more legible on the calling side if you simply don't return anything and force the caller to ask for game_won themselves.

Re. _update_cells_conquered: your outer loop seems like it could be improved in efficiency. You should create an index that, given a position, is able to look up a cell in O(1) time.

game_won is a little nasty and should be decomposed into multiple methods.

This pattern:

    except Exception:
        raise ValueError(f'Could not read player names from {filename}')

is a little problematic. First, you should only be catching the exceptions you reasonably expect, probably IndexError in this case. Second, don't trash the trace. Use except IndexError as e: raise ValueError(...) from e to preserve it. Third, it's probably a better idea to define and raise your own exception types rather than ValueError.

Your type hints are incomplete. main() needs a -> None, and free_positions should return a list[str] I think, assuming position is a string. mypy will tell you these issues when properly configured.

I consider ASCII_BOARD to be large enough that it deserves its own .txt file rather than living in code. Also, don't name it with double underscores; it's a plain-old constant.

There's a formatting problem with your ASCII_BOARD that threw me for a loop. The first line is misaligned and makes the column assignments non-obvious. This will be fixed if you move it to a text file.

You ask:

Specifically review the way I'm using the Python logger. Is this a Pythonic and "good" way to use logging (e.g.: logger initialising, storing a logger object in a class, points where I call the logger, etc.)?

Putting the logger in the class is sketchy, especially in your case since you pull a nasty trick and re-issue calls to __init__ on existing instances. Probably safer to do the usual thing and move it to globals. Consider moving these:

LOGGER = 'conhex'
LOG_FORMAT = ('[%(levelname)s] [%(asctime)s] [%(filename)s:(%(lineno)d] '
              '%(message)s')
logging.basicConfig(format=LOG_FORMAT, level=LOG_LEVEL)

into a logger initialisation function called from the global namespace, possibly something like

def config_logger():
    logging.basicConfig(
        format='[%(levelname)s] [%(asctime)s] [%(filename)s:(%(lineno)d] '
               '%(message)s',
        level=logging.ERROR,
    )
    return logging.getLogger('conhex')

logger = config_logger()

You ask:

Optically, the code seems a bit messy. I tried to adhere to PEP8 and the code passes Flake8. Any suggestions to improve code readability?

Delete Returns: None since that's obvious. And as above, subdivide your longest method into multiple shorter ones. Otherwise the optics are fine.

reset()ting an entire instance, or especially re-calling __init__, is evidence of a kind of mutation that makes debugging and some functional code patterns more difficult. Attempt to migrate to more immutable instances, and if it's possible, instead of reset(), simply abandon the old instance and construct a new one out-of-place.

Answer 2

I also think that overall your code looks pretty good. There's no magic numbers floating around, methods are generally short - although there are exceptions and that ties neatly into the documenting question: I like the philosophy that code should be self documenting, and your variable and function names seem to generally describe their intent. However, sometimes comments are needed to describe code blocks. This is usually a very good sign that that code block can be method extracted, and with a properly named method the comment is no longer needed and the calling function is easier to read, since you don't need to understand the code block, just the intent - which is the called method's name.

Both the _update_cells_conquered and game_won methods are long and nested. Each nesting can probably method extracted. Take the _update_cells_conquered for example:

    def _update_cells_conquered(self, position: str) -> None:
        """Updates the conquered cells after position is played

        Args:
            position (str): last played position
        """
        self.logger.debug(f'Updating conquered cells after playing {position}')
        # Check all cells
        for cell, cell_poss in ct.CELLS.items():
            self._claim_claimables(cell, cell_poss, position)

    def _claim_claimables(self, cell, cell_poss, position):
        # If the move is in the cell and the cell is not taken yet...
        if self._cell_claimable(cell, cell_poss, position):
            self._check_and_claim(cell, cell_poss, position)

    def _check_and_claim(self, cell, cell_poss, position):
        # Count the number of positions for current player
        positions = self._position_count(cell_poss, position)  # rename to position_count?
        # If this is more than half of the positions, claim it!
        if self._count_sufficient(positions, cell_poss):
            self._claim_cell(cell, cell_poss, position, positions)

    def _claim_cell(self, cell, cell_poss, position, positions):
        self.cells_conquered[self._board[position]].add(cell)
        self.cells_conquered[ct.BoardPosValue.EMPTY].remove(cell)
        self.logger.info(
            f'After {position=}, {cell=} with points '
            f'{cell_poss} is added for {self.current_player}; '
            f'player controls {positions=} points of that cell.'
            f' Conquered cells are now: {self.cells_conquered=}')

    def _count_sufficient(self, positions, cell_poss):
        return positions * 2 >= len(cell_poss)

    def _position_count(self, cell_poss, position):
        return sum(self._board[pos] == self._board[position]
                   for pos in cell_poss)

    def _cell_claimable(self, cell, cell_poss, position):
        return position in cell_poss \
               and cell not in (self.cells_conquered[ct.BoardPosValue.PLAYER1] |
                                self.cells_conquered[ct.BoardPosValue.PLAYER2])

To be honest, I am not proud of the name claim_claimables but it should suffice to communicate the point. What I've done is take every code block that can be combined (extracted) and gave it a name - naming it after its intent (small disclaimer: I only skimmed the game rules, so my namings may be a little off, you may have to adjust them). This of course will add a lot of overhead and blow up classes, but this can be resolved with extract class refactorings to make classes do one thing again. The biggest improvement in my eyes though, is that most comments are not needed anymore and I can read through the code without looking at some specifics, but still understanding their intent.