4
\$\begingroup\$

I just finished coding a Tic Tac Toe game for my first Python project. Everything seems to be running correctly, but I wanted to get some advice on things I can improve. The main loop is pretty messy and I'm sure there is a way I can clean that up. Also I have many repetitive if statements that I think can be looped somehow.

import sys
import time


def check_position(the_position):
    if the_position not in taken_positions:
        return True
    else:
        return False

def get_position():
    position = int(input("Enter a position : "))
    return position

def draw_position_x(location):
    if location == 1:
        game_board[0][0] = 'x'
        print("|" + game_board[0][0] + "|" + game_board[1][0] + "|" + game_board[2][0] + "|")
        print("|" + game_board[0][1] + "|" + game_board[1][1] + "|" + game_board[2][1] + "|")
        print("|" + game_board[0][2] + "|" + game_board[1][2] + "|" + game_board[2][2] + "|")

    if location == 2:
        game_board[1][0] = 'x'
        print("|" + game_board[0][0] + "|" + game_board[1][0] + "|" + game_board[2][0] + "|")
        print("|" + game_board[0][1] + "|" + game_board[1][1] + "|" + game_board[2][1] + "|")
        print("|" + game_board[0][2] + "|" + game_board[1][2] + "|" + game_board[2][2] + "|")
    if location == 3:
        game_board[2][0] = 'x'
        print("|" + game_board[0][0] + "|" + game_board[1][0] + "|" + game_board[2][0] + "|")
        print("|" + game_board[0][1] + "|" + game_board[1][1] + "|" + game_board[2][1] + "|")
        print("|" + game_board[0][2] + "|" + game_board[1][2] + "|" + game_board[2][2] + "|")
    if location == 4:
        game_board[0][1] = 'x'
        print("|" + game_board[0][0] + "|" + game_board[1][0] + "|" + game_board[2][0] + "|")
        print("|" + game_board[0][1] + "|" + game_board[1][1] + "|" + game_board[2][1] + "|")
        print("|" + game_board[0][2] + "|" + game_board[1][2] + "|" + game_board[2][2] + "|")
    if location == 5:
        game_board[1][1] = 'x'
        print("|" + game_board[0][0] + "|" + game_board[1][0] + "|" + game_board[2][0] + "|")
        print("|" + game_board[0][1] + "|" + game_board[1][1] + "|" + game_board[2][1] + "|")
        print("|" + game_board[0][2] + "|" + game_board[1][2] + "|" + game_board[2][2] + "|")
    if location == 6:
        game_board[2][1] = 'x'
        print("|" + game_board[0][0] + "|" + game_board[1][0] + "|" + game_board[2][0] + "|")
        print("|" + game_board[0][1] + "|" + game_board[1][1] + "|" + game_board[2][1] + "|")
        print("|" + game_board[0][2] + "|" + game_board[1][2] + "|" + game_board[2][2] + "|")
    if location == 7:
        game_board[0][2] = 'x'
        print("|" + game_board[0][0] + "|" + game_board[1][0] + "|" + game_board[2][0] + "|")
        print("|" + game_board[0][1] + "|" + game_board[1][1] + "|" + game_board[2][1] + "|")
        print("|" + game_board[0][2] + "|" + game_board[1][2] + "|" + game_board[2][2] + "|")
    if location == 8:
        game_board[1][2] = 'x'
        print("|" + game_board[0][0] + "|" + game_board[1][0] + "|" + game_board[2][0] + "|")
        print("|" + game_board[0][1] + "|" + game_board[1][1] + "|" + game_board[2][1] + "|")
        print("|" + game_board[0][2] + "|" + game_board[1][2] + "|" + game_board[2][2] + "|")
    if location == 9:
        game_board[2][2] = 'x'
        print("|" + game_board[0][0] + "|" + game_board[1][0] + "|" + game_board[2][0] + "|")
        print("|" + game_board[0][1] + "|" + game_board[1][1] + "|" + game_board[2][1] + "|")
        print("|" + game_board[0][2] + "|" + game_board[1][2] + "|" + game_board[2][2] + "|")

