Need Criticism for my first Tic Tac Toe Project

Question

Overview

This is my attempt at a Tic Tac Toe game. I need some criticism and some ideas on what I could do better, because I know somethings need to be cleaned up.

Quick Notes

• I also couldn't figure ways to make the dictionary checks less cluttered and more cleaner(I am noob, pardon this ugly code)

• I did this without any form of help

• This was made and tested on Juipiter Notebook

Code

#Tic Tac Toe: Attempt 2
from IPython.display import clear_output


#for printing the gameboard
def game_board(dictionary):
    print(dictionary['part1'])
    print(dictionary['part2'])
    print(dictionary['part3'])


    
#for creating the gameboard
def create_game_board():
    gboard = {'part1':[' ',' ',' '], 'part2':[' ',' ',' '], 'part3':[' ',' ',' ']}
    return gboard



#player specific charachters
def replacement_value(val):
    if val == 'X':
        val = 'O'
    elif val == 'O':
        val ='X'
    return val



#for choosing which player represents what charachter
def player_value():
    choice_check = False
    player_dict = {'X':'', 'O':''}
    
    while choice_check == False:
        choice = input('Player 1: Are you playing first[Y or N]? ')
        
        if choice.upper() == 'Y' or choice.upper() == 'N':
            if choice.upper() == 'Y':
                player_dict['X'] =  'player1'
                player_dict['O'] =  'player2'
                print('Player 1 goes first')
                choice_check = True
            elif choice.upper() == 'N':
                player_dict['O'] =  'player1'
                player_dict['X'] =  'player2'
                print('Player 2 goes first')
                choice_check = True
        else:
            print('Please type in Y or N')
                
    return player_dict




#for choosing what part of the board should be replaced
def input_logic():
    play_location = 'Not'
    in_index = False
    
    while play_location.isdigit() == False or in_index == False:
        play_location = input('Where do you want to play[Between 1 - 9]: ')
        
        if play_location.isdigit() == True:
            if int(play_location) in list(range(1,10)):
                if int(play_location) > 6:
                    if int(play_location) == 7:
                        if gboard['part3'][0] == ' ':
                            in_index = True 
                        else:
                            print('That position is already taken')
                    elif int(play_location) == 8:
                        if gboard['part3'][1] == ' ':
                            in_index = True 
                        else:
                            print('That position is already taken')
                    elif int(play_location) == 9:
                        if gboard['part3'][2] == ' ':
                            in_index = True 
                        else:
                            print('That position is already taken')
                elif int(play_location) > 3:
                    if int(play_location) == 4:
                        if gboard['part2'][0] == ' ':
                            in_index = True 
                        else:
                            print('That position is already taken')
                    elif int(play_location) == 5:
                        if gboard['part2'][1] == ' ':
                            in_index = True 
                        else:
                            print('That position is already taken')
                    elif int(play_location) == 6:
                        if gboard['part2'][2] == ' ':
                            in_index = True 
                        else:
                            print('That position is already taken')
                elif int(play_location) > 0:
                    if int(play_location) == 1:
                        if gboard['part1'][0] == ' ':
                            in_index = True 
                        else:
                            print('That position is already taken')
                    elif int(play_location) == 2:
                        if gboard['part1'][1] == ' ':
                            in_index = True 
                        else:
                            print('That position is already taken')
                    elif int(play_location) == 3:
                        if gboard['part1'][2] == ' ':
                            in_index = True 
                        else:
                            print('That position is already taken')
            else:
                print('Please make sure your number is between 1 - 9')
        else:
            print('Your input was not a number')
        
    return int(play_location)




#for inputing charachters in the board
def board_input_logic(gboard, play_location, val):

    if play_location > 6:
        if play_location == 7:
            gboard['part3'][0] = val
        elif play_location == 8:
            gboard['part3'][1] = val
        elif play_location == 9:
            gboard['part3'][2] = val
    elif play_location > 3:
        if play_location == 4:
            gboard['part2'][0] = val
        elif play_location == 5:
            gboard['part2'][1] = val
        elif play_location == 6:
            gboard['part2'][2] = val
    elif play_location > 0:
        if play_location == 1:
            gboard['part1'][0] = val
        elif play_location == 2:
            gboard['part1'][1] = val
        elif play_location == 3:
            gboard['part1'][2] = val
#Better Optimization: Use for loops




