5
\$\begingroup\$

I'm thinking of making a little strategic game using tkinter (player should interact with it using buttons). But my code is very repetitive. This is a mock-up of my program:

import tkinter as tk
import random


def multi(*args):
    for func in args:
        func()


def show_hide():
    if not button["text"]:
        button.configure(text="Perform action #1", bd=2, bg="white", command=activate_deactivate)
        status_label["text"] = "Status: {}".format(status.get())
        button_2.configure(text="Perform action #2", bd=2, bg="white", command=activate_deactivate_2)
        status_label_2["text"] = "Status: {}".format(status_2.get())
    else:
        button.configure(text="", bd=0, bg="#F0F0F0", command=None)
        status_label["text"] = ""
        button_2.configure(text="", bd=0, bg="#F0F0F0", command=None)
        status_label_2["text"] = ""


def activate_deactivate():
    if status.get() == "Can be done":
        status.set("To be done")
        status_label.configure(text="Status: {}".format(status.get()), fg="blue")
    else:
        status.set("Can be done")
        status_label.configure(text="Status: {}".format(status.get()), fg="black")


def activate_deactivate_2():
    if status_2.get() == "Can be done":
        status_2.set("To be done")
        status_label_2.configure(text="Status: {}".format(status.get()), fg="blue")
    else:
        status_2.set("Can be done")
        status_label_2.configure(text="Status: {}".format(status.get()), fg="black")


def step_forward():
    if status.get() == "To be done":
        button.configure(text="", bd=0, bg="#F0F0F0", state="disabled")
        status_label["text"] = ""
        result = random.choice(["success", "failure"])
        if result == "success":
            status.set("Accomplished")
            status_label["fg"] = "green"
        else:
            status.set("Failed")
            status_label["fg"] = "red"
    else:
        button.configure(text="", bd=0, bg="#F0F0F0", command=None)
        status_label["text"] = ""


def step_forward_2():
    if status_2.get() == "To be done":
        button_2.configure(text="", bd=0, bg="#F0F0F0", state="disabled")
        status_label_2["text"] = ""
        result = random.choice(["success", "failure"])
        if result == "success":
            status_2.set("Accomplished")
            status_label_2["fg"] = "green"
        else:
            status_2.set("Failed")
            status_label_2["fg"] = "red"
    else:
        button_2.configure(text="", bd=0, bg="#F0F0F0", command=None)
        status_label_2["text"] = ""


root = tk.Tk()

status = tk.StringVar()
status.set("Can be done")

status_2 = tk.StringVar()
status_2.set("Can be done")

main = tk.Button(root, text="Show/Hide", bg="white", font="courier 30", command=show_hide)
main.pack()

frame = tk.Frame(root, pady=10)
frame.pack()

frame_1 = tk.Frame(frame, padx=10)
frame_1.pack(side="left")

frame_2 = tk.Frame(frame, padx=10)
frame_2.pack(side="left")

button = tk.Button(frame_1, text="", font="courier 20", bd=0)
button.grid(row=0, column=0)

status_label = tk.Label(frame_1, text="", font="courier 14")
status_label.grid(row=1, column=0)

button_2 = tk.Button(frame_2, text="", font="courier 20", bd=0)
button_2.grid(row=0, column=1)

status_label_2 = tk.Label(frame_2, text="", font="courier 14")
status_label_2.grid(row=1, column=1)

next_day = tk.Button(root, text="Next day", bg="white", font="courier 30",
                     command=lambda: multi(step_forward, step_forward_2))
next_day.pack()

root.mainloop()

This definitely looks terrible. With blank lines, it takes 100+ lines of code just to make 2 buttons (well, it's not only "2 buttons"), but I want to have many of these (a few groups each one containing around 4+ buttons which is a lot). If you know how "this" can be changed, I'd love to hear any suggestions (although this code is bad, it perfectly represents functionality and look of the interactive part of the UI, so these should remain the same).

\$\endgroup\$
  • \$\begingroup\$ I'd definitely make it object oriented with classes, I'm not the best with Tkinter, but it'd be a lot more organized. \$\endgroup\$ – George Nov 4 '16 at 14:15
3
\$\begingroup\$

The main issues, with your code, are that:

  1. you do not factorize part of your code that are similar by correctly parametrizing your functions; and
  2. you do not manage any common state between entities that are related.

But first, let's talk a bit about multi and __name__.

First, you can remove the need for a lambda by having multi return a callable that you will be able to provide to the command parameter. Writting something allong the lines of ..., command=multi(step_forward, step_forward_2)):

def combine(*callables):
    def runner():
        for function in callables:
            function()
    return runner

I also changed names, I feel these are more explicit.

Second, you should wrap your top-level code under an if __name__ == '__main__': clause. That way, if I want to use the building blocks you provide, I can import your file and build around it without having your UI pop up.