def draw_position_o(location):
    if location == 1:
        game_board[0][0] = 'o'
        print("|" + game_board[0][0] + "|" + game_board[1][0] + "|" + game_board[2][0] + "|")
        print("|" + game_board[0][1] + "|" + game_board[1][1] + "|" + game_board[2][1] + "|")
        print("|" + game_board[0][2] + "|" + game_board[1][2] + "|" + game_board[2][2] + "|")
    if location == 2:
        game_board[1][0] = 'o'
        print("|" + game_board[0][0] + "|" + game_board[1][0] + "|" + game_board[2][0] + "|")
        print("|" + game_board[0][1] + "|" + game_board[1][1] + "|" + game_board[2][1] + "|")
        print("|" + game_board[0][2] + "|" + game_board[1][2] + "|" + game_board[2][2] + "|")
    if location == 3:
        game_board[2][0] = 'o'
        print("|" + game_board[0][0] + "|" + game_board[1][0] + "|" + game_board[2][0] + "|")
        print("|" + game_board[0][1] + "|" + game_board[1][1] + "|" + game_board[2][1] + "|")
        print("|" + game_board[0][2] + "|" + game_board[1][2] + "|" + game_board[2][2] + "|")
    if location == 4:
        game_board[0][1] = 'o'
        print("|" + game_board[0][0] + "|" + game_board[1][0] + "|" + game_board[2][0] + "|")
        print("|" + game_board[0][1] + "|" + game_board[1][1] + "|" + game_board[2][1] + "|")
        print("|" + game_board[0][2] + "|" + game_board[1][2] + "|" + game_board[2][2] + "|")
    if location == 5:
        game_board[1][1] = 'o'
        print("|" + game_board[0][0] + "|" + game_board[1][0] + "|" + game_board[2][0] + "|")
        print("|" + game_board[0][1] + "|" + game_board[1][1] + "|" + game_board[2][1] + "|")
        print("|" + game_board[0][2] + "|" + game_board[1][2] + "|" + game_board[2][2] + "|")
    if location == 6:
        game_board[2][1] = 'o'
        print("|" + game_board[0][0] + "|" + game_board[1][0] + "|" + game_board[2][0] + "|")
        print("|" + game_board[0][1] + "|" + game_board[1][1] + "|" + game_board[2][1] + "|")
        print("|" + game_board[0][2] + "|" + game_board[1][2] + "|" + game_board[2][2] + "|")
    if location == 7:
        game_board[0][2] = 'o'
        print("|" + game_board[0][0] + "|" + game_board[1][0] + "|" + game_board[2][0] + "|")
        print("|" + game_board[0][1] + "|" + game_board[1][1] + "|" + game_board[2][1] + "|")
        print("|" + game_board[0][2] + "|" + game_board[1][2] + "|" + game_board[2][2] + "|")
    if location == 8:
        game_board[1][2] = 'o'
        print("|" + game_board[0][0] + "|" + game_board[1][0] + "|" + game_board[2][0] + "|")
        print("|" + game_board[0][1] + "|" + game_board[1][1] + "|" + game_board[2][1] + "|")
        print("|" + game_board[0][2] + "|" + game_board[1][2] + "|" + game_board[2][2] + "|")
    if location == 9:
        game_board[2][2] = 'o'
        print("|" + game_board[0][0] + "|" + game_board[1][0] + "|" + game_board[2][0] + "|")
        print("|" + game_board[0][1] + "|" + game_board[1][1] + "|" + game_board[2][1] + "|")
        print("|" + game_board[0][2] + "|" + game_board[1][2] + "|" + game_board[2][2] + "|")

