6
\$\begingroup\$

I'm a beginner in Python programming, been doing it for only a few weeks and I decided to make a tic-tac-toe game in the Python interpreter. I'm looking for feedback on the program and any way I can improve it and shorten it and point out inefficiencies/errors since I've done everything I can to get rid of all errors I could think of.

I would appreciate if it was advice useful to my level and not all about high-level Python code, though I wouldn't mind some of it if its a very useful concept to use in this program and others.

Here's the code:

print("Welcome to the game of tic-tac-toe, you know how to play, so go on and have fun!")
print("Just know that the positions to input are of the form: ")
print("| 1 | 2 | 3 |")
print("| 4 | 5 | 6 |")
print("| 7 | 8 | 9 |")

p1, p2, p3, p4, p5, p6, p7, p8, p9 = " ", " ", " ", " ", " ", " ", " ", " ", " " # p stands 
#for "position", so p1 means position 1, p2 means #position 2 etc.

game_playout = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i']  # the letters are placeholders 
#until the real game starts, after which each letter #will be chronologically replaced with a 
#0 or a 1, 0 representing 'o' and 1 representing 'x'

won = 0

count = 0

x_pos = None

o_pos = None

used_positions = [ ]


def win_check():

    if game_playout[0] == game_playout[1] == game_playout[2]:
        return 1
    elif game_playout[3] == game_playout[4] == game_playout[5]:
        return 1
    elif game_playout[6] == game_playout[7] == game_playout[8]:
        return 1
    elif game_playout[0] == game_playout[3] == game_playout[6]:
        return 1
    elif game_playout[0] == game_playout[4] == game_playout[8]:
        return 1
    elif game_playout[1] == game_playout[4] == game_playout[7]:
        return 1
    elif game_playout[2] == game_playout[5] == game_playout[8]:
        return 1
    elif game_playout[2] == game_playout[4] == game_playout[6]:
        return 1


def layout_changer():
    global p1, p2, p3, p4, p5, p6, p7, p8, p9

    if x_pos == 1:
        p1 = "x"
    elif x_pos == 2:
        p2 = "x"
    elif x_pos == 3:
        p3 = "x"
    elif x_pos == 4:
        p4 = "x"
    elif x_pos == 5:
        p5 = "x"
    elif x_pos == 6:
        p6 = "x"
    elif x_pos == 7:
        p7 = "x"
    elif x_pos == 8:
        p8 = "x"
    elif x_pos == 9:
        p9 = "x"

    if o_pos == 1:
        p1 = "o"
    elif o_pos == 2:
        p2 = "o"
    elif o_pos == 3:
        p3 = "o"
    elif o_pos == 4:
        p4 = "o"
    elif o_pos == 5:
        p5 = "o"
    elif o_pos == 6:
        p6 = "o"
    elif o_pos == 7:
        p7 = "o"
    elif o_pos == 8:
        p8 = "o"
    elif o_pos == 9:
        p9 = "o"


while won != 1:

    x_pos = input("Choose your position to place x (1 to 9) ")
    while not x_pos.isnumeric():
        print("Error! Looks like you've haven't entered a number, please try again!")
        x_pos = input("Choose your position to place x (1 to 9) ")
    x_pos = int(x_pos)
    while x_pos not in [1, 2, 3, 4, 5, 6, 7, 8, 9] or x_pos in used_positions:
        print(f"| {p1} | {p2} | {p3} |")
        print(f"| {p4} | {p5} | {p6} |")
        print(f"| {p7} | {p8} | {p9} |")
        print("Invalid position! Please try another value:")
        x_pos = int(input("Choose your position to place x (1 to 9) "))

    used_positions.append(x_pos)
    count += 1
    game_playout.pop(x_pos - 1)
    game_playout.insert(x_pos - 1,
                    1)  # We replace the (x_pos-1)th index element in game_playout with 1,where 1 indicates an 'x'
    layout_changer()
    print(f"| {p1} | {p2} | {p3} |")
    print(f"| {p4} | {p5} | {p6} |")
    print(f"| {p7} | {p8} | {p9} |")

    won = win_check()
    if won == 1:
        print("x has won! Congratulations!")
        print()
        print("Thank you for playing the game! Hope you enjoyed!")
        quit()
    elif count >= 9:
        print()
        print("Draw!")
        print("Thank you for playing the game! Hope you enjoyed!")
        quit()

    o_pos = input("Choose your position to place o (1 to 9) ")
    while not o_pos.isnumeric():
        print("Error! Looks like you've haven't entered a number, please try again!")
        o_pos = input("Choose your position to place o (1 to 9) ")
    o_pos = int(o_pos)
    while o_pos not in [1, 2, 3, 4, 5, 6, 7, 8, 9] or o_pos in used_positions:
        print(f"| {p1} | {p2} | {p3} |")
        print(f"| {p4} | {p5} | {p6} |")
        print(f"| {p7} | {p8} | {p9} |")
        print("Invalid position! Please try another value:")
        o_pos = int(input("Choose your position to place o (1 to 9) "))

    used_positions.append(o_pos)
    count += 1
    game_playout.pop(o_pos - 1)
    game_playout.insert(o_pos - 1,
                    0)  # We replace the (o_pos-1)th index element in game_playout with 0,where 0 indicates an 'o'
    layout_changer()
    print(f"| {p1} | {p2} | {p3} |")
    print(f"| {p4} | {p5} | {p6} |")
    print(f"| {p7} | {p8} | {p9} |")

    won = win_check()
    if won == 1:
        print("o has won! Congratulations!")
