1
\$\begingroup\$

I have been learning python since being in lockdown, this is the first Tkinter application I've built without consulting the internet or seeking help anywhere.

It is a time converter where you can select the input unit via a checkbox and the other entry windows will fill with the correct conversion.

I'd like to hear feedback on how it's written, in terms of if I have demonstrated best practice or areas where I could have used less code.

from tkinter import * 
from tkinter.ttk import*

root = Tk() 
Second = IntVar() 
Minute = IntVar() 
Hour = IntVar()

Var1 = IntVar() 
Var2 = IntVar() 
Var3 = IntVar()

def ConvertTime():
    if Var1.get() == 1:
        Minute.set(Second.get()/60)
        Hour.set(Second.get()/3600)
    
    if Var2.get() == 1:
        Second.set(Minute.get()*60)
        Hour.set(Minute.get()*0.01667)

    if Var3.get() == 1:
        Second.set(Hour.get()*3600)
        Minute.set(Hour.get()*60)

class GUI:

    def __init__(self, master):

        self.master = master
        master.title('Time Converter')
        master.geometry('+600+300')

        master.bttn = Button(root, text = 'Convert', command = ConvertTime)
        master.bttn.grid(pady= 5, padx=20, column=1, row=3)

        master.ChooseSecond = Checkbutton(root, text = 'Seconds', variable=Var1, onvalue = 1, offvalue = 0)
        master.ChooseSecond.grid(pady=5, padx=20, column=0, row=0)

        master.entry1 = Entry(root, textvariable=Second)
        master.entry1.grid(pady=5, padx=20, column=0, row=1)

        master.ChooseMinute = Checkbutton(root, text = 'Minutes', variable=Var2, onvalue = 1, offvalue=0)
        master.ChooseMinute.grid(pady=5, padx=20, column=1, row=0)

        master.entry2 = Entry(root, textvariable=Minute)
        master.entry2.grid(pady=5, padx=20, column=1, row=1)

        master.ChooseHour = Checkbutton(root, text = 'Hours',variable=Var3, onvalue=1, offvalue=0)
        master.ChooseHour.grid(pady=5, padx=20, column=2, row=0)

        master.entry3 = Entry(root, textvariable=Hour)
        master.entry3.grid(pady=5, padx=20, column=2, row = 1)

def main():
    GUI(root)
    root.mainloop()

main()
\$\endgroup\$

2 Answers 2

1
\$\begingroup\$
  • Avoid putting variables in the global namespace
  • Use a more meaningful variable name than Var1
  • With your current usage, Varx variables should be boolean variables, not integer variables, due to them connecting to checkboxes
  • Use radio buttons rather than checkboxes, because you really want exclusive selection
  • No reason not to accept floats instead of ints for your entry boxes
  • Generalize a series of conversion factors so that your division and multiplication only need to be expressed once
  • No need to set your controls as members on master
  • Avoid calling things master; it's 2021
  • Centralize the expression of your padding quantities, since they're all the same
  • Separate your business logic from your presentation

Suggested

from enum import Enum
from functools import partial
from tkinter import Button, DoubleVar, Entry, Grid, IntVar, Radiobutton, Tk
from typing import Callable, Dict, Iterable


FACTORS = (1, 60, 60 * 60)


class TimeComponent(Enum):
    SECOND = 0
    MINUTE = 1
    HOUR = 2

    def get_factor(self) -> int:
        return FACTORS[self.value]


class GUIVariable:
    def __init__(
        self,
        root: Tk,
        component: TimeComponent,
        selected_component: IntVar,
    ):
        self.check_button = Radiobutton(
            root,
            text=component.name.title(),
            variable=selected_component,
            value=component.value,
        )

        value = DoubleVar()
        self.get_value: Callable[[], float] = value.get
        self.set_value: Callable[[float], None] = value.set
        self.entry = Entry(root, textvariable=value)


class TimeController:
    def __init__(self, component: TimeComponent, gui: GUIVariable):
        self.get_factor: Callable[[], int] = component.get_factor
        self.gui = gui

    @property
    def normalized(self) -> float:
        return self.gui.get_value() * self.get_factor()

    @normalized.setter
    def normalized(self, x: float) -> None:
        self.gui.set_value(x / self.get_factor())

    def adjust_others(self, others: Iterable['TimeController']):
        value = self.normalized

        for other in others:
            if other is not self:
                other.normalized = value