def win_check():
    if game_board[0][0] == 'x' and game_board[1][0] == 'x' and game_board[2][0] == 'x' or \
        (game_board[0][0] == 'o' and game_board[1][0] == 'o' and game_board[2][0] == 'o'):  # 1 HORIZONTAL
        return True
    if game_board[0][1] == 'x' and game_board[1][1] == 'x' and game_board[2][1] == 'x' or \
        (game_board[0][1] == 'o' and game_board[1][1] == 'o' and game_board[2][1] == 'o'):  # 2 HORIZONTAL
        return True
    if game_board[0][2] == 'x' and game_board[1][2] == 'x' and game_board[2][2] == 'x' or \
        (game_board[0][2] == 'o' and game_board[1][2] == 'o' and game_board[2][2] == 'o'):  # 3 HORIZONTAL
        return True
    if game_board[0][0] == 'x' and game_board[0][1] == 'x' and game_board[0][2] == 'x' or \
        (game_board[0][0] == 'o' and game_board[0][1] == 'o' and game_board[0][2] == 'o'):  # 1 VERTICAL
        return True
    if game_board[1][0] == 'x' and game_board[1][1] == 'x' and game_board[1][2] == 'x' or \
        (game_board[1][0] == 'o' and game_board[1][1] == 'o' and game_board[1][2] == 'o'):  # 2 VERTICAL
        return True
    if game_board[2][0] == 'x' and game_board[2][1] == 'x' and game_board[2][2] == 'x' or \
        (game_board[2][0] == 'o' and game_board[2][1] == 'o' and game_board[2][2] == 'o'):  # 3 VERTICAL
        return True
    if game_board[0][0] == 'x' and game_board[1][1] == 'x' and game_board[2][2] == 'x' or \
        (game_board[0][0] == 'o' and game_board[1][1] == 'o' and game_board[2][2] == 'o'):  # 1 DIAGONAL
        return True
    if game_board[0][2] == 'x' and game_board[1][1] == 'x' and game_board[2][0] == 'x' or \
        (game_board[0][2] == 'o' and game_board[1][1] == 'o' and game_board[2][0] == 'o'):  # 2 DIAGONAL
        return True
    else:
        return False

intro_board = [['1', '4', '7'], ['2', '5', '8'], ['3', '6', '9']]

print("Welcome to TIC TAC TOE!")
print("You can pick location by identifying the position on the board. (There are 9 positions)")
print("The player who plays first will be using 'x' and the second player will be using 'o'.")
print("|" + intro_board[0][0] + "|" + intro_board[1][0] + "|" + intro_board[2][0] + "|")
print("|" + intro_board[0][1] + "|" + intro_board[1][1] + "|" + intro_board[2][1] + "|")
print("|" + intro_board[0][2] + "|" + intro_board[1][2] + "|" + intro_board[2][2] + "|")

game_board = [[' ', ' ', ' '], [' ', ' ', ' '], [' ', ' ', ' ']]
taken_positions = []
which_turn = 'P1'
win = False
num_moves = 0
running = True
isValid = True

while running:
    while num_moves < 9 and not win:
        which_position = get_position()

        if which_turn == 'P1':

            isValid = check_position(which_position)
            if isValid:
                which_turn = 'P2'
                num_moves = num_moves + 1
                taken_positions.append(which_position)
                draw_position_x(which_position)
                break
            if not isValid:
                print("Position taken, try again.")
                break
        if which_turn == 'P2':

            isValid = check_position(which_position)
            if isValid:
                which_turn = 'P1'
                num_moves = num_moves + 1
                taken_positions.append(which_position)
                draw_position_o(which_position)
                break
            if not isValid:
                print("Position taken, try again.")
                break

    win = win_check()
    if win:
        print("We have a winner!!!")
        print("Exiting in 10 seconds")
        time.sleep(10)
        running = False

    if num_moves == 9 and not win:
        print("Draw!")
        print("Exiting in 10 seconds")
        time.sleep(10)
        running = False

if not running:
    print("Exiting Game")
    sys.exit()
\$\endgroup\$

3 Answers 3

Reset to default
3
\$\begingroup\$

Welcome to the site, and welcome to programming! I've recently implemented tic tac toe for console as a practice exercise, so hopefully I can provide some helpful advice in improving your program. If you have any questions about my answer, please feel free to ask them in a comment below my answer.

In general, avoid global variables like intro_board, game_board, etc., with rare exception. It is much better to have each of those variables be part of play_game function that then passes variables as arguments to the other functions when necessary, instead of having unrestricted access to the variables from any Python function. It will make your functions more generic and will make the program flow easier to understand, in turn reducing the chance of errors.

The other main thing I would say about your program is don't repeat yourself (known as the DRY principle). Avoiding repetition is an important step in making your programs easier to maintain. Of course, other design elements can significantly impact the effectiveness of your program, but I think learning to reduce duplication is one of the most important skills one can learn to advance their craft as a programmer. Therefore, I'm going to spend the rest of my answer walking through duplicated code and demonstrating how to fix the duplication.