print()

print("Thank you for playing the game! Hope you enjoyed!")
\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

Where to start…

tuple assignment

p1, p2, p3, p4, p5, p6, p7, p8, p9 = " ", " ", " ", " ", " ", " ", " ", " ", " "

Is that the correct number of " "? Did you count? Of course you did, and yes it is the correct number since tuple assignment will generate an exception if a mismatch occurs. But, you don’t need to type, or count; just use tuple multiplication/replication:

p1, p2, p3, p4, p5, p6, p7, p8, p9 = (" ",) * 9

pop/insert

This code is very obtuse and obfuscated:

    game_playout.pop(x_pos - 1)
    game_playout.insert(x_pos - 1,
                    1)  # We replace the (x_pos-1)th index element in game_playout with 1,where 1 indicates an 'x'

You are popping (removing) an element, and then inserting a new element at the same position. A lot of busy work moving elements backwards in the list for the pop, and then moving them forward again during the insert. Instead, you could simply write:

    game_playout[x_pos - 1] = 1 # Replace the (x_pos-1)th element with 1, indicating an 'x'

WET -vs- DRY

Why do you Write Everything Twice (WET)? Don’t Repeat Yourself (DRY)!

You have this code several times:

        print(f"| {p1} | {p2} | {p3} |")
        print(f"| {p4} | {p5} | {p6} |")
        print(f"| {p7} | {p8} | {p9} |")

You should move it into a function. You’ve demonstrated you know how to use them already.

Return Type Consistency

win_check() will return either an integer 1, or it doesn’t return anything at all, so it implicitly returns None. That makes it hard to reason about. You can’t say it returns an int, nor can you say it returns a bool.

In the code which calls win_check(), you've ended up with if won == 1, and while won != 1 which are opaque conditions. What does won == 1 mean? Would won == 2 mean player 2 has won? Is won == 0 a draw game, or game not over yet? It isn't obvious. In actuality, won == 0 before the first move is made, and suddenly becomes won == None afterwards, until someone wins. Very confusing.

The function should return a boolean True or False. Instead of return 1, use return True. At the end, add an explicit return False.

Lists -vs- Sets

You have the list used_positions recording which moves have been made, and append moves to it, and test pos in used_positions to ensure moves aren’t repeated. The correct data structure to use for this would be a set.

used_positions = set()
...
   while ... or x_pos in used_positions:
      ...
   used_position.add(x_pos)
...

Using sets reduces the search time of x_pos in used_positions to \$O(1)\$ … effectively constant time, instead of the \$O(N)\$ time of searching a list.

But …

Three Different versions of the Same Thing

You have game_playout representing the played positions for determining a win condition. You have used_positions for recording which moves have been played, to prevent repeats, and p1, p2, … p9 for displaying which positions have been played by which player.

These are all the same thing! Or they should be!

Consider:

grid = [" "] * 9

That creates a list of length 9, with each element containing a space. Perfect for displaying the tic-tac-toe grid:

def print_grid():
    print(f"| {grid[0]} | {grid[1]} | {grid[2]} |")
    print(f"| {grid[3]} | {grid[4]} | {grid[5]} |")
    print(f"| {grid[6]} | {grid[7]} | {grid[8]} |")

Or slightly more compactly, as:

def print_grid():
    print(("| {} | {} | {} |\n"
           "| {} | {} | {} |\n"
           "| {} | {} | {} |").format(*grid))

Note: There are no commas between the above strings, so Python automatically concatenates them into one long string containing 9 {} format codes. The *grid explodes the elements of the grid list into individual arguments for the .format() function.

As moves are made, the grid can be filled in with x and o values:

    grid[x_pos - 1] = 'x'
...
    grid[o_pos - 1] = 'o'

… which eliminates the ugly mess called layout_changer().

The used_positions check becomes simply testing if the spot is not “empty”:

    while ... or grid[x_pos - 1] != " ":
      ...

Finally, win_check() needs to be fixed so it doesn’t declare 3 spaces in a row as a winning combination:

def win_check():
    if grid[0] == grid[1] == grid[2] != ' ':
        return True
    elif grid[3] == grid[4] == grid[5] != ' ':
        return True
    …
    return False

Draw

You determine a draw by maintaining a count and checking if it reaches 9. This can again be another variant of the grid. The game is a draw if nobody won and no valid moves remain. No valid moves remain means grid contains no empty cells.

    elif " " not in grid:
        # draw game

