I have written a simple Tic Tac Toe game using some ASCII art. Is there anything I can do to improve upon the logic and structure of the program?
"""A basic command line tic tac toe game"""
import os
import sys
gameBoard = [['' for j in range(3)] for i in range(3)]
displayBoard = [[' ' for j in range(46)] for i in range(25)]
def main():
"""Main method to control game"""
player = 'X'#Player x to go first
moveCounter = 1 #Keeps track of how many turns have been taken
#Setup game
initGame()
printInitScreen()
while True:
#Get player input
square = input(f"Player {player}, choose your square ->")
if validateInput(square):
updateGameBoard(square, player)
updateDisplayBoard(square, player)
printDisplayBoard()
if moveCounter >= 4:
checkIfWon(player)
#Switch player
if player == 'X':
player = 'O'
else:
player = 'X'
moveCounter += 1
else:
print("Please try again")
def initGame():
"""Create and set up game components"""
#Fill board
for i in range(25):
#If on a row boarder set to _
if i == 8 or i == 17:
for j in range(46):
displayBoard[i][j] = '_'
else:
for j in range(46):
#If on column boarder set |
if j == 15 or j == 31:
displayBoard[i][j] = '|'
#Put numbers in corner of square
displayBoard[0][0] = '1'
displayBoard[0][16] = '2'
displayBoard[0][32] = '3'
displayBoard[9][0] = '4'
displayBoard[9][16] = '5'
displayBoard[9][32] = '6'
displayBoard[18][0] = '7'
displayBoard[18][16] = '8'
displayBoard[18][32] = '9'
def validateInput(input):
"""Validates user input"""
#Check given char is allowed
try:
square = int(input[0]) #Check first char of input is number
except:
return False
#Check nothing already in that square
#Get the gameBoard index of users chosen square
index = numToIndex(input)
if gameBoard[index[0]][index[1]] != '':
return False
#If all ok
return True
def updateGameBoard(input, player):
"""Keeps track of users moves"""
#Update the array with new move
index = numToIndex(input[0])
gameBoard[index[0]][index[1]] = player
def printDisplayBoard():
"""Prints a string representation of board"""
os.system('cls' if os.name == 'nt' else 'clear') # Clear screen
for row in displayBoard:
print(''.join(row))
print("")
def checkIfWon(player):
"""Checks to see if the last move won the game"""
gameWon = False
#Check Horiz
for row in gameBoard:
if row[0] == row[1] == row[2] == player:
gameWon = True
#Check Vert
for i in range(3):
if gameBoard[0][i] == gameBoard[1][i] == gameBoard[2][i] == player:
gameWon = True
#Check Diag
if gameBoard[0][0] == gameBoard[1][1] == gameBoard[2][2] == player:
gameWon = True
if gameBoard[0][2] == gameBoard[1][1] == gameBoard[2][0] == player:
gameWon = True
if gameWon:
print(f"Congratualtions player {player}, you won!")
sys.exit()
def printGameBoard():
"""For debugging, prints gameboard"""
for row in gameBoard:
print(row)
def printInitScreen():
"""Prints the initial welcome screen"""
header = """
888 d8b 888 888
888 Y8P 888 888
888 888 888
888888888 .d8888b888888 8888b. .d8888b888888 .d88b. .d88b.
888 888d88P" 888 "88bd88P" 888 d88""88bd8P Y8b
888 888888 888 .d888888888 888 888 88888888888
Y88b. 888Y88b. Y88b. 888 888Y88b. Y88b. Y88..88PY8b.
"Y888888 "Y8888P "Y888"Y888888 "Y8888P "Y888 "Y88P" "Y8888
"""
os.system('cls' if os.name == 'nt' else 'clear') # Clear screen
print(header)
input("Press enter to start!")
printDisplayBoard()
def updateDisplayBoard(num, player):
"""Add the players shape to the chosen position on display board"""
shapes = {"X":
[[' ',' ',' ',' ','?','8','8','8','8','P',' ',' ',' ',' '],
[' ',' ',' ',' ',' ','`','8','8','`',' ',' ',' ',' ',' '],
['8','b',',','_',' ',' ','8','8',' ',' ','_',',','d','8'],
['8','8','8','8','8','S','I','C','K','8','8','8','8','8'],
['8','P','~',' ',' ',' ','8','8',' ',' ',' ','~','?','8'],
[' ',' ',' ',' ',' ',',','8','8','.',' ',' ',' ',' ',' '],
[' ',' ',' ',' ','d','8','8','8','8','b',' ',' ',' ',' ']],
"O":
[[' ',' ',' ',' ',' ',' ','%','%',' ',' ',' ',' ',' ',' '],
[' ',' ',' ',' ','%','%',' ',' ','%','%',' ',' ',' ',' '],
[' ',' ','%','%',' ',' ',' ',' ',' ',' ','%','%',' ',' '],
['%','%',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ','%','%'],
[' ',' ','%','%',' ',' ',' ',' ',' ',' ','%','%',' ',' '],
[' ',' ',' ',' ','%','%',' ',' ','%','%',' ',' ',' ',' '],
[' ',' ',' ',' ',' ',' ','%','%',' ',' ',' ',' ',' ',' ']]}
shape = shapes[player]
num = int(num[0])
offsets = [[0 ,0],[0 ,16],[0 ,32],
[9 ,0],[9 ,16],[9 ,32],
[17,0],[17,16],[17,32]]
iOffset = offsets[num-1][0]
jOffset = offsets[num-1][1]
for i in range(iOffset, iOffset + 7):
for j in range(jOffset, jOffset + 14):
displayBoard[i+1][j] = shape[i - iOffset][j - jOffset]
def numToIndex(num):
"""Returns index [i,j] for given 'square' on board"""
num = int(num[0])
indexList = []
for i in range (3):
for j in range(3):
indexList.append([i,j])
return indexList[int(num)-1]
if __name__ == '__main__':
main()
1 Answer 1
gameBoard = [['' for j in range(3)] for i in range(3)]
displayBoard = [[' ' for j in range(46)] for i in range(25)]
For me, time spend aligning source code is time wasted. It takes too much time initially. Moreover, if any variable is renamed throughout the code (or cleaned up etc. etc.), the aligning will get messed up. So it actively interferes with refactoring. This is highly subjective though, and I like to look at aligned code.
gameBoard = [['' for j in range(3)] for i in range(3)]
A game board (or game board state, more precisely) consists of squares, filled with X'es or O's. What's missing is an object to represent those. For me, this is a bit too "stringly typed".
displayBoard = [[' ' for j in range(46)] for i in range(25)]
This is the start of a problem in the code: unexplained numeric literals. Why 46 or 25? Just perform the calculations and let the computer compute. Make named constants out of the calculations and then use those.
player = 'X'#Player x to go first
If you name the identifier currentPlayer
then the comment would not be necessary. Often choosing a good name will make comments unnecessary.
I'd try and avoid trailing comments as they tend to disappear on long lines. Again, this is somewhat subjective.
Use spaces before and after #
please.
updateGameBoard(square, player)
updateDisplayBoard(square, player)
I spot a design issue here. The UI should display the state of the game, captured in gameBoard
. So I would expect a printDisplayBoard(gameBoard)
instead. Currently the game logic and UI is mixed and duplicated.
if moveCounter >= 4:
checkIfWon(player)
This is not so intuitive to me. The game ends when a row, column or diagonal of three elements is formed. Currently you may have two winning players! If the board is filled without this happening then the game is a draw.
Deviating from this game logic is not a good idea as it is easy to make mistakes. Moreover, and more importantly, it fails if the application is ever extended to, for instance, a 4x4 board. Now I don't expect "tic tac toe too" to happen soon, but for actual application code, this is rather important.
My SudokuSolver was programmed as generically as possible. When I was done it literally took a minute to support differently sized and even special Sudoku's with additional diagonals and such. This shows the importance of programming as generically as possible.
def initGame():
I'd create drawHorizontalLine
and drawVertialLine
functions or similar.
displayBoard[0][0] = '1'
displayBoard[0][16] = '2'
displayBoard[0][32] = '3'
I like how you clearly mark the squares of the board. However, this also is one of the clearest examples of you doing the computing instead of the computer. It should be relatively easy to create a single for
loop and compute the x
and y
of the positions.
Basically you're buffering the image before displaying it, rather than making a single print
with spaghetti code. That's very good.
os.system('cls' if os.name == 'nt' else 'clear') # Clear screen
.
.
.
os.system('cls' if os.name == 'nt' else 'clear') # Clear screen
Repeat 10 times: "I will keep to DRY principles".
If you repeat such a line, then put the line in a function and call that. If you add another method of clearing the screen then likely one of the locations with the same line of code will be forgotten. Looking at the comment, you've already thought of a name and started typing.
Of course, there is a lot of DRY (don't repeat yourself) failures where the same code is used but with different integer values, but this one stood out.
shapes = [...]
Do you really want that variable to be assigned the entire shape all the time? That needs to be a constant or - in real code - a resource that is being read once.
num = int(num[0])
Wait, what? Why? If the reason for code is not immediately clear, then I expect a comment!
offsets = [[0 ,0],[0 ,16],[0 ,32],
[9 ,0],[9 ,16],[9 ,32],
[17,0],[17,16],[17,32]]
Which offsets would that be? End of code, must be Friday :)
def numToIndex(num):
Finally, a function that calculates things! I knew you could do it.
-
\$\begingroup\$ Hi, thank you for taking the time to read through and help me with my code. I have learnt a lot from your answer. I'm sometimes cautious of using functions where maybe I don't need to, but you've cleared some things up for me.
printDisplayBoard(gameBoard)
This especially makes perfect sense now I have seen it. \$\endgroup\$WireInTheGhost– WireInTheGhost2020年04月28日 09:44:30 +00:00Commented Apr 28, 2020 at 9:44 -
\$\begingroup\$ Some people try and create methods with just a few lines, say 5-7 (for a higher level language). That's a bit too extreme for me, but in general large functions / methods are more troublesome than shorter ones, especially if they use a lot of branching (loops, if statements and switches). This is referred to as the cyclomatic complexity of a function. Programming is basically fighting complexity. \$\endgroup\$Maarten Bodewes– Maarten Bodewes2020年04月28日 10:26:55 +00:00Commented Apr 28, 2020 at 10:26
Explore related questions
See similar questions with these tags.