I have just started learning Python. This is code for a TicTacToe game. Can you please suggest any improvements?
pos_matrix=[[' ',' ',' '],[' ',' ',' '],[' ',' ',' ']]
def my_tictactoe(pos,symbol):
if pos_matrix[pos[0]-1][pos[1]-1]!=' ':
print('Invalid input')
exit
else:
pos_matrix[pos[0]-1][pos[1]-1]=symbol
for i in range(0,3):
print(pos_matrix[i][0]+' | '+pos_matrix[i][1]+' | '+pos_matrix[i][2])
print('--|---|--')
if is_win():
print('GAME OVER. \n Player with symbol {x} wins!'.format(x=pos_matrix[pos[0]-1][pos[1]-1]))
for i in [0,1,2]:
pos_matrix[i][0]=pos_matrix[i][1]=pos_matrix[i][2]=' '
def is_win():
for i in [0,1,2]:
if pos_matrix[i][0]==pos_matrix[i][1]==pos_matrix[i][2]!=' ':
return True
elif pos_matrix[0][i]==pos_matrix[1][i]==pos_matrix[2][i]!=' ':
return True
if pos_matrix[0][0]==pos_matrix[1][1]==pos_matrix[2][2]!=' ':
return True
elif pos_matrix[0][2]==pos_matrix[1][1]==pos_matrix[2][0]!=' ':
return True
else:
return False
my_tictactoe((1,1),'o')
my_tictactoe((2,2),'x')
my_tictactoe((3,2),'o')
my_tictactoe((1,3),'x')
my_tictactoe((2,1),'o')
my_tictactoe((3,3),'x')
my_tictactoe((3,1),'o')
my_tictactoe((1,2),'x')
2 Answers 2
What if someone tries my_tictactoe((1,2),'p')?
In addition, a python thing: You can have a main function to turn this into a module of sorts.
At the end of the file, you would add:
def main():
#test game here
if __name__ == "__main__":
main()
You could put your test game in there. That way, you could call your functions from another file to use it as a library, or run it on its own as a module test.
PyLint is a useful tool for checking your code, ensuring good coding habits. For example, PyLint doesn’t like a lack of spaces around operators or after commas. Neither do my old weak eyes. Instead of this:
pos_matrix=[[' ',' ',' '],[' ',' ',' '],[' ',' ',' ']]
... code like this:
pos_matrix = [[' ', ' ', ' '], [' ', ' ', ' '], [' ', ' ', ' ']]
An if
statement that does a return
or exit
doesn’t need to be followed by an else
or elif
; if the if
statement is true, the execution path doesn’t need to skip around the else
clause, because execution has already left the function.
For clarity:
if pos_matrix[pos[0]-1][pos[1]-1]!=' ':
print('Invalid input')
exit
else:
pos_matrix[pos[0]-1][pos[1]-1]=symbol
for i in range(0,3):
# print rows of game grid
After the if-else
statement is the code to print out the game grid. It appears this code belongs to both paths through the if-else
, where as it will only execute if the False
path is taken. As such, it should be indented to belong as part of the else:
clause:
if pos_matrix[pos[0] - 1][pos[1] - 1] != ' ':
print('Invalid input')
exit
else:
pos_matrix[pos[0] - 1][pos[1] - 1] = symbol
for i in range(0,3):
# print rows of game grid
Alternately, the else:
can be omitted entirely, and the statement in the else:
clause outdented to the same level as the if
statement:
if pos_matrix[pos[0]-1][pos[1]-1]!=' ':
print('Invalid input')
exit
pos_matrix[pos[0]-1][pos[1]-1]=symbol
for i in range(0,3):
# print rows of game grid
Invalid input should not unconditionally terminate the program.
if pos_matrix[pos[0]-1][pos[1]-1]!=' ':
print('Invalid input')
exit
This code makes it harder to use in a larger program, since an invalid input will terminate the entire program.
Instead, a ValueError
exception should be raised:
if pos_matrix[pos[0] - 1][pos[1] - 1] != ' ':
raise ValueError('Invalid input')
This allows the caller to use a try ... except:
structure. The user could enter an invalid input, the function detects this and raises the error. The caller catches the error, and request the user re-enter a valid move.
Without the appropriate try ... except:
statement, raising the exception will also terminate the program.
You decode pos
using [pos[0]-1][pos[1]-1]
multiple times. This is ugly and hard to follow. Use variable names to avoid repeating the same calculations:
row, col = pos[0] - 1, pos[1] - 1
Then you can write:
if pos_matrix[row][col] != ' ':
Instead of looping over range(0, 3)
or [0, 1, 2]
, just use range(3)
:
for i in range(3):
....
def my_tictactoe(pos,symbol):
...
pos_matrix[pos[0]-1][pos[1]-1]=symbol
...
print('GAME OVER. \n Player with symbol {x} wins!'.format(x=pos_matrix[pos[0]-1][pos[1]-1]))
Why the complex expression to set x
in the format statement? You’ve just assigned symbol
to that pos_matrix entry; just use that!
print('GAME OVER. \n Player with symbol {x} wins!'.format(x=symbol))
Or better, if you are using python 3.6 or later, use an f-string:
print(f'GAME OVER. \n Player with symbol {symbol} wins!')
Your is_win
code is more complex than needed because you are checking if anyone has won, which means a row, column or diagonal has 3 of the same symbol, which is not a space. If you passed the symbol to the function, you can check if that player has won, which means checking for 3 of that symbol only, which is slightly shorter & simpler.
def is_win(symbol):
for i in range(3):
if all( pos_matrix[i][col] == symbol for col in range(3)):
return True
if all( pos_matrix[row][i] == symbol for row in range(3)):
return True
if all( pos_matrix[i][i] == symbol for i in range(3)):
return True
if all( pos_matrix[i][2 - i] == symbol for i in range(3)):
return True
return False
Who's turn is it, anyway?
You call my_tictactoe(...)
and pass in the character to place in the grid. As Hack Saw pointed out, this could be a 'p'
!
Instead, it would be better for the game to keep track of who's turn it is, and automatically use the appropriate symbol.
For example, you could set a global symbol
variable initially to 'x'
, and then have it flip between 'x'
and 'o'
after processing a valid move.
symbol = 'x'
def my_tictactoe(pos):
# ... existing validation, printing, and win detection code ...
# Alternate players: 'x' <--> 'o'
symbol = 'o' if symbol == 'x' else 'x'
As a bonus, after one player wins, the other player will automatically become the first player.
To test the program, with the automatic player tracking, (expanding on Hack Saw's suggestion a bit) the test code could become:
def main():
for pos in ((1,1), (2,2), (3,2), (1,3), (2,1), (3,3), (3,1), (1,2)):
my_tictactoe(pos)
if __name__ == "__main__":
main()
-
\$\begingroup\$ To your second point: I would still use an
else
statement even if it is not required, just to make it easier to read (as long as this doesn't add to many indentations.) This is simple because some people jump to the end of the function to see what is returns, and they then think, it always returns that, what may not be true. \$\endgroup\$MegaIng– MegaIng2018年07月08日 14:32:52 +00:00Commented Jul 8, 2018 at 14:32 -
\$\begingroup\$ @MegaIng I've clarified my point a bit. \$\endgroup\$AJNeufeld– AJNeufeld2018年07月09日 01:32:39 +00:00Commented Jul 9, 2018 at 1:32