2
\$\begingroup\$

I am learning to program in Python and as my first project I decided to create a simple Tic Tac Toe game. The game is working properly now and I am wondering if there is anything I could do to improve my code.

Notes:

  1. In my original code I am using pictures on buttons that allow to choose players, to draw moves and to cross out the winning combination. I have changed this in the code that is posted below to improve reviewer experience.

  2. While posting the code here on code review in noticed that some lines of my code exceed 79 char limit - sorry about that. I hope it won't get in the way too much.

import random

from tkinter import *
from tkinter import ttk


def main_window():
    root.title('Tic Tac Toe')
    remove_widgets()
    global frame

    frame = ttk.Frame(root, width=250, height=150, relief='groove')
    frame.pack_propagate(False)
    frame.pack(padx=25, pady=75)

    play = ttk.Button(frame, text='Play', command=lambda: play_menu(do=0))
    play.pack(side='top', pady=(50, 0))

    qb = ttk.Button(frame, text="Quit", command=root.destroy)
    qb.pack(side='top', pady=(0, 50))


def play_menu(do):
    root.title('Tic Tac Toe')
    remove_widgets()
    if do == 'redeclare':
    redeclare_vars()

    global frame

    label = ttk.Label(root, text='Choose your side', font=('French Script MT', 20))
    label.pack(side='top', pady=(25, 0))

    frame = ttk.Frame(root, width=250, height=150, relief='groove')
    frame.pack_propagate(False)
    frame.pack(padx=25, pady=25)

    player_x = ttk.Button(frame, text='X', command=lambda: game(pl='X'))
    player_x.grid(column=0, row=0, padx=(5, 0), pady=(5, 0))

    player_o = ttk.Button(frame, text='O', command=lambda: game(pl='O'))
    player_o.grid(column=1, row=0, padx=(0, 5), pady=(5, 0))

    back = ttk.Button(frame, text='Back', command=main_window)
    back.grid(column=0, row=1, sticky=(E, W), padx=(5, 5), pady=(0, 5), columnspan=2)


def game(pl=None):
    root.title('Tic Tac Toe')
    remove_widgets()

    global frame, canvas, player, computer, move, stop_game

    frame = ttk.Frame(root, width=650, height=700)
    frame.pack_propagate(False)
    frame.pack()

    canvas = Canvas(frame, width=600, height=600)
    canvas.pack(side='top', pady=(25, 0))

    restart = ttk.Button(frame, text='Restart', command=lambda: play_menu(do='redeclare'))
    restart.pack(side='bottom', pady=20)

    draw_board()
    canvas.bind('<Button-1>', square_selector)

    if pl == 'X':
        player = 'X'
        computer = 'O'
        move = 'player'
    elif pl == 'O':
        player = 'O'
        computer = 'X'
        move = 'computer'
        computer_move()


def remove_widgets():
    for widget in root.winfo_children():
        widget.destroy()


def square_status_lib(square):
    global statusLib, player
    status = None

    if player == 'X':
        status = 'X'
    elif player == 'O':
        status = 'O'
    statusLib[square-1] = status


def square_selector(event):
    if 1 <= event.x <= 199:
        if 1 <= event.y <= 199:
            player_move(square=1)
    elif 1 <= event.x <= 199:
        if 201 <= event.y <= 399:
            player_move(square=2)
    elif 1 <= event.x <= 199:
        if 401 <= event.y <= 599:
            player_move(square=3)
    elif 201 <= event.x <= 399:
        if 1 <= event.y <= 199:
            player_move(square=4)
    elif 201 <= event.x <= 399:
        if 201 <= event.y <= 399:
            player_move(square=5)
    elif 201 <= event.x <= 399:
        if 401 <= event.y <= 599:
            player_move(square=6)
    elif 401 <= event.x <= 599:
        if 1 <= event.y <= 199:
            player_move(square=7)
    elif 401 <= event.x <= 599:
        if 201 <= event.y <= 399:
            player_move(square=8)
    elif 401 <= event.x <= 599:
        if 401 <= event.y <= 599:
            player_move(square=9)


def computer_move():
    global move, x1, y1, x2, y2
    status, a = 0, 0

    while status is not None:
        a = random.randint(1, 9)
        status = statusLib[a-1]
        x1, y1, x2, y2 = squareLib[a - 1][0], squareLib[a - 1][1], squareLib[a - 1][2], squareLib[a - 1][3]
    if computer == 'X':
        draw_move()
        statusLib[a-1] = 'X'
    elif computer == 'O':
        draw_move()
        statusLib[a-1] = 'O'
    end_game()
    if not stop_game:
        move = 'player'


