I'm brand-new (just joined today) to code review and I am super excited to share my tic tac toe game code with all of you! Please let me know what I could fix up in my program to make it more efficient. Thanks in advance!
P.S. I'm also not sure how to add an actual board to this game (an actual board rather than the way I have done here)
Code:
import time
# Initialize board
board = {1: ' ', 2: ' ', 3: ' ',
4: ' ', 5: ' ', 6: ' ',
7: ' ', 8: ' ', 9: ' '}
# Initialize variables
count = 0 # counter to track number of filled slots
winner = False # boolean to check if anyone won
play = True # boolen to check if the game should continue
tie = False # boolean to check if there is a tie
curr_player = '' # variable to store current player identifier
player_details = [] # list to store player identifier and marker
# Helper functions
def get_player_details(curr_player):
"""Function to get player identifier and marker"""
if curr_player == 'A':
return ['B', 'O']
else:
return ['A', 'X']
def print_board():
"""Function to print the board"""
for i in board:
print(i, ':', board[i], ' ', end='')
if i % 3 == 0:
print()
def win_game(marker, player_id):
"""Function to check for winning combination"""
if board[1] == marker and board[2] == marker and board[3] == marker or \
board[4] == marker and board[5] == marker and board[6] == marker or \
board[7] == marker and board[8] == marker and board[9] == marker or \
board[1] == marker and board[4] == marker and board[7] == marker or \
board[2] == marker and board[5] == marker and board[8] == marker or \
board[3] == marker and board[6] == marker and board[9] == marker or \
board[1] == marker and board[5] == marker and board[9] == marker or \
board[3] == marker and board[5] == marker and board[7] == marker:
print_board()
time.sleep(1)
print("Player", player_id, "wins!")
return True
else:
return False
def insert_input(slot_num, marker):
"""Function for capturing user inputs"""
while board[slot_num] != ' ':
print("spot taken, pick another no.")
slot_num = int(input())
board[slot_num] = marker
def play_again():
"""Function to check if player wants to play again"""
print("Do you want to play again?")
play_again = input()
if play_again.upper() == 'Y':
for z in board:
board[z] = ' '
return True
else:
print("Thanks for playing. See you next time!")
return False
# Main program
while play:
print_board()
player_details = get_player_details(curr_player)
curr_player = player_details[0]
print("Player {}: Enter a number between 1 and 9".format(curr_player))
input_slot = int(input())
# Inserting 'X' or 'O' in desired spot
insert_input(input_slot, player_details[1])
count += 1
# Check if anybody won
winner = win_game(player_details[1], curr_player)
if count == 9 and not winner:
print("It's a tie!!")
tie = True
print_board()
# Check if players want to play again
if winner or tie:
play = play_again()
if play:
curr_player = ''
count = 0
-
\$\begingroup\$ Most of the points I would love to review can be found here do check it out \$\endgroup\$theProgrammer– theProgrammer2020年12月02日 18:52:52 +00:00Commented Dec 2, 2020 at 18:52
3 Answers 3
Your board
dictionary can be simplified from
board = {1: ' ', 2: ' ', 3: ' ',
4: ' ', 5: ' ', 6: ' ',
7: ' ', 8: ' ', 9: ' '}
to a dict
comprehension:
board = {n: ' ' for n in range(1, 10)}
Your win_game
function can be greatly simplified, from
def win_game(marker, player_id):
"""Function to check for winning combination"""
if board[1] == marker and board[2] == marker and board[3] == marker or \
board[4] == marker and board[5] == marker and board[6] == marker or \
board[7] == marker and board[8] == marker and board[9] == marker or \
board[1] == marker and board[4] == marker and board[7] == marker or \
board[2] == marker and board[5] == marker and board[8] == marker or \
board[3] == marker and board[6] == marker and board[9] == marker or \
board[1] == marker and board[5] == marker and board[9] == marker or \
board[3] == marker and board[5] == marker and board[7] == marker:
print_board()
time.sleep(1)
print("Player", player_id, "wins!")
return True
else:
return False
to
def win_game(marker, player_id):
"""Function to check for winning combination"""
if board[1] == board[2] == board[3] == marker or \
board[4] == board[5] == board[6] == marker or \
board[7] == board[8] == board[9] == marker or \
board[1] == board[4] == board[7] == marker or \
board[2] == board[5] == board[8] == marker or \
board[3] == board[6] == board[9] == marker or \
board[1] == board[5] == board[9] == marker or \
board[3] == board[5] == board[7] == marker:
print_board()
time.sleep(1)
print("Player", player_id, "wins!")
return True
return False
You see, the else
statement is not necessary, as the return
statement will make the program jump out of the function. Also, as you can probably tell,
a == 1 and b == 1 and c == 1
is the same as
a == b == c === 1
The unnecessary else
statement applies to your other functions as well
For get_player_details
:
def get_player_details(curr_player):
"""Function to get player identifier and marker"""
if curr_player == 'A':
return ['B', 'O']
else:
return ['A', 'X']
to
def get_player_details(curr_player):
"""Function to get player identifier and marker"""
if curr_player == 'A':
return ['B', 'O']
return ['A', 'X']
For play_again
:
def play_again():
"""Function to check if player wants to play again"""
print("Do you want to play again?")
play_again = input()
if play_again.upper() == 'Y':
for z in board:
board[z] = ' '
return True
else:
print("Thanks for playing. See you next time!")
return False
to
def play_again():
"""Function to check if player wants to play again"""
print("Do you want to play again?")
play_again = input()
if play_again.upper() == 'Y':
for z in board:
board[z] = ' '
return True
print("Thanks for playing. See you next time!")
return False
In your print_board
function, you can increase the readability of your print statement with a formatted string. Using not
instead of == 0
is considered more pythonic:
def print_board():
"""Function to print the board"""
for i in board:
print(i, ':', board[i], ' ', end='')
if i % 3 == 0:
print()
to
def print_board():
"""Function to print the board"""
for i in board:
print(f'{i} : {board[i]} ', end='')
if not i % 3:
print()
-
\$\begingroup\$ thanks. this is exactly the kind of help I was looking for! \$\endgroup\$fast_and_curious– fast_and_curious2020年12月02日 17:15:02 +00:00Commented Dec 2, 2020 at 17:15
-
\$\begingroup\$ @Chocolate Maybe this is better:
return ['B', 'O'] if curr_player == 'A' else ['A', 'X']
\$\endgroup\$Josh– Josh2020年12月03日 09:53:38 +00:00Commented Dec 3, 2020 at 9:53 -
\$\begingroup\$ While removing such
else
s, there's an argument to keep them for better code readability. This is purely a personal preference, though. \$\endgroup\$iBug– iBug2020年12月03日 17:20:14 +00:00Commented Dec 3, 2020 at 17:20
It's good to separate concerns.
play_again()
currently does all of the following:
- collects input.
- decides if it is 'Y' or not 'Y'.
- clears the board
- displays a goodbye
- tells the caller whether the player wants to restart.
Meanwhile, the caller (the main loop):
- decides whether to continue (the while loop)
- resets the current player
I would simplify play_again()
to just collect input and return whether it is a Yes or No. (I would make it handle "y" as well as "Y", and maybe loop back and ask again if they don't give an answer that it Y, y, N, n, Q, or q.)
I would move the code to clear the board and reset the current player so they were next to each other, and not in play_again()
.
Perhaps later, as you understand the concepts of Object Orientation, I would move much of the code into a Board
class - it would understand win conditions, how to represent the board as a string, and how to reset the board (or maybe main() would just throw away the instance and construct a new one), without dealing with any of the input or output.
P.S. I'm also not sure how to add an actual board to this game (an actual board rather than the way I have done here)
If that means to have a graphical user interface, rather than the pure text, then it isn't possible just with Python's standard libraries. You would need to find one of the many GUI frameworks that work with Python.
-
\$\begingroup\$ Thank you for this, will make sure to use your suggestions! \$\endgroup\$fast_and_curious– fast_and_curious2020年12月03日 15:21:05 +00:00Commented Dec 3, 2020 at 15:21
All this looking up markers and identifiers calls out for simplification. Just call the players 'X' and 'O' rather than 'A' and 'B'.
You don't need to check all the possible win conditions; if there is a win, it must have involved the current play.
Much of your booleans are unnecessary; rather than setting the boolean to the result of a function, then exiting if True, just exit if the function returns True. Then move the win announcement out of the function checking for a win.
The function name win_game
is written as verb, when it's actually an interrogative. check_win
is clearer.
The line print("Player", player_id, "wins!")
doesn't put spaces in. It will print PlayerAwins!
or PlayerBwins!
.
def print_board():
"""Function to print the board"""
for i in board:
print(i, ':', board[i], ' ', end='')
if i % 3 == 0:
print()
def check_win(player, move):
"""Function to check for winning combination"""
for vertical in range(4):
if board[(3*vertical + move - 1)%9+1] != player:
break
if vertical == 3:
return True
for horizontal in range(4):
square = move + horizontal
if (square-1)//3 > (move-1)//3:
square = square - 3
if board[square] != player:
break
if horizontal == 3:
return True
#if you don't understand what's going on in the
# above lines, work through a few cases
if ( (board[1] == board[5] == board[9] == player) or
(board[3] == board[5] == board[7] == player) ):
return True
return False
def insert_input(slot_num, player):
"""Function for capturing user inputs"""
while board[slot_num] != ' ':
print("spot taken, pick another no.")
slot_num = int(input())
board[slot_num] = player
def play_again():
"""Function to check if player wants to play again"""
print("Do you want to play again?")
play_again = input()
if play_again.upper() == 'Y':
for z in board:
board[z] = ' '
return True
else:
print("Thanks for playing. See you next time!")
return False
# Main program
count = 0
while True:
count +=1
print_board()
print("Player {}: Enter a number between 1 and 9".format(curr_player))
input_slot = int(input())
# Inserting 'X' or 'O' in desired spot
insert_input(input_slot, player)
# Check if anybody won
if check_win(player, move):
#If you really want to keep the names A and B,
#insert the following line:
#player = {'O':'B', 'X','A'}[player]
print_board()
time.sleep(1)
print("Player ", player, " wins!")
if play_again():
curr_player = 'X'
count = 0
else break
if count == 9:
print("It's a tie!!")
print_board()
if play_again():
curr_player = 'X'
count = 0
else break
-
\$\begingroup\$ Thanks @Acccumulation for your insight on this! Will make sure to use your suggestions as well 😊 \$\endgroup\$fast_and_curious– fast_and_curious2020年12月03日 15:26:49 +00:00Commented Dec 3, 2020 at 15:26
Explore related questions
See similar questions with these tags.