6
\$\begingroup\$

I made this as a personal challenge. It seems to work fine as I can't find any bugs and am happy with how it runs, but I am interested in what I should do to make the code more professional.

I think the draw() function can be replaced with two for loops but I could never get it working. The code for the input by each of the players and the check_board() function seems messy and I think it could be reduced by using arguments. I also think the while loop could be reduced somehow.

def tic_tac_toe():
    board = [1, 2, 3, 4, 5, 6, 7, 8, 9]
    end = False
    win_commbinations = ((0, 1, 2), (3, 4, 5), (6, 7, 8), (0, 3, 6), (1, 4, 7), (2, 5, 8), (0, 4, 8), (2, 4, 6))

    def draw():
        print(board[0], board[1], board[2])
        print(board[3], board[4], board[5])
        print(board[6], board[7], board[8])
        print()

    def p1():
        n = choose_number()
        if board[n] == "X" or board[n] == "O":
            print("\nYou can't go there. Try again")
            p1()
        else:
            board[n] = "X"

    def p2():
        n = choose_number()
        if board[n] == "X" or board[n] == "O":
            print("\nYou can't go there. Try again")
            p2()
        else:
            board[n] = "O"

    def choose_number():
        while True:
            while True:
                a = input()
                try:
                    a  = int(a)
                    a -= 1
                    if a in range(0, 9):
                        return a
                    else:
                        print("\nThat's not on the board. Try again")
                        continue
                except ValueError:
                   print("\nThat's not a number. Try again")
                   continue

    def check_board():
        count = 0
        for a in win_commbinations:
            if board[a[0]] == board[a[1]] == board[a[2]] == "X":
                print("Player 1 Wins!\n")
                print("Congratulations!\n")
                return True

            if board[a[0]] == board[a[1]] == board[a[2]] == "O":
                print("Player 2 Wins!\n")
                print("Congratulations!\n")
                return True
        for a in range(9):
            if board[a] == "X" or board[a] == "O":
                count += 1
            if count == 9:
                print("The game ends in a Tie\n")
                return True

    while not end:
        draw()
        end = check_board()
        if end == True:
            break
        print("Player 1 choose where to place a cross")
        p1()
        print()
        draw()
        end = check_board()
        if end == True:
            break
        print("Player 2 choose where to place a nought")
        p2()
        print()

    if input("Play again (y/n)\n") == "y":
        print()
        tic_tac_toe()

tic_tac_toe()
\$\endgroup\$
6
\$\begingroup\$

My strongest objection is to your use of recursion as a kind of goto:

if input("Play again (y/n)\n") == "y":
    print()
    tic_tac_toe()

I suggest changing the tic_tac_toe() function to play just one game, then calling it in a loop.

You do some similar recursive calls in p1() and p2().

Board layout

Your board layout is

1 2 3
4 5 6
7 8 9

For a better user experience, I suggest flipping it upside-down to mimic a computer number keypad.

I think that the code would be less confusing if the board labels matched the list indices.

Handling two players

The p1() function resembles p2(), and the main loop contains two copies of code that is mostly the same. check_board() also has some repetition. The root cause is the need to associate Player 1 with the symbol X and the verbal description "cross", and Player 2 with the symbol O and the verbal description "nought". The code would be easier to generalize if you just called them "Player X" and "Player O", and I think that the user experience would be enhanced as well. (You could also generalize the code without such a simplification by using, say, a namedtuple to represent each player.)

p1() and p2() could be simplified by moving the validation into choose_number(), especially if you didn't care to distinguish between the "You can't go there" and "That's not on the board" error messages.

Implementation details

  • In choose_number(), there is no need for a nested while True. You also don't need to write continue.
  • Renaming check_board() to is_game_over() would make its purpose more obvious
  • win_commbinations contains a misspelling, and would be better as WIN_COMBINATIONS to indicate that it is a constant. board[a[0]] == board[a[1]] == board[a[2]] is ugly, and would be better expressed as

    for a, b, c in WIN_COMBINATIONS:
        if board[a] == board[b] == board[c] …
    
  • In check_board(), you used the variable a for two different purposes, which is confusing. I would just rewrite the second half of the function entirely as

    if 9 == sum((pos == 'X' or pos == 'O') for pos in board):
        print("The game ends in a tie\n")
        return True
    
  • There is no need for the end variable. (In general, such flag variables should be eliminated.)

Suggested solution