def player_move(square):
    global x1, y1, x2, y2, move, squareLib, stop_game

    if statusLib[square-1] is None:
        x1, y1, x2, y2 = squareLib[square-1][0], squareLib[square-1][1], squareLib[square-1][2], squareLib[square-1][3]
        draw_move()
        square_status_lib(square=square)
        end_game()
        if not stop_game:
            move = 'computer'
            computer_move()


def draw_move():
    global player, x1, y1, x2, y2, canvas, move

    if move == 'player':
        if player == 'X':
            canvas.create_line(x1, y1, x2, y2)
            canvas.create_line(x1, y2, x2, y1)
        elif player == 'O':
            canvas.create_oval(x1, y1, x2, y2)
    elif move == 'computer':
        if computer == 'X':
            canvas.create_line(x1, y1, x2, y2)
            canvas.create_line(x1, y2, x2, y1)
        elif computer == 'O':
            canvas.create_oval(x1, y1, x2, y2)


def draw_board():
    global canvas
    canvas.create_line(0, 200, 600, 200)
    canvas.create_line(0, 400, 600, 400)
    canvas.create_line(200, 0, 200, 600)
    canvas.create_line(400, 0, 400, 600)


def check_end_game():
    global tie, stop_game, fin

    if statusLib[0] == statusLib[1] == statusLib[2] == 'X' or statusLib[0] == statusLib[1] == statusLib[2] == 'O':
        stop_game, fin = True, 1
    elif statusLib[3] == statusLib[4] == statusLib[5] == 'X' or statusLib[3] == statusLib[4] == statusLib[5] == 'O':
        stop_game, fin = True, 2
    elif statusLib[6] == statusLib[7] == statusLib[8] == 'X' or statusLib[6] == statusLib[7] == statusLib[8] == 'O':
        stop_game, fin = True, 3
    elif statusLib[0] == statusLib[3] == statusLib[6] == 'X' or statusLib[0] == statusLib[3] == statusLib[6] == 'O':
        stop_game, fin = True, 4
    elif statusLib[1] == statusLib[4] == statusLib[7] == 'X' or statusLib[1] == statusLib[4] == statusLib[7] == 'O':
        stop_game, fin = True, 5
    elif statusLib[2] == statusLib[5] == statusLib[8] == 'X' or statusLib[2] == statusLib[5] == statusLib[8] == 'O':
        stop_game, fin = True, 6
    elif statusLib[2] == statusLib[4] == statusLib[6] == 'X' or statusLib[2] == statusLib[4] == statusLib[6] == 'O':
        stop_game, fin = True, 7
    elif statusLib[0] == statusLib[4] == statusLib[8] == 'X' or statusLib[0] == statusLib[4] == statusLib[8] == 'O':
        stop_game, fin = True, 8
    elif all(k is not None for k in statusLib):
        stop_game, tie, fin = True, True, 0
    else:
        stop_game, fin = False, 0


def end_game():
    global stop_game, tie, canvas
    check_end_game()
    text = ''

    if stop_game:
        canvas.unbind('<Button-1>')
        if move == 'player' and not tie:
            text = 'You win'
        elif move == 'computer' and not tie:
            text = 'You lose'
        elif tie:
            text = 'It\'s a tie'

        finisher()
        canvas.create_text(300, 300, text=text, font=('French Script MT', 50), fill='#000000')

    elif not stop_game:
        pass


def finisher():
    global fin
    x3, y3, x4, y4 = 0, 0, 0, 0

    if fin != 0:
        if fin == 1:
            x3, y3, x4, y4 = 100, 100, 100, 500
        elif fin == 2:
            x3, y3, x4, y4 = 300, 100, 300, 500
        elif fin == 3:
            x3, y3, x4, y4 = 500, 100, 500, 500
        elif fin == 4:
            x3, y3, x4, y4 = 100, 100, 500, 100
        elif fin == 5:
            x3, y3, x4, y4 = 100, 300, 500, 300
        elif fin == 6:
            x3, y3, x4, y4 = 100, 500, 500, 500
        elif fin == 7:
            x3, y3, x4, y4 = 100, 500, 500, 100
        elif fin == 8:
            x3, y3, x4, y4 = 100, 100, 500, 500
        canvas.create_line(x3, y3, x4, y4)
    elif fin == 0:
        pass


