5
\$\begingroup\$

Below is a class,Countdown, that takes seconds as input from an entry, and starts counting down when the "Start" button is pressed, while displaying the time left on a label:

import tkinter as tk
import datetime

class Countdown(tk.Frame):
    '''A Frame with label to show the time left, an entry to input the seconds to count
    down from, and a start button to start counting down.'''
    def __init__(self, master):
        super().__init__(master)
        self.create_widgets()
        self.show_widgets()
        self.seconds_left = 0
        self._timer_on = False

    def show_widgets(self):

        self.label.pack()
        self.entry.pack()
        self.start.pack()

    def create_widgets(self):

        self.label = tk.Label(self, text="00:00:00")
        self.entry = tk.Entry(self, justify='center')
        self.entry.focus_set()
        self.start = tk.Button(self, text="Start", command=self.start_button)

    def countdown(self):
        '''Update label based on the time left.'''
        self.label['text'] = self.convert_seconds_left_to_time()

        if self.seconds_left:
            self.seconds_left -= 1
            self._timer_on = self.after(1000, self.countdown)
        else:
            self._timer_on = False

    def start_button(self):
        '''Start counting down.'''
        self.seconds_left = int(self.entry.get())   # 1. to fetch the seconds
        self.stop_timer()                           # 2. to prevent having multiple
        self.countdown()                            #    timers at once

    def stop_timer(self):
        '''Stops after schedule from executing.'''
        if self._timer_on:
            self.after_cancel(self._timer_on)
            self._timer_on = False

    def convert_seconds_left_to_time(self):

        return datetime.timedelta(seconds=self.seconds_left)


if __name__ == '__main__':
    root = tk.Tk()
    root.resizable(False, False)

    countdown = Countdown(root)
    countdown.pack()

    root.mainloop()

Review Concern(s):

  • My main concern is to code in an easy to read, efficient, and well-structured manner while still learning the language and concepts such as OOP. Feel free to mention the tiniest issue or improvement that comes to your mind, as I am a beginner and I probably need it.

Note: This was an answer in SO.

\$\endgroup\$
4
\$\begingroup\$

Very clean code indeed, there's not much to be said here.

PEP8

I'm sure you've been lectured on this before, but there's an official style guide for Python called PEP8. It states that module-level class definitions should be separated by two lines:

import tkinter as tk
import datetime


class Countdown(tk.Frame):
    ...

You also leave the first line of every method blank (or make it a docstring). If a function is undocumented, I wouldn't bother adding a blank line:

def foo():
    do_stuff()

If you decide to add a docstring, I'd recommend the following structure:

< function definition >
    < docstring (may span multiple lines) >
    < blank line >
    < function body >

For example:

def bar():
    """I'm a docstring!
    Blah, blah.
    """

    do_stuff()

Private attributes

It's good that you marked _timer_on as private. Why not do the same for seconds_left and the helper methods? This clearly tells other developers: 'I'm not part of the API. Don't rely on me for backwards compatibility.'. (Added bonus: you don't have to document private functions).

class Countdown(tk.Frame):
    ...

    def show_widgets(self):
        < docstring >

        ...

    def create_widgets(self):
        < docstring >

        ...

    def _countdown(self):
        ...

    def _start_button(self):
        ...

    def _stop_timer(self):
        ...

    def _convert_seconds_left_to_time(self):
        ...

Basically, any method, property, or attribute that isn't useful for end users, should be private.

OOP

convert_seconds_left_to_time could be a staticmethod. A static method does not receive an implicit first argument. You should make a method static if it doesn't interact with the instance of the class, but is still strongly related to the class.

I'd then rename it to _get_timedelta_from_seconds:

class Countdown(tk.Frame):
    ...

    def countdown(self):
        """Update the label based on the time left."""

        self.label["text"] = self._get_timedelta_from_seconds(self.seconds_left)

    @staticmethod
    def _get_timedelta_from_seconds(seconds):
        return datetime.timedelta(seconds=seconds)
\$\endgroup\$
  • \$\begingroup\$ Thanks a lot for reviewing, I think you've made some good points. I agree with all the points here, will just try to make a habit of using these styles. \$\endgroup\$ – Nae Jan 5 '18 at 0:52

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.