I've been learning to code in my spare time for a couple of years. I have covered object oriented stuff before and can usually understand other people's object oriented code. This is the first time I've written an object oriented program from scratch.
It has all the functionality that I want and (hopefully) no mistakes - I have stress tested it.
However, I still feel the code is a bit dirty, so I'd like some suggested improvements. Some areas that I feel I've gone wrong:
1, I have put too much functionality into the board class and not enough into the player class. Is this an issue?
2, It was relatively streamlined to begin with, but then I started adding stuff to "make it look cool" such as a coin toss to decide who goes first and drawing the winning line on the board/displaying a victory message. This is all great, but my code seems to have gotten less clean as the game loop function is a bit messy - any recommendations for how to repackage it?
3, Any other comments?
4, Any recommendations for a next project that would be slightly harder but still manageable for someone of my level.
My code:
import random
import pygame
import itertools
# Initailize PyGame object
pygame.init()
DISPLAY_WIDTH = 600
DISPLAY_HEIGHT = 600
BLOCK_SIZE = 200
# Defining colors (rgb values)
BACKGROUND_COLOR = (0, 0, 0)
white = (255, 255, 255)
black = (0, 0, 0)
red = (255, 0, 0)
blue = (0, 0, 255)
green = (0, 255, 0)
pink = (255, 20, 147)
cyan = (0, 255, 255)
gray = (128, 128, 128)
# Draw black rectangle to cover square numbers once played
def draw_rect(x, y, color = black, size=30, width = 0):
pygame.draw.polygon(gameDisplay, color, [(x-size, y-size), (x-size, y+size), (x+size, y+size), (x+size, y-size)], width)
def draw_wide_rect(x, y, color = black, size=30, width = 0):
pygame.draw.polygon(gameDisplay, color, [(x-3*size, y-2*size), (x-3*size, y+2*size), (x+3*size, y+2*size), (x+3*size, y-2*size)], width)
# How to draw naughts and crosses
def draw_naught(x, y, color, radius = 40, thickness = 20):
pygame.draw.circle(gameDisplay, color, (x,y), radius, thickness)
def draw_cross(x, y, color, closed = False, thickness = 20):
pygame.draw.lines(gameDisplay, color, closed, [(x-30, y-30), (x+30, y+30)], thickness)
pygame.draw.lines(gameDisplay, color, closed , [(x+30, y-30), (x-30, y+30)], thickness)
largefont = pygame.font.SysFont(None, 80)
player_font = pygame.font.SysFont('Comic Sans MS', 40)
def display_text(text, x, y, color):
text = largefont.render("{}".format(str(text)), True, color)
gameDisplay.blit(text, [x, y])
# Set up the display
gameDisplay = pygame.display.set_mode((DISPLAY_WIDTH, DISPLAY_HEIGHT + 100))
pygame.display.set_caption("Naughts and Crosses")
clock = pygame.time.Clock()
# Draw the background grid
def drawGrid(w, rows, surface):
sizeBtwn = w // rows
x = 0
y = 0
pygame.draw.line(gameDisplay, white, (x,0),(x,w), 10)
pygame.draw.line(gameDisplay, white, (0,y),(w,y), 10)
for l in range(rows):
x = x + sizeBtwn
y = y + sizeBtwn
pygame.draw.line(gameDisplay, white, (x,0),(x,w), 10)
pygame.draw.line(gameDisplay, white, (0,y),(w,y), 10)
class Board:
def __init__(self, current_player, next_player, naught_list = [], cross_list = []):
self.naught_list = naught_list
self.cross_list = cross_list
self.empty_list = [1,2,3,4,5,6,7,8,9]
self.current_player = current_player
self.next_player = next_player
self.naught_trebles = []
self.naught_treble_sums = []
self.cross_trebles = []
self.cross_treble_sums = []
self.magic_dic = {1: 6,
2: 1,
3: 8,
4: 7,
5: 5,
6: 3,
7: 2,
8: 9,
9: 4}
def create_board(self):
gameDisplay.fill(BACKGROUND_COLOR)
drawGrid(DISPLAY_WIDTH, int(DISPLAY_WIDTH / BLOCK_SIZE), gameDisplay)
for i in range(9):
display_text(str(i+1), ((i)%3)*BLOCK_SIZE + BLOCK_SIZE/2.5, (i//3)*BLOCK_SIZE + BLOCK_SIZE/2.5, white)
def cover_text(self, square_num):
x = int((square_num%3)*BLOCK_SIZE + BLOCK_SIZE/2)
y = int((square_num//3)*BLOCK_SIZE + BLOCK_SIZE/2)
def play_move(self, marker, square_num, player):
if player.marker == 'naughts':
if int(square_num) in self.empty_list:
idx = square_num - 1 # user numbering starts at 1 but Python starts at 0
x = int((idx%3)*BLOCK_SIZE + BLOCK_SIZE/2)
y = int((idx//3)*BLOCK_SIZE + BLOCK_SIZE/2)
draw_rect(x, y)
draw_naught(x, y, player.color)
self.empty_list.remove(square_num)
self.naught_list.append(square_num)
tmp = player
self.current_player = self.next_player
self.next_player = tmp
else:
print("Invalid move - square already taken. Pick again please:")
return 0
elif player.marker == 'crosses':
if int(square_num) in self.empty_list:
idx = square_num - 1 # user numbering starts at 1 but Python starts at 0
x = int((idx%3)*BLOCK_SIZE + BLOCK_SIZE/2)
y = int((idx//3)*BLOCK_SIZE + BLOCK_SIZE/2)
draw_rect(x, y)
draw_cross(x, y, player.color)
self.empty_list.remove(square_num)
self.cross_list.append(square_num)
tmp = player
self.current_player = self.next_player
self.next_player = tmp
else:
print("Invalid move - square already taken. Pick again please:")
return 0
else:
print("Invalid marker for this player.")
def victory_line(self, winning_list, color, closed = False, thickness = 20):
# get the square numbers of the winning line. These will be in terms of magic square from magic_dic
magic_list_0 = winning_list[0]
magic_list_1 = winning_list[1]
magic_list_2 = winning_list[2]
# convert them back to the square numbers we see on the screen
for key in self.magic_dic:
if magic_list_0 == self.magic_dic[key]:
list_0 = key
for key in self.magic_dic:
if magic_list_1 == self.magic_dic[key]:
list_1 = key
for key in self.magic_dic:
if magic_list_2 == self.magic_dic[key]:
list_2 = key
non_magic_list = [list_0, list_1, list_2]
# subtract 1 as pygame numbers squares from 0 and we number from 1
left_idx = min(non_magic_list) - 1
right_idx = max(non_magic_list) -1
# get coords
left_x = int((left_idx%3)*BLOCK_SIZE + BLOCK_SIZE/2)
left_y = int((left_idx//3)*BLOCK_SIZE + BLOCK_SIZE/2)
right_x = int((right_idx%3)*BLOCK_SIZE + BLOCK_SIZE/2)
right_y = int((right_idx//3)*BLOCK_SIZE + BLOCK_SIZE/2)
# draw a line to show winning combination
pygame.draw.lines(gameDisplay, color, closed , [(left_x, left_y), (right_x, right_y)], thickness)
def magic_square_winner_check(self, player):
# magic square has every row, column and diagonal sum to 15 meaning we can just check if any triple of used squares sums to 15
# this won't be computationally challenging due to small board size. at most 5C3 = 10 combinations (no player can occupy more than 5 squares)
# current numbering is not magic so we must first use a dictionary to magic-ify the board
if player.marker == 'naughts':
updated_naught_list = [self.magic_dic[x] for x in self.naught_list]
self.naught_trebles = list(itertools.combinations(updated_naught_list, 3))
self.naught_treble_sums = [sum(x) for x in self.naught_trebles]
if 15 in self.naught_treble_sums:
print("Victory")
return 1
elif player.marker == 'crosses':
updated_cross_list = [self.magic_dic[x] for x in self.cross_list]
self.cross_trebles = list(itertools.combinations(updated_cross_list, 3))
self.cross_treble_sums = [sum(x) for x in self.cross_trebles]
if 15 in self.cross_treble_sums:
print("Victory")
return 1
else:
return 0
def victory_message(self, msg, x = DISPLAY_WIDTH//5, y=DISPLAY_HEIGHT+20, color = cyan):
text = largefont.render("{} wins!".format(str(msg)), True, color)
gameDisplay.blit(text, [x, y])
def draw_message(self, x = DISPLAY_WIDTH//6, y=DISPLAY_HEIGHT+20, color = cyan):
text = largefont.render("A boring draw...", True, color)
gameDisplay.blit(text, [x, y])
def player1_text(self, color, x = 10, y=DISPLAY_HEIGHT+30):
text = player_font.render("Player 1", True, color)
gameDisplay.blit(text, [x, y])
def player2_text(self, color, x = DISPLAY_WIDTH - 160, y= DISPLAY_HEIGHT+30):
text = player_font.render("Player 2", True, color)
gameDisplay.blit(text, [x, y])
class Player:
def __init__(self, marker, coin_toss_victor, color, name):
self.marker = marker # naughts or crosses
self.coin_toss_victor = coin_toss_victor #boolean indication of who starts
self.color = color
self.name = name
pass
def request_move(self, board):
move = input("Choose a square: ")
tmp = move
while move.isnumeric() == False:
move = input("{} is not an int! Choose again: ".format(tmp))
move = int(move)
# check valid
while move not in range(1,10):
tmp = move
move = int(input("{} is not a valid square. Please choose again: ".format(tmp)))
while move not in board.empty_list:
#board.square_taken_message()
tmp = move
move = int(input("Square {} is already taken. Please choose again: ".format(tmp)))
return move
"""<<<GAME LOOP>>>"""
def game_loop():
# display screen asking for coin fkip guess
gameDisplay.fill(BACKGROUND_COLOR)
myfont = pygame.font.SysFont('Comic Sans MS', 50)
textsurface = myfont.render("Heads or Tails?", True, white)
gameDisplay.blit(textsurface, [60, 10])
pygame.display.update()
# receive coin guess
coin = ["H", "T"]
coin_toss = input("We will decide who goes first with a coin flip. Player 1 pease enter H or T: ")
if coin_toss not in coin:
coin_toss = input("Not a valid input. Please enter H or T: ")
# dictionary for translating input to display words
coin_dic = {"H" : "Heads", "T": "Tails"}
# random toss. display the guess now. display the outcome after images
random_toss = random.choice(coin)
myfont = pygame.font.SysFont('Comic Sans MS', 50)
textsurface2 = myfont.render("Your choice: {}".format(coin_dic[coin_toss]), True, white)
gameDisplay.blit(textsurface2, [60, DISPLAY_HEIGHT - 140])
# load two images of coin
coin_a = pygame.image.load('coin_a.png')
coin_b = pygame.image.load('coin_b.png')
tosser = 0
textsurface3 = myfont.render("Result: {}".format(coin_dic[random_toss]), True, white)
if coin_toss == random_toss:
textsurface4 = myfont.render("Win! You go first!", True, white)
else:
textsurface4 = myfont.render("Lose! You go second!", True, white)
# change image 20 times for nice effect. display outcome of coin flip after 10 times.
while tosser < 21:
print(20 - tosser)
gameDisplay.blit(coin_a, (60,DISPLAY_HEIGHT//3))
pygame.display.update()
pygame.time.wait(100)
gameDisplay.blit(coin_b, (60, DISPLAY_HEIGHT//3))
pygame.display.update()
pygame.time.wait(100)
tosser += 1
if tosser > 10:
gameDisplay.blit(textsurface3, [60, DISPLAY_HEIGHT - 70])
gameDisplay.blit(textsurface4, [60, DISPLAY_HEIGHT])
pygame.display.update()
# short pause for user to read result before switching screen to tic-tac-toe
pygame.time.wait(500)
# instantiate the player objects according to the result of the coin toss
while coin_toss not in coin:
coin_toss = input("You didn't enter a valid input! Try again please: ")
if coin_toss == random_toss:
print("Coin toss result was {}".format(random_toss))
print("Player 1 to go first!")
p1 = Player('naughts', True, pink, 'Player 1')
p2 = Player('crosses', False, green, 'Player 2')
current_player = p1
next_player = p2
else:
print("Coin toss result was {}".format(random_toss))
print("Player 2 to go first!")
p1 = Player('naughts', False, pink, 'Player 1')
p2 = Player('crosses', True, green, 'Player 2')
current_player = p2
next_player = p1
# instantiate the board and display it
b = Board(current_player, next_player)
b.create_board()
# display current player
if current_player == p1:
b.player1_text(white)
b.player2_text(gray)
elif current_player == p2:
b.player1_text(gray)
b.player2_text(white)
pygame.display.update()
# begin while loop - initialise all variables here so they're reset if we restart the game
play_again = True
end = False
b.empty_list = [1,2,3,4,5,6,7,8,9]
b.naught_list = []
b.naught_trebles = []
b.naught_treble_sums = []
b.cross_list = []
b.cross_trebles = []
b.cross_treble_sums = []
while play_again:
# display current player
if current_player == p1:
b.player1_text(white)
b.player2_text(gray)
pygame.display.update()
elif current_player == p2:
b.player1_text(gray)
b.player2_text(white)
pygame.display.update()
# allow window to be quit
for event in pygame.event.get():
if event.type == pygame.QUIT:
play_again = False
# print useful stuff to terminal
print("-----Empty Squares-----")
print(b.empty_list)
print("-----Naughts-----")
print(b.naught_list)
print(b.naught_trebles)
print(b.naught_treble_sums)
print("-----Crosses-----")
print(b.cross_list)
print(b.cross_trebles)
print(b.cross_treble_sums)
# request player move and display it
move = current_player.request_move(b)
b.play_move(current_player.marker, move, current_player)
# display current player
if current_player == p1:
b.player1_text(white)
b.player2_text(gray)
pygame.display.update()
elif current_player == p2:
b.player1_text(gray)
b.player2_text(white)
pygame.display.update()
# check if current player has won and then switch player
if current_player == p1 and end == False:
if b.magic_square_winner_check(p1) == 1:
draw_wide_rect(70, DISPLAY_HEIGHT+70)
draw_wide_rect(DISPLAY_WIDTH - 80, DISPLAY_HEIGHT+70)
b.victory_message(current_player.name)
winning_idx = b.naught_treble_sums.index(15)
winning_tuple = b.naught_trebles[winning_idx]
winning_list = [x for x in winning_tuple]
print(winning_list)
b.victory_line(winning_list, p1.color)
pygame.display.update()
end = True
else:
current_player = p2
elif current_player == p2 and end == False:
if b.magic_square_winner_check(p2) == 1:
draw_wide_rect(70, DISPLAY_HEIGHT+70)
draw_wide_rect(DISPLAY_WIDTH - 80, DISPLAY_HEIGHT+70)
b.victory_message(current_player.name)
winning_idx = b.cross_treble_sums.index(15)
winning_tuple = b.cross_trebles[winning_idx]
winning_list = [x for x in winning_tuple]
b.victory_line(winning_list, p2.color)
pygame.display.update()
end = True
else:
current_player = p1
# check if game is a draw
if len(b.empty_list) == 0 and end == False:
b.draw_message()
pygame.display.update()
end = True
# if game reaches a terminal state, return 1 (used as code in main loop to ask about restart)
if end:
return 1
pygame.display.update()
def main():
end_msg = game_loop()
while end_msg == True:
play_again = int(input("Play again? 1 = Yes, 0 = No : "))
if play_again == 1:
game_loop()
if play_again == 0:
break
if __name__ == '__main__':
main()
1 Answer 1
UX
When I run the code in my shell, a GUI window immediately pops up, and it is in front of my shell. This means I can't see the the input prompts in my shell until I move the GUI window out of the way.
It is confusing for the user to interact with both a GUI and the shell. Consider one or the other. Since it is nice to have the GUI, perhaps the prompts can be added to the GUI instead of the shell.
Input
Since the code calls input
several times, it is worth considering using
pyinputplus. This offers many
features for validating user input.
It is convenient that you allow the user to just type a a single letter (H or T) at the prompt:
coin_toss = input("We will decide who goes first with a coin flip. Player 1 pease enter H or T: ")
However, it would be even more convenient if you accepted case-insensitive input
(h or t) using the upper
function:
coin_toss = input("We will decide who goes first with a coin flip. Player 1 please enter H or T: ").upper()
Note that I fixed the typo "pease".
Layout
You should add blank lines after each function
def draw_rect(x, y, color = black, size=30, width = 0):
pygame.draw.polygon(gameDisplay, color, [(x-size, y-size), (x-size, y+size), (x+size, y+size), (x+size, y-size)], width)
def draw_wide_rect(x, y, color = black, size=30, width = 0):
For example:
def draw_rect(x, y, color = black, size=30, width = 0):
pygame.draw.polygon(gameDisplay, color, [(x-size, y-size), (x-size, y+size), (x+size, y+size), (x+size, y-size)], width)
def draw_wide_rect(x, y, color = black, size=30, width = 0):
Move the classes to the top after the import
lines. Also move the other functions
after the classes.
Having them in the middle of the code interrupts the natural flow of the
code (from a human readability standpoint).
Documentation
The PEP 8 style guide recommends adding docstrings for classes and functions. You can replace your comments with docstrings:
def draw_rect(x, y, color = black, size=30, width = 0):
""" Draw black rectangle to cover square numbers once played """
Simpler
You can use f-strings to simplify the code:
move = int(input(f"{tmp} is not a valid square. Please choose again: "))
In the play_move
function, there is no need for the tmp
variable:
tmp = player
self.current_player = self.next_player
self.next_player = tmp
This is simpler:
self.current_player = self.next_player
self.next_player = player
The same can be done in the request_move
function.
This line:
b.empty_list = [1,2,3,4,5,6,7,8,9]
is simpler using range
:
b.empty_list = list(range(1, 10))
Since there are only 2 players, all the if/elif
statements like this:
if player.marker == 'naughts':
elif player.marker == 'crosses':
can be simplified as if/else
:
if player.marker == 'naughts':
else:
Partitioning
1, I have put too much functionality into the board class and not enough into the player class. Is this an issue?
It is not an issue, but there may be an opportunity to simplify the code here.
Naming
The PEP 8 style guide recommends snake_case for function and variable names.
For example, the function drawGrid
would be draw_grid
.
Also, lower-case l
is a bad name for a variable because it is easily confused
with the number 1
, not to mention that it conveys little meaning:
for l in range(rows):
Since you don't even use the variable, it is better to use the _
placeholder instead:
for _ in range(rows):
While we're looking at this function, lines like this:
x = x + sizeBtwn
are simpler using the special assignment operators:
x += sizeBtwn
Tools
You could run code development tools to automatically find some style issues with your code.
ruff
identifies a few that you can address.
Explore related questions
See similar questions with these tags.