Duplication

Case study: repeated "|"

To start, I want to focus on just one line:

print("|" + game_board[0][0] + "|" + game_board[1][0] + "|" + game_board[2][0] + "|")

Looking at this line, you may notice "|" is duplicated. We can fix that! Instead of duplicating "|", you could just write a function to surround game tiles with "|":

def surround_tiles(sep, tile1, tile2, tile3):
    """Surround and intersperse three tiles with sep."""
    return sep + tile1 + sep + tile2 + sep + tile3 + sep

At this point, I will mention that Python already provides a function that is appealing for avoiding string repetition: it's called str.join (where "str" represents an object of type str). We could use it as follows:

print("|" + "|".join([game_board[0][0], game_board[1][0], game_board[2][0]]) + "|")

However, we still retain the repetition of "|", so unfortunately str.join is not quite right for our situation. A better solution would be:

def intersperse(sep, lst):
    """Surround and intersperse an iterable with sep."""
    output = sep
    for i in lst:
        output += i + sep
    return output

Mind, this is probably not the fastest solution, but that is a topic for another question. This solution is simply an example of removing the duplicated "|" from that single line.

The larger duplication

These three lines are repeated verbatim 18 times in your program:

    print("|" + game_board[0][0] + "|" + game_board[1][0] + "|" + game_board[2][0] + "|")
    print("|" + game_board[0][1] + "|" + game_board[1][1] + "|" + game_board[2][1] + "|")
    print("|" + game_board[0][2] + "|" + game_board[1][2] + "|" + game_board[2][2] + "|")

