11
\$\begingroup\$

I'm brand-new (just joined today) to code review and I am super excited to share my tic tac toe game code with all of you! Please let me know what I could fix up in my program to make it more efficient. Thanks in advance!

P.S. I'm also not sure how to add an actual board to this game (an actual board rather than the way I have done here)

Code:

import time

# Initialize board
board = {1: ' ', 2: ' ', 3: ' ',
         4: ' ', 5: ' ', 6: ' ',
         7: ' ', 8: ' ', 9: ' '}

# Initialize variables
count = 0  # counter to track number of filled slots
winner = False  # boolean to check if anyone won
play = True  # boolen to check if the game should continue
tie = False  # boolean to check if there is a tie
curr_player = ''  # variable to store current player identifier
player_details = []  # list to store player identifier and marker


# Helper functions
def get_player_details(curr_player):
    """Function to get player identifier and marker"""
    if curr_player == 'A':
        return ['B', 'O']
    else:
        return ['A', 'X']


def print_board():
    """Function to print the board"""
    for i in board:
        print(i, ':', board[i], ' ', end='')
        if i % 3 == 0:
            print()


def win_game(marker, player_id):
    """Function to check for winning combination"""
    if board[1] == marker and board[2] == marker and board[3] == marker or \
            board[4] == marker and board[5] == marker and board[6] == marker or \
            board[7] == marker and board[8] == marker and board[9] == marker or \
            board[1] == marker and board[4] == marker and board[7] == marker or \
            board[2] == marker and board[5] == marker and board[8] == marker or \
            board[3] == marker and board[6] == marker and board[9] == marker or \
            board[1] == marker and board[5] == marker and board[9] == marker or \
            board[3] == marker and board[5] == marker and board[7] == marker:

        print_board()
        time.sleep(1)
        print("Player", player_id, "wins!")
        return True

    else:
        return False


def insert_input(slot_num, marker):
    """Function for capturing user inputs"""
    while board[slot_num] != ' ':
        print("spot taken, pick another no.")
        slot_num = int(input())
    board[slot_num] = marker


def play_again():
    """Function to check if player wants to play again"""
    print("Do you want to play again?")
    play_again = input()

    if play_again.upper() == 'Y':
        for z in board:
            board[z] = ' '
        return True
    else:
        print("Thanks for playing. See you next time!")
        return False


# Main program
while play:

    print_board()

    player_details = get_player_details(curr_player)
    curr_player = player_details[0]
    print("Player {}: Enter a number between 1 and 9".format(curr_player))
    input_slot = int(input())

    # Inserting 'X' or 'O' in desired spot
    insert_input(input_slot, player_details[1])
    count += 1

    # Check if anybody won
    winner = win_game(player_details[1], curr_player)
    if count == 9 and not winner:
        print("It's a tie!!")
        tie = True
        print_board()

    # Check if players want to play again
    if winner or tie:
        play = play_again()
        if play:
            curr_player = ''
            count = 0
\$\endgroup\$
  • \$\begingroup\$ Most of the points I would love to review can be found here do check it out \$\endgroup\$ – theProgrammer Dec 2 '20 at 18:52
  • \$\begingroup\$ @theProgrammer thanks, ill check it out ASAP \$\endgroup\$ – fast_and_curious Dec 2 '20 at 18:57
7
\$\begingroup\$

Your board dictionary can be simplified from

board = {1: ' ', 2: ' ', 3: ' ',
         4: ' ', 5: ' ', 6: ' ',
         7: ' ', 8: ' ', 9: ' '}

to a dict comprehension:

board = {n: ' ' for n in range(1, 10)}

Your win_game function can be greatly simplified, from

def win_game(marker, player_id):
    """Function to check for winning combination"""
    if board[1] == marker and board[2] == marker and board[3] == marker or \
            board[4] == marker and board[5] == marker and board[6] == marker or \
            board[7] == marker and board[8] == marker and board[9] == marker or \
            board[1] == marker and board[4] == marker and board[7] == marker or \
            board[2] == marker and board[5] == marker and board[8] == marker or \
            board[3] == marker and board[6] == marker and board[9] == marker or \
            board[1] == marker and board[5] == marker and board[9] == marker or \
            board[3] == marker and board[5] == marker and board[7] == marker:

        print_board()
        time.sleep(1)
        print("Player", player_id, "wins!")
        return True

    else:
        return False

to

def win_game(marker, player_id):
    """Function to check for winning combination"""
    if board[1] == board[2] == board[3] == marker or \
       board[4] == board[5] == board[6] == marker or \
       board[7] == board[8] == board[9] == marker or \
       board[1] == board[4] == board[7] == marker or \
       board[2] == board[5] == board[8] == marker or \
       board[3] == board[6] == board[9] == marker or \
       board[1] == board[5] == board[9] == marker or \
       board[3] == board[5] == board[7] == marker:
       print_board()
       time.sleep(1)
       print("Player", player_id, "wins!")
       return True
    return False

You see, the else statement is not necessary, as the return statement will make the program jump out of the function. Also, as you can probably tell,

a == 1 and b == 1 and c == 1

is the same as

a == b == c === 1

The unnecessary else statement applies to your other functions as well

For get_player_details:

def get_player_details(curr_player):
    """Function to get player identifier and marker"""
    if curr_player == 'A':
        return ['B', 'O']
    else:
        return ['A', 'X']

to

def get_player_details(curr_player):
    """Function to get player identifier and marker"""
    if curr_player == 'A':
        return ['B', 'O']
    return ['A', 'X']

For play_again:

def play_again():
    """Function to check if player wants to play again"""
    print("Do you want to play again?")
    play_again = input()

    if play_again.upper() == 'Y':
        for z in board:
            board[z] = ' '
        return True
    else:
        print("Thanks for playing. See you next time!")
        return False

to

def play_again():
    """Function to check if player wants to play again"""
    print("Do you want to play again?")
    play_again = input()
    if play_again.upper() == 'Y':
        for z in board:
            board[z] = ' '
        return True
    print("Thanks for playing. See you next time!")
    return False

In your print_board function, you can increase the readability of your print statement with a formatted string. Using not instead of == 0 is considered more pythonic:

def print_board():
    """Function to print the board"""
    for i in board:
        print(i, ':', board[i], ' ', end='')
        if i % 3 == 0:
            print()

to

def print_board():
    """Function to print the board"""
    for i in board:
        print(f'{i} : {board[i]}  ', end='')
        if not i % 3:
            print()
\$\endgroup\$
  • \$\begingroup\$ thanks. this is exactly the kind of help I was looking for! \$\endgroup\$ – fast_and_curious Dec 2 '20 at 17:15
  • \$\begingroup\$ @Chocolate Maybe this is better: return ['B', 'O'] if curr_player == 'A' else ['A', 'X'] \$\endgroup\$ – Josh Dec 3 '20 at 9:53
  • \$\begingroup\$ While removing such elses, there's an argument to keep them for better code readability. This is purely a personal preference, though. \$\endgroup\$ – iBug Dec 3 '20 at 17:20
1
\$\begingroup\$

It's good to separate concerns.

play_again() currently does all of the following:

  • collects input.
  • decides if it is 'Y' or not 'Y'.
  • clears the board
  • displays a goodbye
  • tells the caller whether the player wants to restart.

Meanwhile, the caller (the main loop):

  • decides whether to continue (the while loop)
  • resets the current player

I would simplify play_again() to just collect input and return whether it is a Yes or No. (I would make it handle "y" as well as "Y", and maybe loop back and ask again if they don't give an answer that it Y, y, N, n, Q, or q.)

I would move the code to clear the board and reset the current player so they were next to each other, and not in play_again().


Perhaps later, as you understand the concepts of Object Orientation, I would move much of the code into a Board class - it would understand win conditions, how to represent the board as a string, and how to reset the board (or maybe main() would just throw away the instance and construct a new one), without dealing with any of the input or output.


P.S. I'm also not sure how to add an actual board to this game (an actual board rather than the way I have done here)

If that means to have a graphical user interface, rather than the pure text, then it isn't possible just with Python's standard libraries. You would need to find one of the many GUI frameworks that work with Python.

\$\endgroup\$
  • \$\begingroup\$ Thank you for this, will make sure to use your suggestions! \$\endgroup\$ – fast_and_curious Dec 3 '20 at 15:21
1
\$\begingroup\$

All this looking up markers and identifiers calls out for simplification. Just call the players 'X' and 'O' rather than 'A' and 'B'.

You don't need to check all the possible win conditions; if there is a win, it must have involved the current play.

Much of your booleans are unnecessary; rather than setting the boolean to the result of a function, then exiting if True, just exit if the function returns True. Then move the win announcement out of the function checking for a win.

The function name win_game is written as verb, when it's actually an interrogative. check_win is clearer.

The line print("Player", player_id, "wins!") doesn't put spaces in. It will print PlayerAwins! or PlayerBwins!.

def print_board():
    """Function to print the board"""
    for i in board:
        print(i, ':', board[i], ' ', end='')
        if i % 3 == 0:
            print()


def check_win(player, move):
    """Function to check for winning combination"""
    for vertical in range(4):
        if board[(3*vertical + move - 1)%9+1] != player:
            break
    if vertical == 3:
        return True
    for horizontal in range(4):
        square = move + horizontal
        if (square-1)//3 > (move-1)//3:
            square = square - 3
        if board[square] != player:
            break
   if horizontal == 3:
       return True
   #if you don't understand what's going on in the 
   # above lines, work through a few cases
   if ( (board[1] == board[5] == board[9] == player) or
        (board[3] == board[5] == board[7] == player) ):
       return True
   return False


def insert_input(slot_num, player):
    """Function for capturing user inputs"""
    while board[slot_num] != ' ':
        print("spot taken, pick another no.")
        slot_num = int(input())
    board[slot_num] = player


def play_again():
    """Function to check if player wants to play again"""
    print("Do you want to play again?")
    play_again = input()

    if play_again.upper() == 'Y':
        for z in board:
            board[z] = ' '
        return True
    else:
        print("Thanks for playing. See you next time!")
        return False


# Main program
count = 0        
while True:
    count +=1
    print_board()

    print("Player {}: Enter a number between 1 and 9".format(curr_player))
    input_slot = int(input())

    # Inserting 'X' or 'O' in desired spot
    insert_input(input_slot, player)


    # Check if anybody won
    if check_win(player, move):
        #If you really want to keep the names A and B, 
        #insert the following line:
        #player = {'O':'B', 'X','A'}[player]
        print_board()
        time.sleep(1)
        print("Player ", player, " wins!")
        if play_again():
            curr_player = 'X'
            count = 0
        else break    
    if count == 9:
        print("It's a tie!!")
        print_board()
        if play_again():
            curr_player = 'X'
            count = 0
        else break   
\$\endgroup\$
  • \$\begingroup\$ Thanks @Acccumulation for your insight on this! Will make sure to use your suggestions as well 😊 \$\endgroup\$ – fast_and_curious Dec 3 '20 at 15:26

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.