Now onto simplifying your code.

There is at least 3 things you are trying to achieve all at once:

  1. Build logical blocks such as a button and a label in a frame;
  2. Interact with such blocks from "the outside"; I believe the show/hide should be more generic than the "next step" one, but not knowing what you want to achieve with such UI, I can be misleading;
  3. Manage states and control flow to have all your parts work together.

The first part is pretty straightforward: if you want several elements that behave like a single entity, you build a class to hold these elements together. Having done that, the second one comes naturally: you provide methods on your class so that "the outside" can interact with it. Managing states are thus nearly done: combine fed by instance method is very helpful. You only need to set it as a command on UI elements once each of them has been built.

In the following improvements, you’ll see that I provide several level of customization to be able to interact with the group of item both from the inside and the outside. Depending on your needs, you can merge some or detail even more:

import tkinter as tk
import random


class ButtonWithStatus:
    def __init__(self, root, pack_side, frame_config=None, button_config=None, label_config=None):
        if frame_config is None:
            frame_config = {}
        self._status_config = {} if label_config is None else label_config
        self._button_config = {} if button_config is None else button_config
        self._button_config['command'] = self.on_click
        self.hidden = True

        frame = tk.Frame(root, **frame_config)
        frame.pack(side=pack_side)
        self._hidden_background_color = frame.cget('bg')
        # Build hidden button
        parameters = self.hidden_parameters(self._button_config)
        self.button = tk.Button(frame, **parameters)
        self.button.grid(row=0, column=0)
        # Build label with hidden text
        parameters = self.hidden_parameters(self._status_config, False)
        self.label = tk.Label(frame, **parameters)
        self.label.grid(row=1, column=0)

    def hidden_parameters(self, configuration, is_button=True):
        parameters = configuration.copy()
        parameters.update(text='', bg=self._hidden_background_color)
        if is_button:
            parameters['command'] = None
        return parameters

    def on_click(self):
        raise NotImplementedError

    def show_hide(self):
        if self.hidden:
            self.button.configure(**self._button_config)
            self.label.configure(**self._status_config)
        else:
            parameters = self.hidden_parameters(self._button_config)
            self.button.configure(**parameters)
            parameters = self.hidden_parameters(self._status_config, False)
            self.label.configure(**parameters)
        self.hidden = not self.hidden


class ActivableButton(ButtonWithStatus):
    STATUS = 'Status: {}'

    def __init__(self, root, button_text, inactive_status, active_status):
        super().__init__(root, 'left', {'padx': 10},
                         {'text': button_text, 'font': 'courier 20', 'bd': 0},
                         {'text': self.STATUS.format(inactive_status), 'font': 'courier 14'})
        self.activated = False
        self._active = active_status
        self._inactive = inactive_status

    def on_click(self):
        self.activated = not self.activated
        if self.activated:
            self._status_config.update(text=self.STATUS.format(self._active), fg='blue')
        else:
            self._status_config.update(text=self.STATUS.format(self._inactive), fg='black')
        self.label.configure(**self._status_config)


class ValidableActivableButton(ActivableButton):
    def __init__(self, root, button_text):
        super().__init__(root, button_text, 'Can be done', 'To be done')
        self._done = False

    def step_forward(self):
        if self._done:
            return

        if self.activated:
            self._button_config.update(text='', bd=0, bg='#F0F0F0', state='disabled')
            if random.randint(0, 1):
                self._status_config.update(text='Accomplished', fg='green')
            else:
                self._status_config.update(text='Failed', fg='red')
            self._done = True
        self.button.configure(**self._button_config)
        self.label.configure(**self._status_config)


def combine(*callables):
    def runner():
        for function in callables:
            function()
    return runner


if __name__ == '__main__':
    root = tk.Tk()
    main = tk.Button(root, text="Show/Hide", bg="white", font="courier 30")
    main.pack()

    frame = tk.Frame(root, pady=10)
    frame.pack()

    button1 = ValidableActivableButton(frame, 'Perform action #1')
    button2 = ValidableActivableButton(frame, 'Perform action #2')

    main['command'] = combine(button1.show_hide, button2.show_hide)

    next_day = tk.Button(root, text="Next day", bg="white", font="courier 30",
                         command=combine(button1.step_forward, button2.step_forward))
    next_day.pack()

    root.mainloop()

Several other improvements:

  • usage of booleans instead of text to determine toggable state;
  • usage of the "default" background color of the container to determine the background color of the hidden state;
  • usage of dictionaries to store/restore widget configuration.

Depending on your needs, you may want to add/remove some of the parameter of the constructors. Or even combine ValidableActivableButton and ActivableButton into a single class.