This is easily avoidable: simply remove the printing code from the printing code out of the conditional changing of a board position, and do it after that task is accomplished (I'll show how in a moment).

Looking ahead, draw_position_x and draw_position_o are the same function with one letter different! We could halve our lines of code between the two functions by just making a generic draw_position function with a symbol parameter that determines the character that is inserted into the game board.

Using all the knowledge we've gathered so far in this section, we can turn the original 94 lines into just 24 lines:

def draw_position(location, symbol):
    if location == 1:
        game_board[0][0] = symbol
    if location == 2:
        game_board[1][0] = symbol
    if location == 3:
        game_board[2][0] = symbol
    if location == 4:
        game_board[0][1] = symbol
    if location == 5:
        game_board[1][1] = symbol
    if location == 6:
        game_board[2][1] = symbol
    if location == 7:
        game_board[0][2] = symbol
    if location == 8:
        game_board[1][2] = symbol
    if location == 9:
        game_board[2][2] = symbol
    print("|" + game_board[0][0] + "|" + game_board[1][0] + "|" + game_board[2][0] + "|")
    print("|" + game_board[0][1] + "|" + game_board[1][1] + "|" + game_board[2][1] + "|")
    print("|" + game_board[0][2] + "|" + game_board[1][2] + "|" + game_board[2][2] + "|")

But we can do even better! The input translation conditional is unnecessarily repetitive with the if statements, having an individual conditional check for each number from 1 to 9. We could make a dictionary with each input value and its resultant coordinates:

NUM_TO_COORD = {
    1: (0, 0)
    2: (1, 0)
    3: (2, 0)
    4: (0, 1)
    5: (1, 1)
    6: (2, 1)
    7: (0, 2)
    8: (1, 2)
    9: (2, 2)
}

def draw_position(location, symbol):
    coord = NUM_TO_COORD[location]
    game_board[coord[0]][coord[1]] = symbol

    # ...

But we don't actually need such a dictionary. We could simply translate the coordinate on the fly, and avoid hard-coding something that could easily have a typographical error somewhere. To do this we, use a process of translating our one dimensional user input into two dimensional coordinates with the modulus and division operators (this is basically doing a base conversion process):

def num_to_coord(num):
    # 0-index num
    num -= 1
    coord = []
    while True:
        curr_coord = num % 3
        coord.append(curr_coord)
        if len(coord) >= 2:
            break
        num -= curr_coord
        num //= 3
    return coord

def draw_position(location, symbol):
    coord = num_to_coord(location)
    game_board[coord[0]][coord[1]] = symbol

    # ...

And finally, we can come back to the game board printing code:

    print("|" + game_board[0][0] + "|" + game_board[1][0] + "|" + game_board[2][0] + "|")
    print("|" + game_board[0][1] + "|" + game_board[1][1] + "|" + game_board[2][1] + "|")
    print("|" + game_board[0][2] + "|" + game_board[1][2] + "|" + game_board[2][2] + "|")

It can be printed with much less repetition by doing two loops to print it, and using the intersperse function from before:

    for y in range(3):
        print(intersperse('|', [game_board[x][y] for x in range(3)]))

There are more improvements that can be made, but I've demonstrated the general principle. If you apply this methodology to your coding, you will have much better code. You could even try revising this code and post a new question!

Addendum

Based on the conversation we had, there are a few things I'll add:

It is better to have everything* in a function because it allows the module to be imported without running the program. However, there is a standard way that Python programmers run code directly from the module: by conditioning any run code with:

if __name__ == '__main__':
    # ...

Generally, this code block is placed at the bottom of the program. It is good to put the code in a main function, because it allows one to import the main running code. Also, splitting things into smaller function parts is called procedural programming.

* However, I have asterisked the word "everything" because there is another aspect of programming that may be new to you: object oriented programming (abbreviated OOP). It uses something (unsurprisingly) called objects, which are basically date types. Python is a bit special because every data type and everything in Python is an object. And by everything, I mean pretty much everything, not just variable values!

In modern programming, procedural programming and OOP are generally used together, though it depends on the program.

But don't worry about taking in all the subtle nuances now, it will come with time, and having a greater context. I personally find that practicing programming and reviewing your work is the most effective way to improve as a programmer, though it does depend on what you're programming.

\$\endgroup\$
2
  • \$\begingroup\$ @BobPage Whoops. I've never linked to chat before, so I didn't realize it would automatically do that previous comment. I think we're done talking for now, but the chat transcript is here, in case a moderator comes around and deletes comments (since comments are designed to be temporary on Stack Exchange sites). \$\endgroup\$
    – Graham
    Commented Jan 6, 2019 at 19:29
  • \$\begingroup\$ I don't know what happened but I accidentally deleted the comments... I've got the transcript so no big deal. \$\endgroup\$
    – BobPage
    Commented Jan 6, 2019 at 20:18
3
\$\begingroup\$

draw_position_x and draw_position_o do two things currently - they add a move to the board and they draw it. You should separate those into two functions, one to add a move and one to draw the board, because drawing the board doesn't depend on the move that was just played. A separate draw function won't need any if statements, eliminating a lot of the duplication currently in the code.

You do also do not need a separate functions for placing an 'x' move and placing an 'o' move; use a single function and pass the move 'x' or 'o' as a parameter.

while running:
    while num_moves < 9 and not win:

You should be able to eliminate one of these two while statements. You can have a single while num_moves < 9 and not win which is sufficient to keep execution in the main game loop until the game is over, and remove all the break statements within the loop. If you need or want to jump back to the top of the loop (for this program, perhaps after bad input) you can use continue instead of break.

The three VERTICAL cases and three HORIZONTAL cases in win_check could be tested with a for loop over columns/rows, instead of repetition.

You don't need to sys.exit() at the end of the program — the program exits when it reaches the end of the code.

\$\endgroup\$
2
\$\begingroup\$

Use boolean expressions directly

This:

if the_position not in taken_positions:
    return True
else:
    return False

should be

return the_position not in taken_positions

Use generators

This:

game_board = [[' ', ' ', ' '], [' ', ' ', ' '], [' ', ' ', ' ']]

can be

game_board = [[' ']*3 for _ in range(3)]

Avoid sleep

This:

print("Exiting in 10 seconds")
time.sleep(10)

is generally not a good idea. The convention for console programs is that they run in a persistent console that doesn't vanish after the program exits. As such, drop the sleep.

Indentation bug?

This code:

if not running:
    print("Exiting Game")
    sys.exit()

seems out-of-place. If it belongs in the loop, indent it. Also, you don't need an exit here; simply break out of the loop.

If it does belong at the top level, it doesn't do anything that wouldn't already happen by the program exiting normally (other than the print).

\$\endgroup\$

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.