3
\$\begingroup\$

I started a simple OOP GUI project with tkinter in Python since I've been studying the basics for a while, but I'm not sure if my code is properly structured and following the programming conventions. I haven't finished it yet but the sooner I learn the good practices the better. My goal is to make a mini game. For now it only has a main window and a character selection window with some widgets in it. (For didactic purposes, I want it to be done with an OOP approach).

import tkinter as tk
from itertools import product


# Creating main page
class MainApplication(tk.Frame):
    def __init__(self, master, *args, **kwargs):
        tk.Frame.__init__(self, master, *args, **kwargs)
        self.master = master

        # produce the set of coordinates of the buttons
        main_positions = product(range(2), range(5))

        # Creating main page widgets
        frame = tk.Frame(root, bg="#34495e")
        frame.grid()

        lb = tk.Label(frame, width=111, height=4, bg="#2c3e50", text="Champions", fg="white", font=50,
                      justify=tk.CENTER)
        lb.grid(row=0, column=0, columnspan=5, pady=(0, 50), sticky="snew")

        # Creating multiple buttons
        for i, item in enumerate(main_positions):
            button_main = tk.Button(frame, width=16, height=8, bg="#2c3e50", relief=tk.RIDGE, fg="white",
                                    justify=tk.CENTER, text=f"Champion {item[1] + 1}",
                                    command=ChampionWindow.champions_window)

            button_main.grid(row=item[0] + 1, column=item[1], pady=(0, 50))

        button = tk.Button(frame, width=30, height=3, bg="#2c3e50", relief=tk.RIDGE, text="Done", fg="white", font=20,
                           command=root.destroy)

        button.grid(row=3, columnspan=5, pady=(0, 150))


# Creating champion select window
class ChampionWindow(tk.Frame):
    def __init__(self, master, *args, **kwargs):
        tk.Frame.__init__(self, master, *args, **kwargs)
        self.master = master

    @staticmethod
    def champions_window():
        ch_window = tk.Tk()
        ch_window.title("Champion Select")
        ch_window.resizable(False, False)
        ch_window.configure(background="#192a56")

        champion_position = product(range(30), range(5))

        def scroll(ch_event):
            ch_canvas.configure(scrollregion=ch_canvas.bbox("all"))

        def select_champion():
            pass

        ch_canvas = tk.Canvas(ch_window, bg="blue", width=470, height=500)
        ch_frame = tk.Frame(ch_canvas, bg="#273c75")

        vscrollbar = tk.Scrollbar(ch_window, orient="vertical", command=ch_canvas.yview)

        ch_canvas.configure(yscrollcommand=vscrollbar.set)
        ch_canvas.grid(sticky="snew")

        vscrollbar.grid(row=0, column=3, sticky="sn")

        ch_canvas.create_window((0, 0), window=ch_frame, anchor="nw")
        ch_frame.bind("<Configure>", scroll)

        # Creating multiple buttons
        for x, itm in enumerate(champion_position):
            btn = tk.Button(ch_frame, width=12, height=6, bg="#2c3e50", relief=tk.RIDGE, fg="white", justify=tk.CENTER,
                            command=select_champion)

            btn["text"] = f"Pick champ {x+1}"
            btn.grid(row=itm[0], column=itm[1])


if __name__ == "__main__":
    root = tk.Tk()
    MainApplication(root)
    root.configure(background="#273c75")
    root.geometry("1000x570+450+200")
    root.resizable(False, False)
    root.title("Champion")
    root.mainloop()

Images:

Main Window
- The buttons open the character selection window

Character Selection Window (with scrollbar)
- In the future the buttons will pick the selected character that will be represented by an image with PIL library.

\$\endgroup\$

2 Answers 2

Reset to default
3
\$\begingroup\$

Don't create more than one instance of Tk

You should only ever create exactly one instance of Tk. If you need to make more windows, useToplevel`.

You aren't using inheritance properly

This code is wrong:

class MainApplication(tk.Frame):
    def __init__(self, master, *args, **kwargs):
        tk.Frame.__init__(self, master, *args, **kwargs)
        ...
        # Creating main page widgets
        frame = tk.Frame(root, bg="#34495e")
        ...
        lb = tk.Label(frame, ...)

First, you inherit from tk.Frame. This is good, in that when done properly you can treat MainApplication as if it were a standalone widget. However, the first thing you do after creating this frame is to create another frame as a child of root. That makes no sense since the object is itself already a frame. It completely negates any advantage of inheriting from tk.Frame.

The proper way to do this is to not create the "main page widgets" frame. Just remove that, and then put all widgets created by MainApplication inside self. The basic structure should look like this:

class MainApplication(tk.Frame):
    def __init__(self, master, *args, **kwargs):
        tk.Frame.__init__(self, master, *args, **kwargs):

        lb = tk.Label(self, ...)
        ...
            button_main = tk.Button(self, ...)
        ...
        button = tk.Button(self, ...)
        ...

if __name__ == "__main__":
    root = tk.Tk()
    app = MainApplication(root)
    # app is a frame, so add it to root. Since it's the 
    # only widget in root, pack is the simplest method 
    # to add it to the root window
    app.pack(fill="both", expand=True)

With that, everything in MainApplication is encapsulated inside the MainApplication frame, Nothing is leaking out into other widgets.

Don't inherit from Frame if it's not a frame

Your ChampionWindow inherits from Frame, but I can't see any reason to do so based on the way you coded it. You never use an instance of ChampionsWindow anywhere so there's no point in inheriting from anything.

Since the only purpose of ChampionWindow appears to be to hold a static method which creates a new window, I recommend inheriting from Toplevel and moving all code from the static method into a normal method called from __init__.

Don't remove control from the user

This is more of a personal preference, but I don't see any reason to remove the user's ability to resize the window with root.resizable(False, False). Usually, when I see that, it's a sign you didn't take the time to make your UI responsive. That seems to be the case here - the UI doesn't respond properly to resizing. Instead of just removing that ability, I recommend doing a few things to make the UI respond properly to resizing.

\$\endgroup\$
1
  • \$\begingroup\$ Thank you very much for your review, it helped me a lot! \$\endgroup\$ Commented Apr 15, 2020 at 16:02
2
\$\begingroup\$

Sadly, I am not familiar with tkinter but have used QT a bit and some concepts are similar. You will be interested in reading more about the layout managers for resizable windows and controls - see Layout management in Tkinter. You are not exploiting their full potential.

I would always allow resize and never expect that the screen dimensions will always remain the same. And it takes very little to spoil your layout - just changing font could have a dramatic effect. Grid layouts exist for a reason, so use them.

Hardcoded colors like #273c75 should be defined as variables at the top op your code, possibly in a dedicated module if you end up with multiple files. But a configuration file would be nice. Because it is the kind of things that users will want to customize. Some people have poor sight and prefer high contrast, and the colors are not rendered the same on every screen.

So #34495e could be named BLUE_GRAY or something along these lines (use meaningful and descriptive names). I have no idea what #34495e looks like otherwise.

Same remark for fg="white": don't use hardcoded colors. Everything that is related to the customization of controls like padding, font size etc should be defined in a dedicated section or separate file. But I still prefer a configuration file of some sort.

Same here:

champion_position = product(range(30), range(5))

Don't use fixed ranges in code, add some level of abstraction (and flexibility).

In programming it is important to separate logic from presentation as much as possible.

\$\endgroup\$
1
  • \$\begingroup\$ Thank you very much for your review, it was really useful! I'll be more careful with the presentation. \$\endgroup\$ Commented Apr 15, 2020 at 20:49

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.