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):
ConHex coordinate system of positions and cells
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
-
\$\begingroup\$ For those interested, I added a GUI with PySimpleGUI, which allows you to use this code interactively. Please see: github.com/agtoever/ConHex. Apart from some minor changes an a little bugfix, the code on GitHub is practically equal to the code above. So the review request still stands... \$\endgroup\$agtoever– agtoever2022年05月26日 10:01:12 +00:00Commented May 26, 2022 at 10:01
-
\$\begingroup\$ The picture for your coordinate system is wrong. Bottom row ("First coordinate of the cells") aren't supposed to be numbers. \$\endgroup\$Mast– Mast ♦2022年05月27日 06:11:21 +00:00Commented May 27, 2022 at 6:11
-
\$\begingroup\$ Considering no answers have been posted yet, feel free to replace the code with the improved one on GitHub. \$\endgroup\$Mast– Mast ♦2022年05月27日 06:12:21 +00:00Commented May 27, 2022 at 6:12
-
\$\begingroup\$ @Mast : thanks for your comments. There are two coordinate systems: one for the positions (circles), which is indeed letter+number. Ref up/left in drawing. The cells also have (different) coordinates. Number, number. Those are right/bottom in the image. \$\endgroup\$agtoever– agtoever2022年05月27日 07:02:52 +00:00Commented May 27, 2022 at 7:02
-
1\$\begingroup\$ Yea, our policy can be a bit confusing. The 'once posted, don't change the code' applies when answers have arrived since answer invalidation can give everyone involved a headache. No problem. \$\endgroup\$Mast– Mast ♦2022年05月27日 07:18:25 +00:00Commented May 27, 2022 at 7:18
2 Answers 2
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.
-
\$\begingroup\$ Thank you for your review! One thing I don’t understand and that’s your last sentence ("moving these...global namespace.") Could you please clarify what you mean by this? Second: could you please (briefly) address the fourth review question (code looks optically messy)? Specifically in methods with a lot of indentation levels (such ad update_cells_conquered, game_won, str), I’m not happy "how the code looks". Any reflection on that? Do you (as an experienced reader of code written by others) find it readable enough? Again: thank you for your time and constructive and very clear review!!! \$\endgroup\$agtoever– agtoever2022年06月06日 05:59:08 +00:00Commented Jun 6, 2022 at 5:59
-
\$\begingroup\$ @agtoever Alright, I've attempted an edit. \$\endgroup\$Reinderien– Reinderien2022年06月06日 12:13:34 +00:00Commented Jun 6, 2022 at 12:13
-
1\$\begingroup\$ thank you for this addition. +100 points well deserved. Again: thank you for your time! \$\endgroup\$agtoever– agtoever2022年06月06日 18:26:34 +00:00Commented Jun 6, 2022 at 18:26
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.
-
1\$\begingroup\$ Thanks you. That’s some very clear and usefull advice. I’ll try to keep that in mind. \$\endgroup\$agtoever– agtoever2022年06月06日 18:39:29 +00:00Commented Jun 6, 2022 at 18:39
-
1\$\begingroup\$ @agtoever if you're looking for more similar useful advice, I'd suggest reading Clean Code by uncle Bob or Refactoring by Martin Fowler. They're both really good books for every programmer to have read and will get you quite far with keeping OOP projects understandable. \$\endgroup\$lukstru– lukstru2022年06月06日 18:49:37 +00:00Commented Jun 6, 2022 at 18:49
-
\$\begingroup\$ tbh my advice was completely copied (well, recalled from my mind) from Clean Code. \$\endgroup\$lukstru– lukstru2022年06月06日 18:52:15 +00:00Commented Jun 6, 2022 at 18:52