#checks for winner
def win_check(gboard,player_dict):
    player_list = list(player_dict.values())
    if gboard['part1'][0] == 'X' and gboard['part1'][1] == 'X' and gboard['part1'][2] == 'X' or gboard['part2'][0] == 'X' and gboard['part2'][1] == 'X' and gboard['part2'][2] == 'X' or gboard['part3'][0] == 'X' and gboard['part3'][1] == 'X' and gboard['part3'][2] == 'X' or gboard['part1'][0] == 'X' and gboard['part2'][0] == 'X' and gboard['part3'][0] == 'X' or gboard['part1'][1] == 'X' and gboard['part2'][1] == 'X' and gboard['part3'][1] == 'X' or gboard['part1'][2] == 'X' and gboard['part2'][2] == 'X' and gboard['part3'][2] == 'X' or gboard['part1'][0] == 'X' and gboard['part2'][1] == 'X' and gboard['part3'][2] == 'X' or gboard['part1'][2] == 'X' and gboard['part2'][1] == 'X' and gboard['part3'][0] == 'X':
        print(f'{player_list[0].capitalize()} is the winner!')
        return False
    elif gboard['part1'][0] == 'O' and gboard['part1'][1] == 'O' and gboard['part1'][2] == 'O' or gboard['part2'][0] == 'O' and gboard['part2'][1] == 'O' and gboard['part2'][2] == 'O' or gboard['part3'][0] == 'O' and gboard['part3'][1] == 'O' and gboard['part3'][2] == 'O' or gboard['part1'][0] == 'O' and gboard['part2'][0] == 'O' and gboard['part3'][0] == 'O' or gboard['part1'][1] == 'O' and gboard['part2'][1] == 'O' and gboard['part3'][1] == 'O' or gboard['part1'][2] == 'O' and gboard['part2'][2] == 'O' and gboard['part3'][2] == 'O' or gboard['part1'][0] == 'O' and gboard['part2'][1] == 'O' and gboard['part3'][2] == 'O' or gboard['part1'][2] == 'O' and gboard['part2'][1] == 'O' and gboard['part3'][0] == 'O':
        print(f'{player_list[1].capitalize()} is the winner!')
        return False
    elif gboard['part1'][0] != ' ' and gboard['part1'][1] != ' ' and gboard['part1'][2] != ' ' and gboard['part2'][0] != ' ' and gboard['part2'][1] != ' ' and gboard['part2'][2] != ' ' and gboard['part3'][0] != ' ' and gboard['part3'][1] != ' ' and gboard['part3'][2] != ' ':
        print("It's a tie")
        return False
    else:
        return True
#couldnt figure out how to make code more compact, for-loops maybe?




#Checks if game should be continued
def continue_game():
    running = True
    ans_list = ['Y', 'N']
    
    while running:
        cont = input('Would you like to continue[Y or N]? ')
        
        if cont.upper() in ans_list:
            if cont.upper() == 'Y':
                return True
            else:
                return False
        else:
            print('Please type in "Y" or "N"')

            
            
            
#game loop
#variables
gboard = create_game_board()
player_dict = player_value()
game_loop = True
val = 'O'
running = True

#enclosing loop for repeatability
while running == True:
    
    #actual loop for game
    while game_loop == True:
        val = replacement_value(val)
        play_location = input_logic()
        game_board(gboard)
        board_input_logic(gboard,play_location,val)
        clear_output()
        game_board(gboard)
        game_loop = win_check(gboard, player_dict)
    
    #deciding factor for repeating game
    game_loop = continue_game()
    if game_loop == True:
        gboard = create_game_board()
        player_dict = player_value()
        val = 'O'
        continue
    #breaking out from game(AKA - End)
    else:
        clear_output()
        print('Thanks for playing!')
        break

Any improvements/better ideas are much appreciated

Answer 1

You did a great job at making the code clear and also provide a good user experience for the players by providing clear messages to the user.

How can you make this better? Enable other developers from other teams to write tic-tac-toe applications for Desktop, Web, Android and iOS, using your code.

But right now your game works only in Console app.

Can you think about how you can restructure your code to make this possible? What code needs to be separated?

After you restructure your code, your Console app should continue working and other developers should be able to create a tic-tac-toe game for the favorite platform.

Answer 2

Comments can be a two edged sword. They should clarify why you're doing what you're doing. They shouldn't repeat what the code is doing. So I would rename create_game_board() to get_new_board() and drop the comment. As another step, instead of a comment saying that game_board(dictionary) will print the board, just name it print_board(board). win_check should more clearly indicate that the game is continuing.

Why are you using a dictionary for your board? Why not a list of lists? gboard = [ [' ',' ',' '], [' ',' ',' '], [' ',' ',' '] ], and then you can simplify a lot of your later logic. You could also consider changing the inner ones to [' ']*3, and maybe even gboard = [ [' ']*3 for _ in (0,1,2) ]. This also lets you translate from the 1-9 location to the [y][x] location: L=3*y+x+1 and y,x=divmod(L-1,3).

