Code Review Asked by Alaa mihoub on October 27, 2021
I just finished coding a Tic Tac Toe game for my first Python project. I need some advice on things I can improve; please help. Everything seems to be running correctly. I used many functions. In the comments you can know what functions do; there is board initialization and prints the board out to the console checks the input and updates the board according to the user’s decision, function analyzes the board status in order to check the player using ‘O’s or ‘X’s has won the game and a function draws the computer’s move and updates the board.
from random import randrange
result = False
board=[[1,2,3],[4,'X',6],[7,8,9]]
#
#board initialization always first move of computer is in the middle
def DisplayBoard(board):
for j in range(4):
for i in range(4):
print("+",end='')
if i==3:
break
for i in range(7):
print("-",end='')
if j==3:
break
print()
for d in range (3):
for r in range(4):
print("|",end='')
if r==3:
break
for i in range(7):
if d==1 and i==3:
print(board[j][r],end='')
else:
print(" ",end='')
print()
print()
#
# the function accepts one parameter containing the board's current status
# and prints it out to the console
#
def EnterMove(board):
entredMove=int((input("Enter your move: ")))
while not any(entredMove in i for i in board):
print ("this value is wrong")
entredMove=int((input("Enter your move: ")))
for i in range(3):
for j in range(3):
if int(entredMove)==board[i][j]:
board[i][j]= 'O'
#
# the function accepts the board current status, asks the user about their move,
# checks the input and updates the board according to the user's decision
#
def MakeListOfFreeFields(board):
freeFields=[]
s=0
global result
for i in range(3):
for j in range(3):
if type(board[i][j])== int:
freeFields.append((i,j))
s+=1
if s==0 and result==False:
result = True
print ("it is a DRAW")
# the function browses the board and builds a list of all the free squares;
# the list consists of tuples, while each tuple is a pair of row and column numbers
# and test if list is empty that means it is a draw
def VictoryFor(board, sign):
global result
xxx=0
xxxx=0
for i in range(3):
x=0
for j in range(3):
if board[i][j]==sign:
x+=1
if x==3:
print(sign,' is won the game')
result=True
if result == True:
break
xx=0
for j in range(3):
if board[j][i]==sign:
xx+=1
if xx==3:
print(sign,' is won the game')
result=True
if result == True:
break
for j in range(3):
if i==j and board[i][j]==sign:
xxx+=1
if xxx==3:
print(sign,' is won the game')
result=True
if result ==True:
break
for j in range(3):
if i+j==2 and board[i][j]==sign:
xxxx+=1
if xxxx==3:
print(sign,' is won the game')
result=True
if result ==True:
break
#
# the function analyzes the board status in order to check if
# the player using 'O's or 'X's has won the game
#
def DrawMove(board):
entredMove=randrange(8)+1
while not any(entredMove in i for i in board):
entredMove=randrange(9)+1
for i in range(3):
for j in range(3):
if int(entredMove)==board[i][j]:
print('computer move in ',entredMove)
board[i][j]= 'X'
#
# the function draws the computer's move and updates the board
#
DisplayBoard(board)
while result == False:
EnterMove(board)
DisplayBoard(board)
VictoryFor(board, 'O')
if result == False:
DrawMove(board)
VictoryFor(board, 'X')
DisplayBoard(board)
MakeListOfFreeFields(board)
Welcome to the community, my first post was also a tictactoe, allthough in a functional Style in Scala.
So first some general rules / suggestions, then later I will go into the details. If I am wrong on anything then please feel free to correct me. I am more experienced in Java and allthough I have done some projects in Python I still might be ignorant about how things are different.
So first I would make play_game() function, so you have a clean interface to start your game. You just put everything you have in the bottom in it, and place it at the top, so it's more readable.
Ok on the VictoryFor() function:
The general pattern is pretty interesting. I wouldn't have thought about the way you solved it kind of algorhitmically.
Personally I solved this by defining a set of sets of all win patterns, then check if the values in those indexes are all the same. I used a flat array, you used a map, so if you want to try implementing it you need to change that. (Copied from scala but the idea is the same).
val patterns: Set[Set[Int]] = Set(
Set(0, 1, 2),
Set(3, 4, 5),
Set(6, 7, 8),
Set(0, 3, 6),
Set(1, 4, 7),
Set(2, 5, 8),
Set(0, 4, 8),
Set(2, 4, 6)
)
Now back to your implementation and some suggestions. You can represent the string in a different format. Just some syntactic sugar.
print(sign,' is won the game')
#changed to
print(f'{sign} has won the game')
To make your intent more clear you could split the loops up in separate functions. def check_hor_winner, def check_vert_winner, def check_diag_winner
Also I would rename sign to player.
If you changed victoryFor, so that it returns true or false, then you can remove those result = True and breaks, and just return True.
Here is the final changed VictoryFor function in your algorhitmic Style. Especially in the diagonal functions I would have probably just put in the hardcoded patterns but if you would make a 100x100 ticTacToe then it would make sense.
def has_won(board, player):
if (
has_won_vertically(board, player) or
has_won_horizontally(board, player) or
has_won_diagonal_1(board, player) or
has_won_diagonal_2(board, player)):
return True
return False
def has_won_vertically(board, player):
for row in range(3):
player_count = 0
for column in range(3):
if board[row][column] == player:
player_count += 1
if player_count == 3:
return True
return False
def has_won_horizontally(board, player):
for column in range(3):
player_count = 0
for row in range(3):
if board[row][column] == player:
player_count += 1
if player_count == 3:
return True
return False
def has_won_diagonal_1(board, player):
player_count = 0
for row in range(3):
for column in range(3):
if row == column and board[row][column] != player:
return False
return True
def has_won_diagonal_2(board, player):
player_count = 0
for row in range(3):
for column in range(3):
if row+column == 2 and board[row][column] != player:
return False
return True
Next up your MakeListOfFreeFields The function Name does not represent what it is doing. Making a List of the free fields is just an implementation Detail. What it actually is doing is checking if it is a draw. To reflect that let's rename it to is_draw, and while we're at it let's also remove the global variable result and make is_draw return True or false.
DrawMove and EnterMove can also be renamed to enter_move_player() and enter_move_computer. I'm still not satisfied completely with the names but it's more clear.
Here is the final result I made. There are still a lot of improovements possible but my time is running out. I'm open to any criticism
from random import randrange
board=[[1,2,3],[4,'X',6],[7,8,9]]
#
#board initialization always first move of computer is in the middle
def play_game():
display_board(board)
won = False
draw = False
while won == False and draw == False:
enter_move_player(board)
display_board(board)
won = has_won(board, 'O')
if won == False:
enter_move_computer(board)
won = has_won(board, 'X')
display_board(board)
draw = is_draw(board)
def display_board(board):
for j in range(4):
for i in range(4):
print("+",end='')
if i==3:
break
for i in range(7):
print("-",end='')
if j==3:
break
print()
for d in range (3):
for r in range(4):
print("|",end='')
if r==3:
break
for i in range(7):
if d==1 and i==3:
print(board[j][r],end='')
else:
print(" ",end='')
print()
print()
def enter_move_player(board):
enteredMove=int((input("Enter your move: ")))
while not any(enteredMove in i for i in board):
print ("this value is wrong")
enteredMove=int((input("Enter your move: ")))
for i in range(3):
for j in range(3):
if int(enteredMove)==board[i][j]:
board[i][j]= 'O'
def is_draw(board):
freeFields=[]
s=0
for i in range(3):
for j in range(3):
if type(board[i][j])== int:
freeFields.append((i,j))
s+=1
if s==0 and result==False:
print ("it is a DRAW")
return True
return False
def has_won(board, player):
if (
has_won_vertically(board, player) or
has_won_horizontally(board, player) or
has_won_diagonal_1(board, player) or
has_won_diagonal_2(board, player)):
return True
return False
def has_won_vertically(board, player):
for row in range(3):
player_count = 0
for column in range(3):
if board[row][column] == player:
player_count += 1
if player_count == 3:
return True
return False
def has_won_horizontally(board, player):
for column in range(3):
player_count = 0
for row in range(3):
if board[row][column] == player:
player_count += 1
if player_count == 3:
return True
return False
def has_won_diagonal_1(board, player):
player_count = 0
for row in range(3):
for column in range(3):
if row == column and board[row][column] != player:
return False
return True
def has_won_diagonal_2(board, player):
player_count = 0
for row in range(3):
for column in range(3):
if row+column == 2 and board[row][column] != player:
return False
return True
def enter_move_computer(board):
enteredMove = randrange(8)+1
while not any(enteredMove in i for i in board):
enteredMove=randrange(9)+1
for i in range(3):
for j in range(3):
if int(enteredMove)==board[i][j]:
print('computer move in ',enteredMove)
board[i][j]= 'X'
play_game()
Answered by elauser on October 27, 2021
Welcome to the community. Here are few pointers from first look at the code:
The code does not follow PEP-8 style guide. You should follow the snake_case
naming convention for variables and functions; classes follow the CamelCase
naming.
Instead of the code running as is, the execution condition should be placed inside an if __name__
block.
For comparing True
, False
, None
; instead of ==
the is
comparison check is preferred. So, instead of result == False
, it would be result is False
or just not result
.
If using python 3.6+, you can also provide type hinting.
Instead of comments around the function definitions, use docstrings.
The following print statement(s) have a redundant loop (unnecessary CPU instructions):
for i in range(7):
print("-",end='')
and can simply be:
print("-" * 7)
Answered by hjpotter92 on October 27, 2021
Get help from others!
Recent Questions
Recent Answers
© 2024 TransWikia.com. All rights reserved. Sites we Love: PCI Database, UKBizDB, Menu Kuliner, Sharing RPP