3
\$\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')
\$\endgroup\$
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()
\$\endgroup\$
  • \$\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 Jul 8 '18 at 14:32
  • \$\begingroup\$ @MegaIng I've clarified my point a bit. \$\endgroup\$ – AJNeufeld Jul 9 '18 at 1:32
1
\$\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.

\$\endgroup\$
  • \$\begingroup\$ I didn't know about that. Thank you :) \$\endgroup\$ – Sahil Batra Jul 8 '18 at 4:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.