4
\$\begingroup\$

I've self-taught myself Python and have recently started learning tkinter. I made a simple stopwatch program and I wanted to know if the code is clean and written in a way that the code works efficiently. I would highly appreciate any suggestions on how to improve my code! Thank you!

from tkinter import *
import time

root = Tk()

numberOfSeconds = 0

def counting():
    global numberOfSeconds
    global stopCounting

    if stopCounting == False:
        numberOfSeconds += 1
        seconds.config(text=str(numberOfSeconds))
        seconds.after(1000, counting)
    elif stopCounting == True:
        stopCounting = False
        numberOfSeconds = 0
        seconds.config(text=str(numberOfSeconds))


def start():
    global stopCounting
    stopCounting = False
    stopButton.config(state=NORMAL)
    seconds.after(1000, counting)

def stop():
    global stopCounting
    stopButton.config(state=DISABLED)
    stopCounting = True


seconds = Label(text=str(numberOfSeconds))
startButton = Button(text="Start", command=start)
stopButton = Button(text="Stop", command=stop, state=DISABLED)

seconds.grid(row=0, column=0, columnspan=2)
startButton.grid(row=1, column=0)
stopButton.grid(row=1, column=1)

root.mainloop()

\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

Globals

Generally speaking it's not a good idea to use globals like this. It harms re-entrance. What if you want to support two stopwatches at once, either in one UI or as a web server? Having globals like this will prevent that.

It also harms testability. It is more difficult to test methods that rely on global state than it is to test methods that are self-contained, having state passed to them either in an object context (self) or as method parameters.

One way to get around this is make a class with attributes number_of_seconds and is_counting (which I find would be more intuitive than stop_counting).

Booleans

This block:

if stopCounting == False:
    numberOfSeconds += 1
    seconds.config(text=str(numberOfSeconds))
    seconds.after(1000, counting)
elif stopCounting == True:
    stopCounting = False
    numberOfSeconds = 0
    seconds.config(text=str(numberOfSeconds))

is more easily expressed as

if stopCounting:
    stopCounting = False
    numberOfSeconds = 0
    seconds.config(text=str(numberOfSeconds))
else:
    numberOfSeconds += 1
    seconds.config(text=str(numberOfSeconds))
    seconds.after(1000, counting)

Variable names

They should be lower_snake_case, i.e. start_button.

\$\endgroup\$
4
  • \$\begingroup\$ Thank you so much! However, I don't really understand why I can't be using globals that way, is there any chance you could explain it to me a little bit more? \$\endgroup\$
    – AryS
    Commented May 26, 2020 at 20:02
  • \$\begingroup\$ I've added a little more explanatory text. \$\endgroup\$
    – Reinderien
    Commented May 26, 2020 at 20:05
  • \$\begingroup\$ It's not that you can't do it (you already are); it's that it makes your life more difficult when you have non-trivial application development to do. \$\endgroup\$
    – Reinderien
    Commented May 26, 2020 at 20:05
  • \$\begingroup\$ Ah, ok, thank you so much for your help! \$\endgroup\$
    – AryS
    Commented May 26, 2020 at 20:11

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.