No more counting required.

Don’t quit!

Avoid quit(). Never ever call it, as it exits the Python interpreter. This makes testing nigh impossible, since tests are usually written in Python, and don’t get a chance to declare pass or failure if the Python interpreter vanishes before they can determine the pass/fail state!

Instead, move the mainline code into a function, and simple return from the function instead of calling quit().

X/O

The X player and O player code is virtually identical. Different variables are used (x_pos -vs- o_pos) but these could be replaced with just pos eliminating that difference. Then the only difference is the message printed, and the code that is assigned to the given position.

If the code is the same, it too can be moved into a function.

def player_move(symbol):
    pos = input(f"Choose your position to place {symbol} (1 to 9) ")
    while not pos.isnumeric():
        print("Error! Looks like you've haven't entered a number, please try again!")
        pos = input(f"Choose your position to place {symbol} (1 to 9) ")
    pos = int(pos)
    while pos not in [1, 2, 3, 4, 5, 6, 7, 8, 9] or grid[pos - 1] != " ":
        print_grid()
        print("Invalid position! Please try another value:")
        pos = int(input(f"Choose your position to place {symbol} (1 to 9) "))

     grid[pos - 1] = symbol

Avoid Global variables

At this point, it should be obvious the only global variable left is grid. This could simply be passed to the various functions as an argument, eliminating the last global variable.

Why are global variables bad? In 150 lines, you have 15 global variables. Which functions can change a global variable? Which functions use global variables? These are hard questions. If you expand your code to 1500 lines, say to add a TkInter GUI interface for your game, are you going to end up with 150 global variables? Now reasoning about which functions use and change which variables becomes even harder.

Another question: what variables do you need to reset if you wanted to play a second game?

What if you wanted to player against the computer, and you'd like the computer to "think" about playing various moves to try and play smarter. Currently you have to make a tentative move, evaluate it, then "undo" the move to explore another possibility. If you used local variables, you could make a copy of the game state to try out a move, and then discard it; nothing to "undo".

Reworked code

Here is a reworking of your code, using the above points. The code has gone from 150 lines to 74. I've introduced a few extra concepts for you to study try...except..., import itertools, any(...) and all(...). Hopefully, these are not too far beyond your current level, so you find them understandable.

import itertools

BLANK = ' '
X = 'x'
O = 'o'

WINNING_ROWS = (
    (0, 1, 2), (3, 4, 5), (6, 7, 8),    # rows
    (0, 3, 6), (1, 4, 7), (2, 5, 8),    # columns
    (0, 4, 8), (2, 4, 6),               # diagonals
    )
    
def print_grid(grid):
    print("| {} | {} | {} |\n"
          "| {} | {} | {} |\n"
          "| {} | {} | {} |".format(*grid))

def win_check(grid, symbol) -> bool:
    return any(all(grid[index] == symbol
                   for index in row)
               for row in WINNING_ROWS)

def player_move(grid, symbol):
    while True:
        try:
            pos = input(f"Choose your position to place {symbol} (1 to 9): ")
            pos = int(pos) - 1

            if pos not in range(9):
                print("Error!  Looks like you've entered an invalid position, please try again!")
            elif grid[pos] != BLANK:
                print("Error!  Looks like you've entered a filled position, please try again!")
            else:
                break

        except ValueError:
            print("Error! Looks like you've haven't entered a number, please try again!")

    grid[pos] = symbol
    print_grid(grid)

def game():
    grid = [BLANK] * 9
    print_grid(grid)

    symbols = itertools.cycle((X, O))
    
    while any(cell == BLANK for cell in grid):

        symbol = next(symbols)
        player_move(grid, symbol)
        if win_check(grid, symbol):
            print(f"{symbol} has won! Congratulations!")
            break
    else:
        print("Draw!")

    print()
    print("Thank you for playing the game! Hope you enjoyed!")
    

def main():
    print("Welcome to the game of tic-tac-toe, you know how to play, so go on and have fun!")
    print("Just know that the positions to input are of the form: ")
    print("| 1 | 2 | 3 |")
    print("| 4 | 5 | 6 |")
    print("| 7 | 8 | 9 |")
    print()

    game()


if __name__ == '__main__':
    main()
\$\endgroup\$
2
  • \$\begingroup\$ Thank you SO MUCH for the reply, i never expected to get such an in-depth reply, you are a legend❤️! I only didn't get 2 things, in your explanation: 1) I dont see the need for win_check to return a value of False since we won't be using it anyway and will only use True,(unless it's a reason related to some internal working if python that makes the code better to execute. 2) Why should we avoid global variables? \$\endgroup\$ Sep 29, 2022 at 11:40
  • \$\begingroup\$ @Feraminecarts I've expanded the win_check & global section portions of my answer, and added my reworking of the code. Hope that helps answer your questions. \$\endgroup\$
    – AJNeufeld
    Sep 29, 2022 at 23:04

Your Answer

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

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