def tic_tac_toe():
    board = [None] + list(range(1, 10))
    WIN_COMBINATIONS = [
       (1, 2, 3),
       (4, 5, 6),
       (7, 8, 9),
       (1, 4, 7),
       (2, 5, 8),
       (3, 6, 9),
       (1, 5, 9),
       (3, 5, 7),
    ]

    def draw():
        print(board[7], board[8], board[9])
        print(board[4], board[5], board[6])
        print(board[1], board[2], board[3])
        print()

    def choose_number():
        while True:
            try:
                a = int(input())
                if a in board:
                    return a
                else:
                    print("\nInvalid move. Try again")
            except ValueError:
               print("\nThat's not a number. Try again")

    def is_game_over():
        for a, b, c in WIN_COMBINATIONS:
            if board[a] == board[b] == board[c]:
                print("Player {0} wins!\n".format(board[a]))
                print("Congratulations!\n")
                return True
        if 9 == sum((pos == 'X' or pos == 'O') for pos in board):
            print("The game ends in a tie\n")
            return True

    for player in 'XO' * 9:
        draw()
        if is_game_over():
            break
        print("Player {0} pick your move".format(player))
        board[choose_number()] = player
        print()

while True:
    tic_tac_toe()
    if input("Play again (y/n)\n") != "y":
        break
\$\endgroup\$
3
\$\begingroup\$

A small suggestion: Your tic_tac_toe has a lot of nested functions. Consider making it a class:

class TicTacToe:

and then giving it a run method:

TicTacToe.run()
\$\endgroup\$
2
\$\begingroup\$

Initialising board as containing integers that will then be replaced by strings is a bit off. You're mixing types needlessly when you could just make these strings. It has no functional difference but it's good to be in the mindset of keeping data consistent unless you have a good reason to deviate.

draw is not well laid out at all. Instead of just plain printing arguments, use str.format. That would make it easier to adjust how they're arranged, instead of just sticking with the default result of using print with commas. This is how you could do it:

def draw():
    print("{} {} {}\n{} {} {}\n {} {} {}\n".format(*board))

To explain, str.format will replace each {} with arguments passed to it. * is the unpacking operator, that means that all the values from board are being unpacked and passed. So you're actually passing the nine elements of the list, not the list itself.

Instead of board[n] == "X" or board[n] == "O" you can use board[n] in "XO". The in keyword tests for if a value is in a string, list or other collection. It's often an easier way of testing equality for multiple values.

It's good you're doing rigorous input testing in choose_number. However it has 2 while True loops for no reason. Just having one does exactly the same job. Also you don't need to call continue in either of the places you've used it. In both cases one iteration is about to end and it will start a new iteration even if you remove those lines so they make no difference.

9 in here is what's known as a 'magic number', that's a value that's used without making it clear why. Instead of 9, you should use len(board) so that it's obvious why it matters. If you don't want to call len each time, then set it as a value when you initialise board. Also testing for a in range(0, len(board)) is a bit silly, instead use 2 less than operators. Python allows you to compare three values at once, so you can do this:

if -1 < a < len(board):

You could do 0 <= if you prefer, but I like using the same operator on both sides. Also a isn't a great name, consider using value or choice.

That's a lot of suggestions, so here's how I'd rewrite it:

def choose_number():
    while True:
        value = input()
        try:
            value = int(value) - 1
            if -1 < value < len(board):
                return value
            else:
                print("\nThat's not on the board. Try again")
        except ValueError:
           print("\nThat's not a number. Try again")

In check_board you're using for a in range(9) and then accessing board[a], but you could instead use for cell in board, where cell (or whatever you choose to name it) is now directly assigned to the value of each element of board. This is faster, easier and more idiomatic.

   for cell in board:
        if cell in "XO":
            count += 1

You can also use all for this looped test. all will basically run a test on all elements of an iterator and return True if they all turn out to be True. Which is basically what you're doing with your count value. What you need to do is pass a generator expression to all. Generator expressions are like for loops but collapsed into a single expression. Here's how yours might look:

if all(cell in "XO" for cell in board):
    print("The game ends in a Tie\n")
    return True

Instead of storing end then immediately using it once, just use the returned value from check_board directly. Also don't use == True. Just test if value, it's more idiomatic. If you want to explicitly test that it's a boolean then use is True instead.

    if check_board():
        break
\$\endgroup\$
  • 1
    \$\begingroup\$ The part on the generator is a bit dense, but probably the most important part of SuperBiasedMan's answer. They are really useful. If you haven't already, definitely read the documentation and learn to use them. \$\endgroup\$ – spectras Nov 1 '15 at 17:48

protected by Community May 8 '18 at 9:11

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

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