I have 3 classes to create a simple checkers game move validator. The entry point is checkers.py
and is mostly complete with a few things hard-coded. For example, the auto-generated board (if none is given) starts with the upper left square as white, whereas if we are given a board, we are assuming the upper left square is black (black = legal square we can move to, white squares we cannot move to). Also when validating a move, we are only considering the case where the piece we are moving can move diagonally downwards (e.g. the piece started at the top of the board and is not a king).
I feel like my design is not very good and there is too much hard-coding, so I want to get some feedback before I continue any further. Please let me know what improvements I can make.
checkers.py
from piece import Piece, Side
from validator import Validator
class Checkers:
'''
Checkers class can either generate an empty board or take in a board. The two different checkers boards available
are either top left square black or white and then they alternate. I have top left color as black hard coded ri
ght now (if a board is given). If a board is not given, the board is harded coded for top left color as white.
'''
def __init__(self, board=None):
self.BOARD_ROWS, self.BOARD_COLS = 8, 8
self.legal_spaces = set()
if not board:
self.board = [[None for _ in range(self.BOARD_COLS)] for _ in range(self.BOARD_ROWS)]
for row in range(self.BOARD_ROWS):
if (row % 2) == 0:
for col in range(1, self.BOARD_COLS, 2):
self.legal_spaces.add((row, col))
if 0 <= row <= 2:
self.board[row][col] = Piece(Side.RED)
elif 5 <= row <= 7:
self.board[row][col] = Piece(Side.BLACK)
else:
for col in range(0, self.BOARD_COLS, 2):
self.legal_spaces.add((row, col))
if 0 <= row <= 2:
self.board[row][col] = Piece(Side.RED)
elif 5 <= row <= 7:
self.board[row][col] = Piece(Side.BLACK)
elif Validator.validate_board(board):
self.board = board
self.generate_legal_spaces('B')
for row in range(self.BOARD_ROWS):
for col in range(self.BOARD_COLS):
match board[row][col]:
case 'R':
board[row][col] = Piece(Side.RED)
case 'B':
board[row][col] = Piece(Side.BLACK)
case _:
board[row][col] = None
else:
raise ValueError('Invalid Board.')
def generate_legal_spaces(self, top_left_color):
if top_left_color not in ('B', 'W'):
raise ValueError('Incorrect top left color provided')
for row in range(self.BOARD_ROWS):
if top_left_color == 'B':
if (row % 2 == 0):
self.legal_spaces.update([(row, 0), (row, 2), (row, 4), (row, 6)])
else:
self.legal_spaces.update([(row, 1), (row, 3), (row, 5), (row, 7)])
else:
if (row % 2 == 0):
self.legal_spaces.update([(row, 1), (row, 3), (row, 5), (row, 7)])
else:
self.legal_spaces.update([(row, 0), (row, 2), (row, 4), (row, 6)])
def __str__(self):
output = ''
for row in range(self.BOARD_ROWS):
for col in range(self.BOARD_COLS):
square = self.board[row][col]
if not square:
output += '_'
else:
output += square.side.value
if col != self.BOARD_COLS - 1:
output += ' '
if row != self.BOARD_ROWS - 1:
output+= '\n'
return output
validator.py
from piece import Side
class Validator:
@staticmethod
def validate_board(board):
BOARD_ROWS, BOARD_COLS = 8, 8
invalid_vectors = [(0, 1), (0, -1), (1, 0), (-1, 0)]
if len(board) != BOARD_ROWS:
return False
def occupied(location):
row, col = location[0], location[1]
if (0 <= row <= 7) and (0 <= col <= 7) and board[row][col]:
return True
return False
for row in range(BOARD_ROWS):
for col in range(BOARD_COLS):
curr_location = (row, col)
if occupied(curr_location):
for invalid_vector in invalid_vectors:
invalid_candidate = tuple([sum(x) for x in zip(curr_location, invalid_vector)])
if occupied(invalid_candidate):
return False
return True
@staticmethod
def valid_location(legal_spaces, location):
row, col = location[0], location[1]
if not (0 <= row <= 7) or not (0 <= col <= 7):
return False
if location not in legal_spaces:
return False
return True
@staticmethod
def occupied(board, location):
return not Validator.unoccupied(board, location)
@staticmethod
def unoccupied(board, location):
row, col = location[0], location[1]
if not (0 <= row <= 7) or not (0 <= col <= 7):
return False
return not board[row][col]
@staticmethod
def occupied_side(board, location, side):
# is square within board and occupied by the color given in side
row, col = location[0], location[1]
if not (0 <= row <= 7) or not (0 <= col <= 7):
return False
piece = board[row][col]
if piece and piece.side == side:
return True
return False
@staticmethod
def validate_move(board, legal_spaces, start, end):
'''
Only handles the case where starting piece is red and not a king,
so we can only move diagonally downwards
'''
if not Validator.valid_location(legal_spaces, start) or not \
Validator.valid_location(legal_spaces, end):
raise ValueError('Provided start or end location is out of bounds')
start_row, start_col = start[0], start[1]
end_row, end_col = end[0], end[1]
# check if we are ending at unoccupied square
if Validator.occupied(board, end):
return False
piece = board[start_row][start_col]
villain_side = Side.BLACK if (piece.side == Side.RED) else Side.RED
move_vectors = [(1, -1), (1, 1)]
jump_vectors = [{'villain_vector': (1, -1), 'captured_vector': (2, -2)}, \
{'villain_vector': (1, 1), 'captured_vector': (2, 2)}]
# can we get to end by just moving
for move_vector in move_vectors:
move_candidate = tuple(sum(x) for x in zip(start, move_vector))
if Validator.unoccupied(board, move_candidate) and move_candidate == end:
return True
# cannot get to end by move, need to jump
return Validator.validate_jump(board, legal_spaces, jump_vectors, start, end, villain_side)
@staticmethod
def validate_jump(board, legal_spaces, jump_vectors, curr, target, villain_side):
if curr == target:
return True
if curr in legal_spaces:
for jump_vector in jump_vectors:
villain_candidate = tuple(sum(x) for x in zip(curr, jump_vector['villain_vector']))
jump_candidate = tuple(sum(x) for x in zip(curr, jump_vector['captured_vector']))
if Validator.occupied_side(board, villain_candidate, villain_side) and Validator.unoccupied(board, jump_candidate):
villain_row, villain_col = villain_candidate[0], villain_candidate[1]
captured_piece = board[villain_row][villain_col]
board[villain_row][villain_col] = None
return Validator.validate_jump(board, legal_spaces, jump_vectors, jump_candidate, target, villain_side)
board[villain_row][villain_col] = captured_piece
return False
piece.py
from enum import Enum
class Side(Enum):
RED = 'R'
BLACK = 'B'
class Piece:
def __init__(self, side):
if side not in (Side.RED, Side.BLACK):
raise ValueError('Piece must be either red or black and be of type Side.')
self.side = side
self.is_king = False
def __str__(self):
return f'<Color: {self.side}, King: {self.is_king}>'
3 Answers 3
Documentation
The Checkers
class docstring has lines that are too long.
There is also a word split across 2 lines:
coded ri
ght now
This has shorter lines:
'''
Checkers class can either generate an empty board or take in a board. The
two different checkers boards available are either top left square black
or white and then they alternate. I have top left color as black hard
coded right now (if a board is given). If a board is not given, the board
is hard coded for top left color as white.
'''
The PEP 8 style guide recommends adding docstrings for each class, as well as functions.
OOP
It is more common to use self
instead of the class name
when accessing class members, such as Validator.unoccupied
in the following line:
return not Validator.unoccupied(board, location)
You did this in the other 2 classes.
DRY
These lines are repeated 4 times in different functions in the
Validator
class:
row, col = location[0], location[1]
if not (0 <= row <= 7) or not (0 <= col <= 7):
Also, the same check is repeated within the line.
You can factor this code out to a new function to reduce the repetition.
You probably also want to use the constants (BOARD_ROWS
, BOARD_COLS
)
instead of the magic number 7
.
Simpler
In Checkers
__str__
, the code could be simpler if you invert the logic.
Change:
if not square:
output += '_'
else:
output += square.side.value
to:
if square:
output += square.side.value
else:
output += '_'
Unreachable code
The line after the return
line in the validate_jump
function
will never be reached and executed:
return Validator.validate_jump(board, legal_spaces, jump_vectors, jump_candidate, target, villain_side)
board[villain_row][villain_col] = captured_piece
Review the code to see if you really meant to use the line for something; otherwise, you should delete the line.
tuple unpack
In a few places we see
row, col = location[0], location[1]
Prefer to unpack that tuple in the usual way:
row, col = location
Complexity
Your Checkers.__init__
has a reasonably high complexity.
I would structure it as:
class Checkers:
def __init__(self, board=None):
if board is None:
self._create_new_board()
else:
self._validate_board(board)
This signals two distinct paths for the initialization of Checkers.
Subjectively, for maintenance purposes if you never changed the arguments to it (i.e. board
) you would never need to amend this method, which is good because amending the root initialization method of classes can be dangerous. Of course you might want to amend the create_new_board
and validate_board
methods but in my opinion that would be easier for colleagues to review.
Explore related questions
See similar questions with these tags.