You also don't really need to manage the hidden state yourself by removing text (and optionnally changing background color) as your can hide widgets pretty easily. This allows you to manipulate your widgets directly or using configure without worying about storing their state for future hide/show:

import tkinter as tk
import random


class ButtonWithStatus:
    def __init__(self, root, pack_side, frame_config=None, button_config=None, label_config=None):
        if frame_config is None:
            frame_config = {}
        if button_config is None:
            button_config = {}
        if label_config is None:
            label_config = {}
        button_config['command'] = self.on_click

        frame = tk.Frame(root, **frame_config)
        frame.pack(side=pack_side)
        self.button = tk.Button(frame, **button_config)
        self.button.grid(row=0, column=0)
        self.label = tk.Label(frame, **label_config)
        self.label.grid(row=1, column=0)

        # Hide label and button
        self.button.grid_remove()
        self.label.grid_remove()
        self.hidden = True

    def on_click(self):
        raise NotImplementedError

    def show_hide(self):
        if self.hidden:
            self.button.grid()
            self.label.grid()
        else:
            self.button.grid_remove()
            self.label.grid_remove()
        self.hidden = not self.hidden


class ActivableButton(ButtonWithStatus):
    STATUS = 'Status: {}'

    def __init__(self, root, button_text, inactive_status, active_status):
        super().__init__(root, 'left', {'padx': 10},
                         {'text': button_text, 'font': 'courier 20', 'bd': 0},
                         {'text': self.STATUS.format(inactive_status), 'font': 'courier 14'})
        self.activated = False
        self._done = False
        self._active = active_status
        self._inactive = inactive_status

    def on_click(self):
        self.activated = not self.activated
        if self.activated:
            self.label['text'] = self.STATUS.format(self._active)
            self.label['fg'] = 'blue'
        else:
            self.label['text'] = self.STATUS.format(self._inactive)
            self.label['fg'] = 'black'

    def step_forward(self):
        if self._done:
            return

        if self.activated:
            self.button['text'] = ''
            self.button['bd'] = 0
            self.button['bg'] = '#F0F0F0'
            self.button['state'] = 'disabled'
            if random.randint(0, 1):
                self.label['text'] = 'Accomplished'
                self.label['fg'] = 'green'
            else:
                self.label['text'] = 'Failed'
                self.label['fg'] = 'red'
            self._done = True


def combine(*callables):
    def runner():
        for function in callables:
            function()
    return runner


if __name__ == '__main__':
    root = tk.Tk()
    main = tk.Button(root, text="Show/Hide", bg="white", font="courier 30")
    main.pack()

    frame = tk.Frame(root, pady=10)
    frame.pack()

    button1 = ActivableButton(frame, 'Perform action #1', 'Can be done', 'To be done')
    button2 = ActivableButton(frame, 'Perform action #2', 'Can be done', 'To be done')

    main['command'] = combine(button1.show_hide, button2.show_hide)

    next_day = tk.Button(root, text="Next day", bg="white", font="courier 30",
                         command=combine(button1.step_forward, button2.step_forward))
    next_day.pack()

    root.mainloop()
\$\endgroup\$
-1
\$\begingroup\$

I rewrote my code. If you, for any reason, want to see how this can be improved, here's what I've got:

import tkinter as tk
import random


class ActionButton(tk.Button):
    def __init__(self, *args, **kwargs):
        tk.Button.__init__(self, *args, **kwargs)
        self.configure(text="", font="courier 20", bd=0)


class ActionLabel(tk.Label):
    def __init__(self, *args, **kwargs):
        tk.Label.__init__(self, *args, **kwargs)
        self.configure(text="", font="courier 14")


def multi(*args):
    for func in args:
        return func


def show_hide(a, b, c, d):
    if not b[0]["text"]:
        b[0].configure(text="{}".format(d[0]), bd=2, bg="white",
                       command=lambda: activate_deactivate(a[0], c[0]))
        c[0]["text"] = "Status: {}".format(a[0].get())
        b[1].configure(text="{}".format(d[1]), bd=2, bg="white",
                       command=lambda: activate_deactivate(a[1], c[1]))
        c[1]["text"] = "Status: {}".format(a[1].get())
    else:
        b[0].configure(text="", bd=0, bg="#F0F0F0", command=None)
        c[0]["text"] = ""
        b[1].configure(text="", bd=0, bg="#F0F0F0", command=None)
        c[1]["text"] = ""


def activate_deactivate(a, b):
    if a.get() == "Can be done":
        a.set("To be done")
        b.configure(text="Status: {}".format(a.get()), fg="blue")
    else:
        a.set("Can be done")
        b.configure(text="Status: {}".format(a.get()), fg="black")


