4
\$\begingroup\$

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')
Jamal
35.2k13 gold badges134 silver badges238 bronze badges
asked Jul 8, 2018 at 3:35
\$\endgroup\$

2 Answers 2

2
\$\begingroup\$

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.

answered Jul 8, 2018 at 4:06
\$\endgroup\$
0
2
\$\begingroup\$

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 xin the format statement? You’ve just assigned symbolto 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()
answered Jul 8, 2018 at 6:01
\$\endgroup\$
2
  • \$\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\$ Commented Jul 8, 2018 at 14:32
  • \$\begingroup\$ @MegaIng I've clarified my point a bit. \$\endgroup\$ Commented Jul 9, 2018 at 1:32

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.