class GUI:
    def __init__(self):
        root = Tk()
        self.run: Callable[[], None] = root.mainloop

        root.title('Time Converter')
        root.geometry('+600+300')

        padded_grid = partial(Grid.grid, pady=5, padx=20)
        convert_button = Button(root, text='Convert', command=self.convert_time)
        padded_grid(convert_button, column=1, row=3)

        self.selected_var = IntVar()
        self.controllers: Dict[TimeComponent, TimeController] = {}
        for component in TimeComponent:
            var = GUIVariable(root, component, self.selected_var)
            padded_grid(var.check_button, column=component.value, row=0)
            padded_grid(var.entry, column=component.value, row=1)

            controller = TimeController(component, var)
            self.controllers[component] = controller

    @property
    def current_controller(self) -> TimeController:
        return self.controllers[
            TimeComponent(self.selected_var.get())
        ]

    def convert_time(self) -> None:
        self.current_controller.adjust_others(self.controllers.values())


if __name__ == '__main__':
    GUI().run()

Alternative

An alternate GUI that is fairly simplified: don't have any selectors or buttons, and when one field changes, update the others.

from enum import Enum
from functools import partial
from tkinter import DoubleVar, Entry, Grid, Label, Tk, TclError
from typing import Callable, Dict, Iterable


FACTORS = (1, 60, 60 * 60)


class TimeComponent(Enum):
    SECOND = 0
    MINUTE = 1
    HOUR = 2

    def get_factor(self) -> int:
        return FACTORS[self.value]


class GUIVariable:
    def __init__(
        self,
        root: Tk,
        component: TimeComponent,
        trace: Callable[['GUIVariable'], None],
    ):
        value = DoubleVar()
        value.trace('w', self.trace_inner)
        self.trace = trace
        self.component = component
        self.get_value: Callable[[], float] = value.get
        self.set_value: Callable[[float], None] = value.set

        self.label = Label(root, text=component.name.title())
        self.entry = Entry(root, textvariable=value)

    def trace_inner(self, name: str, _: str, mode: str) -> None:
        self.trace(self)


class TimeController:
    def __init__(self, component: TimeComponent, gui: GUIVariable):
        self.get_factor: Callable[[], int] = component.get_factor
        self.gui = gui

    @property
    def normalized(self) -> float:
        return self.gui.get_value() * self.get_factor()

    @normalized.setter
    def normalized(self, x: float) -> None:
        self.gui.set_value(x / self.get_factor())

    def adjust_others(self, others: Iterable['TimeController']):
        try:
            value = self.normalized
        except TclError:
            return

        for other in others:
            if other is not self:
                other.normalized = value


class GUI:
    def __init__(self):
        root = Tk()
        self.run: Callable[[], None] = root.mainloop

        root.title('Time Converter')
        root.geometry('+600+300')

        padded_grid = partial(Grid.grid, pady=5, padx=20)

        self.controllers: Dict[TimeComponent, TimeController] = {}
        for component in TimeComponent:
            var = GUIVariable(root, component, self.on_change)
            padded_grid(var.label, column=component.value, row=0)
            padded_grid(var.entry, column=component.value, row=1)

            controller = TimeController(component, var)
            self.controllers[component] = controller

    def on_change(self, var: GUIVariable) -> None:
        controller = self.controllers[var.component]
        controller.adjust_others(self.controllers.values())


if __name__ == '__main__':
    GUI().run()

simplified

\$\endgroup\$
1
\$\begingroup\$

One issue I can see on first glance is the *. It's considered bad practice to import everything like that, and here is why from this Stack Overflow post:

  • Because it puts a lot of stuff into your namespace (might shadow some other object from previous import and you won't know about it).
  • Because you don't know exactly what is imported and can't easily find from which module a certain thing was imported (readability).
  • Because you can't use cool tools like pyflakes to statically detect errors in your code.
\$\endgroup\$
3
  • \$\begingroup\$ Overall your feedback is correct - splat-import should generally be discouraged. However, how does this interfere with static analysis? I think it probably doesn't? \$\endgroup\$
    – Reinderien
    Commented May 18, 2021 at 16:30
  • \$\begingroup\$ @Reinderien I didn't say anything about the static analsis. The OP asked "I'd like to hear feedback on how it's written, in terms of if I have demonstrated best practice". \$\endgroup\$
    – Chocolate
    Commented May 18, 2021 at 18:36
  • \$\begingroup\$ Then what did you mean by statically detect errors in your code ? \$\endgroup\$
    – Reinderien
    Commented May 18, 2021 at 18:58

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.