There are a lot of places you can avoid a literal True or False. if condition==True should be just if condition. if condition==False should be if not condition. if condition: return True else: return False should be return condition.

Several of your functions have a variable that ends up being set, a sentinal boolean to tell if that variable has been set, and then at the end of the function you return the variable. It makes things a lot simpler to just return the variable that you're interested in, and skip the boolean.

int(play_location) in list(range(1,10)) can be changed to 1<=int(play_location)<=9.

At a minimum, win_check needs more parentheses so that it's clear where the and and or group. But we can use the same idea of the previous item to change gboard[0][0] == 'X' and gboard[0][1] == 'X' and gboard[0][2] == 'X' into gboard[0][0] == gboard[0][1] == gboard[0][2] == 'X'. A better approach could be to use sets and change that into set(gboard[0])=={'X'}. To check the columns, you can transpose the board: tran = [*zip(*gboard)], and check the rows of the transpose. And now that we've figured out how to check a row in a single command, we can group several rows into one: any( set(row)=={'X'} for row in gboard ). And we can have gboard+transposed in that last to check the columns at the same time.

We can also use the any idea to look for ties: if ' ' is in any row, then we're not done: if not any( ' ' in row for row in gboard ).

player_list[0] is using that dict.values() uses the same order as the assignment. This was an implementation quirk before Python 3.7. It would be better to get the key and value with .items(). And since the key is X or O, we can use that instead of hardcoding the values.

You never change running from being True. Just put True, and remove running. You can also get rid of game_loop by slightly adjusting things.

player_value has: if A or B: (if A: elif B: ) else:. That is easier to read as if A: elif B: else:.

Instead of the function replacement_value, I would go with a dict: {'X':'O','O':'X'}. But that may be more of a preference.

That gets us here:

#Tic Tac Toe: Attempt 2
from IPython.display import clear_output

def print_board(board):
    for row in board:
        print(row)

def get_new_board():
    gboard = [ [' ']*3 for _ in (0,1,2) ]
    return gboard

#player specific characters
next_val {
    'X': 'O',
    'O': 'X'
}

#for choosing which player represents what charachter
def player_value():
    while True:
        choice = input('Player 1: Are you playing first[Y or N]? ')
        if choice.upper() == 'Y':
            print('Player 1 goes first')
            return { 'X': 'player1', 'O': 'player2' }
        elif choice.upper() == 'N':
            print('Player 2 goes first')
            return { 'O': 'player1', 'X': 'player2' }
        else:
            print('Please type in Y or N')

#for choosing what part of the board should be replaced
def input_logic():
    while True:
        play_location = input('Where do you want to play[Between 1 - 9]: ')
        if play_location.isdigit() and 1 <= int(play_location) <= 9:
            y,x = divmod( int(play_location)-1, 3)
            if gboard[y][x] == ' ':
                return int(play_location)
            else:
                print('That position is already taken')
        else:
            print('Please make sure your number is between 1 - 9')

#for inputing charachters in the board
def board_input_logic(gboard, play_location, val):
    y,x = divmod( play_location-1, 3 )
    gboard[y][x] = val

def game_continues(gboard,player_dict):
    transposed = [*zip(*gboard)]
    for value, player in player_dict.items():
        if any( set(row)=={value} for row in gboard+transposed ) or gboard[0][0] == gboard[1][1] == gboard[2][2] == value or gboard[0][2] == gboard[1][1] == gboard[2][0] == value:
            print(f'{player.capitalize()} is the winner!')
            return False
        elif not any( ' ' in row for row in gboard ):
            print("It's a tie")
            return False
    else:
        return True

def continue_game():
    while True:
        cont = input('Would you like to continue[Y or N]? ')
        if cont.upper() in ('Y','N'):
            return cont.upper() == 'Y'
        else:
            print('Please type in "Y" or "N"')

gboard = get_new_board()
player_dict = player_value()
val = 'O'

while True: # each game
    while True: # each turn
        val = next_val[val]
        play_location = input_logic()
        print_board(gboard)
        board_input_logic(gboard,play_location,val)
        clear_output()
        print_board(gboard)
        if not game_continues(gboard, player_dict):
            break
    
    if continue_game():
        gboard = get_new_board()
        player_dict = player_value()
        val = 'O'
    else:
        clear_output()
        print('Thanks for playing!')
        break

Some final items:

I don't understand your distinction between board_input_logic and input_logic. Now that gboard is a list of lists, these are much shorter and run up against each other. It would be good to rename them to clarify what they're doing.

I'm not using IPython, so clear_output isn't doing anything. But it looks like your first call to print_board is immediately cleared.

You could consider creating a class for the board. And possibly for the players as well.