On the whole, this game works nicely and the design decisions make sense for the size and purpose of the program. Variable names are clear, there's some separation between GUI and game logic and you've made an effort to use functions. Some of my suggestions might be premature, but if you plan to extend the program to add a feature like an AI or undo/redo moves, I'd suggest a redesign.
Crash
On my screen, a white bar renders on the right side of the game. If you click it, drop_a_piece
raises an uncaught IndexError
because it has no bounds checking.
Avoid unnecessary comments
Many of the comments are redundant:
# drop a piece
def drop_piece(board, row, col, piece):
board[row][col] = piece
I can tell from the function name that this drops a piece. Comments like this feel insulting to the reader's intelligence. If you want to document your functions, the Python way is to use docstrings and (optionally) doctests to enforce contracts.
Occasional #
comments are OK, as long as they actually offer deep insight into the code that isn't necessarily apparent otherwise. You have a few of these, like
# turn decision, if black(1)/white(2) piece already placed, go back to the previous turn
...but make sure these comments aren't crutches to make up for unnecessarily confusing code.
Avoid comments as a substitute for proper code abstractions
Other comments are used in place of namespaces or functions, real program structures that organize logic.
For example, something as simple as:
# colors
BLACK = (0,0,0)
WHITE = (255,255,255)
BROWN = (205,128,0)
could be:
class Colors:
BLACK = 0, 0, 0
WHITE = 255, 255, 255
BROWN = 205, 128, 0
An example of a comment that seems to be trying to denote a function is in who_wins
# check for vertical win
for c in range(COL_COUNT):
# ... more code ...
which might as well be broken out into a function called check_vertical_win
. After following this to its conclusion, who_wins
would look like:
def who_wins(board, piece):
return (
check_left_diagonal_win(board, piece) or
check_right_diagonal_win(board, piece) or
check_vertical_win(board, piece) or
check_horizontal_win(board, piece)
)
The problem now is that board
and piece
have to be passed around through multiple layers of these C-style, non-OOP functions, which I'll discuss later.
Go all-in or all-out with NumPy
If you're bringing in NumPy, you might as well use it to its full potential! The only NumPy call in the whole code is board = np.zeros((row,col))
. This seems like a missed opportunity -- you should be able to vectorize many of your operations and write idiomatic NumPy code. This might seem premature, but if you were to add an AI that needs to traverse the game tree, NumPy can offer potentially massive efficiency boosts. Readability would hopefully improve.
If you're not going to use NumPy as more than a glorified 2d list print formatter, I'd drop the dependency and add a few-line pretty print helper, or remove the print
entirely since it's a GUI game.
Go all-in or all-out on making it a module
The code has the "driver" code that's typical of modules that expect to be imported as a distinct package:
if __name__ == '__main__':
main()
but due to all of the scattershot variables and functions outside of main
, it's not a very convenient module that would be usable if imported, so this seems like a false promise.
main
should be really simple -- that's your client using the code as a black box library (think of it like how you use NumPy), so you're asking them to implement 80 lines in order to use your functions to implement a Gomoku game. Clearly, far too much of the game logic has been dumped into main
. Ideally, you'd get that down to a couple lines or so that work as a black box, possibly with some configutation knobs, something like:
if __name__ == '__main__':
game = Gomoku(some_config)
game.play()
Creating a class to operate on board state, or at least using modules and namespacing your functions and data can help solve the problem of having to pass board
to your functions and break encapsulation constantly to read globals.
I'd prefer a Gomoku
class that runs the GUI and game loop which is the client of a GomokuPosition
class that encompasses the board, ply and methods that operate on game state.
Reduce cyclomatic complexity
You do a pretty good job of using fairly small and single-ish responsibility functions up until main
, when complexity blows up. There are 13 branches and loops that nest up to 6 deep:
while not game_over:
for event in pygame.event.get():
elif event.type == pygame.MOUSEBUTTONDOWN:
if turn == 0:
if is_valid_loc(board, row, col):
if who_wins(board,piece_1):
# you may ask yourself, "well... how did I get here?"
This is difficult to reason about and should be multiple, single-purpose functions, keeping UI and game logic as separate as possible (helpful if you plan to add AI).
Keep nesting to no more than 2-3 blocks deep and keep if
/else
chains to no more than 2-3 branches long, otherwise the function is too complex.
Avoid long lines
I have to horizontally scroll to read lines like:
if board[r][c] == piece and board[r+1][c+1] == piece and board[r+2][c+2] == piece and board[r+3][c+3] == piece\
and board[r+4][c+4] == piece:
Please stick to a width of 80 characters. Checks like this could be broken out into functions or be broken into multiple lines with parentheses (almost always avoid the backslash to continue a line):
if (
board[r][c] == piece and
board[r+1][c+1] == piece and
board[r+2][c+2] == piece and
board[r+3][c+3] == piece and
board[r+4][c+4] == piece
):
(This uses the "sad face" style advocated by Black).
Better yet, there's a pattern here you can extract and exploit with NumPy:
if np.all(board.diagonal(r)[c:c+5] == piece):
This isn't just a matter of elegance, iterating at a higher level of abstraction in place of heavy reliance on low-level indexing/range()
should reduce bugs. A codebase with tons of verbose manual indexing tends to harbor subtle off-by-one, copy-paste and typo-style errors and can be hard to trust and validate.
If you're as inept at NumPy as I am, I can usually find the right vector by searching for stuff like "numpy diagonal slice". 99% of the time there's a builtin or a one-liner that puts my imperative Python for
loop code to shame. If you decide against using NumPy, you can still use slices (and often itertools
) to try to minimize error-prone and non-idiomatic range
calls and indexes.
DRY out similar code
The chunks of code # Ask for Player 1 move
and # Ask for Player 2 move
are pretty much the same with obvious parameters: piece_num
and on_win_message
. Code like this is ready for a function. After that refactor, they still bake together game logic and presentation/UI, so I'd tease those two apart.
Use intermediate variables to simplify code
Consider:
for x in range(BLOCKSIZE // 2, S_WIDTH - BLOCKSIZE // 2 + BLOCKSIZE, BLOCKSIZE):
line_start = (x, BLOCKSIZE // 2)
line_end = (x,S_HEIGHT-BLOCKSIZE // 2)
pygame.draw.line(screen, BLACK, line_start,line_end,2)
Assuming we won't use NumPy here, BLOCKSIZE // 2
is done repeatedly and the code is generally screamy/COBOL-y and hard on the eyes. Even using two local variables offers a bit of relief, gaining horizontal readability for a few extra vertical lines:
half = BLOCKSIZE // 2
end = S_WIDTH - half + BLOCKSIZE
for x in range(half, end, BLOCKSIZE):
line_start = x, half
line_end = x, S_HEIGHT - half
pygame.draw.line(screen, BLACK, line_start, line_end, width=2)
I snuck in width=2
, because it's hard to tell what that parameter means otherwise. It's generally a great idea to use named parameters.
Avoid magic variables/literals
The code
piece_1 = 1 # black
piece_2 = 2 # white
looks like an attempt at an enumeration but why not BLACK = 0
and WHITE = 1
? An empty square is a 0, so on further inspection this makes sense, but I'm not sure these pieces need to exist; the only places in the code these are used are:
drop_piece(board, row, col, piece_1)
draw_piece(SCREEN,board)
if who_wins(board,piece_1):
and another location, where they're substituted in favor of magic constants:
if board[row][col] == 1: # <-- really piece_1
turn = 0
if board[row][col] == 2: # <-- really piece_2
turn = 1
All of this seems a little ad-hoc.
Similarly, the hardcoded literal 4
appears many times inside of who_wins
. It's not clear why this wasn't made a constant as was done for the other board configuration numbers.
Simplify win checking
There's no need to scan the entire board to check a win. You can count in 4 directions from the last move, counting matching cells outward until you hit the desired total on one of the directions.
You can use a count of the number of turns taken so far to skip the entire win check if there are simply not enough pieces on the board for a win to be possible. For almost no expense, you get a massive performance boost if you add an AI that searches the tree later.
Minor nitpicks
In col = int(math.floor(x_pos / BLOCKSIZE))
, math.floor
already returns an int
so you can skip the extra call. Or skip both and use the floor division operator //
.
Unpacking can be elegant:
x_pos = event.pos[0]
y_pos = event.pos[1]
goes to x_pos, y_pos = event.pos
who_wins
is a bit oddly-worded because nobody may have won. I'd prefer is_won
, check_win
or just_won
if it only checks for a win on the last move as proposed above.
game_over
isn't necessary; you could break the loop (or return if it was a function) and run the after-game delay separately.
2-player turn-based strategy games usually use ply
instead of turn
. You can keep incrementing ply
and use its parity (ply & 1
or ply % 2 == 0
) to determine side. If you do use turn
as a single bit that flips back and forth, I'd prefer turn ^= 1
over
turn += 1
turn = turn % 2
Be consistent with spacing:
pygame.draw.line(screen, BLACK, line_start,line_end,2)
should be
pygame.draw.line(screen, BLACK, line_start, line_end, 2)
Since draws are possible when the board fills up without either side making 5 in a row, you might want to handle this in the UI and game logic.
Pluralization: draw_piece
really draws all the pieces, so it should be called draw_pieces
.
Caption typo: 'Gomoku (Connet 5)'
. Connect 4 is a drop connection game, so Gomoku is more like generalized mxnxk tic-tac-toe but I'm just being pedantic.
Alphabetize imports.
Possible rewrite
Here's the GomokuPosition
module:
class GomokuPosition:
dirs = (
((0, -1), (0, 1)),
((1, 0), (-1, 0)),
((1, 1), (-1, -1)),
((1, -1), (-1, 1)),
)
def __init__(self, rows, cols, n_to_win, players="wb", blank="."):
self.ply = 0
self.rows = rows
self.cols = cols
self.last_move = None
self.n_to_win = n_to_win
self.boards = [[[0] * cols for _ in range(rows)] for i in range(2)]
self.players = players
self.blank = blank
def board(self, row=None, col=None):
if row is None and col is None:
return self.boards[self.ply&1]
elif col is None:
return self.boards[self.ply&1][row]
return self.boards[self.ply&1][row][col]
def move(self, row, col):
if self.in_bounds(row, col) and self.is_empty(row, col):
self.board(row)[col] = 1
self.ply += 1
self.last_move = row, col
return True
return False
def is_empty(self, row, col):
return not any(board[row][col] for board in self.boards)
def in_bounds(self, y, x):
return y >= 0 and y < self.rows and x >= 0 and x < self.cols
def count_from_last_move(self, dy, dx):
if not self.last_move:
return 0
last_board = self.boards[(self.ply-1)&1]
y, x = self.last_move
run = 0
while self.in_bounds(y, x) and last_board[y][x]:
run += 1
x += dx
y += dy
return run
def just_won(self):
return self.ply >= self.n_to_win * 2 - 1 and any(
(self.count_from_last_move(*x) +
self.count_from_last_move(*y) - 1 >= self.n_to_win)
for x, y in self.dirs
)
def is_draw(self):
return self.ply >= self.rows * self.cols and not self.just_won()
def last_player(self):
if self.ply < 1:
raise IndexError("no moves have been made")
return self.players[(self.ply-1)&1]
def char_for_cell(self, row, col):
for i, char in enumerate(self.players):
if self.boards[i][row][col]:
return char
return self.blank
def to_grid(self):
return [
[self.char_for_cell(row, col) for col in range(self.cols)]
for row in range(self.rows)
]
def __repr__(self):
return "\n".join([" ".join(row) for row in self.to_grid()])
if __name__ == "__main__":
pos = GomokuPosition(rows=4, cols=4, n_to_win=3)
while not pos.just_won() and not pos.is_draw():
print(pos, "\n")
try:
if not pos.move(*map(int, input("[row col] :: ").split())):
print("try again")
except (ValueError, IndexError):
print("try again")
print(pos, "\n")
if pos.just_won():
print(pos.last_player(), "won")
else:
print("draw")
Now the Gomoku
GUI module can import the position and use it as a backend for the game logic as shown below. Admittedly, I got a little bored with the GUI so there are plenty of rough edges and questionable UX decisions left as an exercise for the reader.
import itertools
import pygame
from gomoku_position import GomokuPosition
class Colors:
BLACK = 0, 0, 0
WHITE = 255, 255, 255
BROWN = 205, 128, 0
class Gomoku:
def __init__(
self,
size=60,
piece_size=20,
rows=15,
cols=15,
n_to_win=5,
caption="Gomoku"
):
self.rows = rows
self.cols = cols
self.w = rows * size
self.h = cols * size
self.size = size
self.piece_size = piece_size
self.half_size = size // 2
pygame.init()
pygame.display.set_caption(caption)
self.screen = pygame.display.set_mode((self.w, self.h))
self.screen.fill(Colors.WHITE)
self.player_colors = {"w": Colors.WHITE, "b": Colors.BLACK}
self.player_names = {"w": "White", "b": "Black"}
self.board = GomokuPosition(rows, cols, n_to_win)
def row_lines(self):
half = self.half_size
for y in range(half, self.h - half + self.size, self.size):
yield (half, y), (self.w - half, y)
def col_lines(self):
half = self.half_size
for x in range(half, self.w - half + self.size, self.size):
yield (x, half), (x, self.h - half)
def draw_background(self):
rect = pygame.Rect(0, 0, self.w, self.h)
pygame.draw.rect(self.screen, Colors.BROWN, rect)
def draw_lines(self):
lines = itertools.chain(self.col_lines(), self.row_lines())
for start, end in lines:
pygame.draw.line(
self.screen,
Colors.BLACK,
start,
end,
width=2
)
def draw_board(self):
self.draw_background()
self.draw_lines()
def draw_piece(self, row, col):
player = self.board.last_player()
circle_pos = (
col * self.size + self.half_size,
row * self.size + self.half_size,
)
pygame.draw.circle(
self.screen,
self.player_colors[player],
circle_pos,
self.piece_size
)
def show_outcome(self):
player = self.player_names[self.board.last_player()]
msg = "draw!" if self.board.is_draw() else f"{player} wins!"
font_size = self.w // 10
font = pygame.font.Font("freesansbold.ttf", font_size)
label = font.render(msg, True, Colors.WHITE, Colors.BLACK)
x = self.w // 2 - label.get_width() // 2
y = self.h // 2 - label.get_height() // 2
self.screen.blit(label, (x, y))
def exit_on_click(self):
while True:
for event in pygame.event.get():
if (event.type == pygame.QUIT or
event.type == pygame.MOUSEBUTTONDOWN):
pygame.quit()
return
def make_move(self, x, y):
col = x // self.size
row = y // self.size
if self.board.move(row, col):
self.draw_piece(row, col)
def play(self):
pygame.time.Clock().tick(10)
self.draw_board()
pygame.display.update()
while not self.board.just_won() and not self.board.is_draw():
for event in pygame.event.get():
if event.type == pygame.QUIT:
pygame.quit()
return
elif event.type == pygame.MOUSEBUTTONDOWN:
self.make_move(*event.pos)
pygame.display.update()
self.show_outcome()
pygame.display.update()
self.exit_on_click()
if __name__ == "__main__":
game = Gomoku(rows=5, cols=5, n_to_win=4)
game.play()