def redeclare_vars():
    global statusLib, myVal, x1, x2, y1, y2, canvas, move, fin, stop_game, tie
    statusLib = []
    myVal = None
    for l in range(0, 9):
        statusLib.append(myVal)
    x1, y1, x2, y2, canvas, move, fin = 0, 0, 0, 0, None, '', 0
    stop_game, tie = False, False


root = Tk()
root.minsize(width=300, height=300)

statusLib = []
myVal = None
for i in range(0, 9):
    statusLib.append(myVal)
x1, y1, x2, y2, canvas, move, fin = 0, 0, 0, 0, None, '', 0
stop_game, tie = False, False
game_mode = IntVar()

squareLib = [
             [20, 20, 180, 180],
             [20, 220, 180, 380],
             [20, 420, 180, 580],
             [220, 20, 380, 180],
             [220, 220, 380, 380],
             [220, 420, 380, 580],
             [420, 20, 580, 180],
             [420, 220, 580, 380],
             [420, 420, 580, 580]
            ]

main_window()
root.mainloop()

I also do have some specific questions about what I could change:

  1. I have noticed that many tkinter applications posted on the internet as examples make use of classes. Is there any specific reason to do that? tkinter applications do seem to work fine without that and the code gets shorter without classes.

  2. In my app I am clearing the main tkinter window from any widgets whenever I go to a different menu. It seems a little bit strange to destroy and recreate objects while moving between screens, but I couldn't figure out a different solution. Is my approach a good practice?

  3. Should I create main() function to wrap up all the remaining code at the bottom of the program?

\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review. Don't worry about the code exceeding 79 characters, that might get addressed by a reviewer, though. \$\endgroup\$ – Zeta Jan 18 at 15:16
  • \$\begingroup\$ "The game is working properly now " - it doesn't seem to work properly for me. It seems to only allow me to click in the top row of boxes. \$\endgroup\$ – Bryan Oakley Jan 18 at 18:20
  • \$\begingroup\$ Oh, thanks for pointing this out Bryan. I have recoded this part before posting (have been using some aditional stuff to be able to resize the game window to my liking) and probably forgot to fix the copy-pasted parts. I'll edit my post asap. \$\endgroup\$ – Aivaras Kazakevičius Jan 19 at 16:21
  • \$\begingroup\$ I fixed the function square_selector and also one place were the indentation was missing. Now the game seems to work fine for me. Looking forward to more comments. \$\endgroup\$ – Aivaras Kazakevičius Jan 19 at 16:31
  • \$\begingroup\$ I rolled back your last couple edits. After getting an answer you are not allowed to change your code anymore. This is to ensure that answers do not get invalidated and have to hit a moving target. If you have changed your code you can either post it as an answer (if it would constitute a code review) or ask a new question with your changed code (linking back to this one as reference). Refer to this post for more information \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Jan 31 at 16:47
1
\$\begingroup\$

I think I found the problem Bryan Oakley mentionned with the impossibilty to play on any row other than the first one.

The square_selector(event) function is flawed: you check the position of the event by column but you always check for x before EACH y.

if 1 <= event.x <= 199:
        if 1 <= event.y <= 199:
            player_move(square=1)
elif 1 <= event.x <= 199:
        if 201 <= event.y <= 399:
            player_move(square=2)
elif 1 <= event.x <= 199:
        if 401 <= event.y <= 599:
            player_move(square=3)

But you never reach the elif statements. You should check for an x position, if true -> check EACH y position for that same x position. If the first check for y evaluates false, check the next y, in the same block. Like this :

if 1 <= event.x <= 199:
        if 1 <= event.y <= 199:
            player_move(square=1)
        elif 201 <= event.y <= 399:
            player_move(square=2)
        elif 401 <= event.y <= 599:
            player_move(square=3)
\$\endgroup\$
  • \$\begingroup\$ Yup, you are right @Clepsyd. I changed some things in this function before posting and didn't test it out. Now I changed all elif statments back to if statments in the square_selector() and the game seems to be working properly. Thanks for your note. \$\endgroup\$ – Aivaras Kazakevičius Jan 19 at 16:35

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.