def step_forward(a, b, c):
    if a.get() == "To be done":
        b.configure(text="", bd=0, bg="#F0F0F0", state="disabled")
        c["text"] = ""
        result = random.choice(["success", "failure"])
        if result == "success":
            a.set("Accomplished")
            c["fg"] = "green"
        else:
            a.set("Failed")
            c["fg"] = "red"
    else:
        b.configure(text="", bd=0, bg="#F0F0F0", command=None)
        c["text"] = ""


root = tk.Tk()

status = tk.StringVar()
status.set("Can be done")

status_2 = tk.StringVar()
status_2.set("Can be done")

main = tk.Button(root, text="Show/Hide", bg="white", font="courier 30",
                 command=lambda: show_hide([status, status_2],
                                           [button, button_2],
                                           [label, label_2],
                                           ["Perform action #1", "Perform action #2"]))
main.pack()

frame = tk.Frame(root, pady=10)
frame.pack()

frame_1 = tk.Frame(frame, padx=10)
frame_1.pack(side="left")

frame_2 = tk.Frame(frame, padx=10)
frame_2.pack(side="left")

button = ActionButton(frame_1)
button.grid(row=0, column=0)

label = ActionLabel(frame_1)
label.grid(row=1, column=0)

button_2 = ActionButton(frame_2)
button_2.grid(row=0, column=1)

label_2 = ActionLabel(frame_2)
label_2.grid(row=1, column=1)

next_day = tk.Button(root, text="Next day", bg="white", font="courier 30",
                     command=lambda: multi(step_forward(status, button, label),
                                           step_forward(status_2, button_2, label_2)))
next_day.pack()

root.mainloop()

CHANGES DONE:

1) I created 2 classes for Label and Button - no need to apply same text, border size, etc. to those.

class ActionButton(tk.Button):
    def __init__(self, *args, **kwargs):
        tk.Button.__init__(self, *args, **kwargs)
        self.configure(text="", font="courier 20", bd=0)


class ActionLabel(tk.Label):
    def __init__(self, *args, **kwargs):
        tk.Label.__init__(self, *args, **kwargs)
        self.configure(text="", font="courier 14")

# code...
# code...
# code...

button = ActionButton(frame_1)
button.grid(row=0, column=0)

label = ActionLabel(frame_1)
label.grid(row=1, column=0)

button_2 = ActionButton(frame_2)
button_2.grid(row=0, column=1)

label_2 = ActionLabel(frame_2)
label_2.grid(row=1, column=1)

2) Now all functions were changed to use parameters. Instead of rewriting the same, they all take arguments.

def show_hide(a, b, c, d):
    if not b[0]["text"]:
        b[0].configure(text="{}".format(d[0]), bd=2, bg="white",
                       command=lambda: activate_deactivate(a[0], c[0]))
        c[0]["text"] = "Status: {}".format(a[0].get())
        b[1].configure(text="{}".format(d[1]), bd=2, bg="white",
                       command=lambda: activate_deactivate(a[1], c[1]))
        c[1]["text"] = "Status: {}".format(a[1].get())
    else:
        b[0].configure(text="", bd=0, bg="#F0F0F0", command=None)
        c[0]["text"] = ""
        b[1].configure(text="", bd=0, bg="#F0F0F0", command=None)
        c[1]["text"] = ""


def activate_deactivate(a, b):
    if a.get() == "Can be done":
        a.set("To be done")
        b.configure(text="Status: {}".format(a.get()), fg="blue")
    else:
        a.set("Can be done")
        b.configure(text="Status: {}".format(a.get()), fg="black")


def step_forward(a, b, c):
    if a.get() == "To be done":
        b.configure(text="", bd=0, bg="#F0F0F0", state="disabled")
        c["text"] = ""
        result = random.choice(["success", "failure"])
        if result == "success":
            a.set("Accomplished")
            c["fg"] = "green"
        else:
            a.set("Failed")
            c["fg"] = "red"
    else:
        b.configure(text="", bd=0, bg="#F0F0F0", command=None)
        c["text"] = ""

# code...
# code...
# code...

main = tk.Button(root, text="Show/Hide", bg="white", font="courier 30",
                 command=lambda: show_hide([status, status_2],
                                           [button, button_2],
                                           [label, label_2],
                                           ["Perform action #1", "Perform action #2"]))
main.pack()

next_day = tk.Button(root, text="Next day", bg="white", font="courier 30",
                     command=lambda: multi(step_forward(status, button, label),
                                       step_forward(status_2, button_2, label_2)))
next_day.pack()

NOTE:
It's still imperfect ("def show_hide" is still repetitive, and yet idk what can be done with it) but this is definitely a big step forward from what it was.

\$\endgroup\$
  • 2
    \$\begingroup\$ a, b, c, d are really bad names for parameters... \$\endgroup\$ – Graipher Nov 6 '16 at 7:09

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.