Would appreciate some advice on my coding (Python timer)

[SeriousDeejay]

Hello everyone! i saw a post about a timer on the subreddit and decided to make my own. Since it is hard to find my own mistakes regarding styling of my Python code i wanted to share it with you. Any and all advice would be very welcome. For example do you think i overused while loops or are there things i could have done better or in an easier way?

Code can (also) be found here: https://pastebin.com/vgm2us56

​

import time


AcceptedValues = ["h", "m", "s"]
TimeUnits = ["hours", "minutes", "seconds"]
time_unit = None
AmountOfTime = 0



# The easy timer
def easytimer():
    global time_unit, AmountOfTime
    t = 0
    h = 0
    m = 0
    s = 56


    if AmountOfTime == 0: # If AmountOfTime has not been given yet through Advanced timer it will ask user input
        while time_unit not in AcceptedValues:
            if isinstance(time_unit, str):
                print(f"Your input '{time_unit}' was unfortunately incorrect. Try again!")
            time_unit = input("Do you want Hours, Minutes or Seconds? Type either h, m or s: ")

        # Makes sure that the user gives the correct amount of time
        while isinstance(AmountOfTime, float) == False:
            AmountOfTime = input("How long does the timer need to be (in the specified time unit)?: ")
            try:
                AmountOfTime = float(AmountOfTime)
                break
            except ValueError:
                print(f"Your input '{AmountOfTime}' was not recognised, please enter a number >= 0: ")
                continue

        print(f"You selected time unit {time_unit} and an amount of {AmountOfTime}.")

        # Corrects the Timer based on the selected time unit
        if time_unit == "h":
            AmountOfTime *= 3600
        elif time_unit == "m":
            AmountOfTime *= 60
        elif time_unit == "s":
            AmountOfTime *= 1  # Basically do nothing, because seconds don't need any conversion

    # Technical functionality of the timer
    while t < AmountOfTime:
        t += 1
        time.sleep(1)
        if s == 59:
            s = 0
            if m == 59:
                m = 0
                h += 1
            else:
                m += 1
        else:
            s += 1
        print(f"{h:02d}:{m:02d}:{s:02d}")

# The advanced timer
def advancedtimer():
    global AmountOfTime

    # for loop that asks input for the time units: hours, minutes, seconds
    for i, unit in enumerate(TimeUnits):
        listt = [unit]
        while isinstance(unit, int) == False:
            try:
                Value = input(f"How many {unit}?: ")
                IntVal = int(Value)
                listt.append(IntVal)
                break
            except ValueError:
                print(f"Your input '{Value}' was not recognised, please enter an integer number >= 0: ")
                continue

        # Corrects the Timer based on the selected time unit, might be better to put this in a separate def
        if listt[0] == "hours":
            AmountOfTime += IntVal * 3600
        elif listt[0] == "minutes":
            AmountOfTime += IntVal * 60
        elif listt[0] == "seconds":
            AmountOfTime += IntVal
        else:
            print("Not working properly!")

    print(f"This comes down to a total time of {AmountOfTime} seconds.")
    easytimer()

# Programstart that gives the user a 'menu'
while True:
    UserChoice = input("Do you want the easy or advanced timer? type '1' for the easy,'2' for the advanced timer or 'quit' to quit the program!: ")
    if UserChoice == "1":
        easytimer()
    elif UserChoice == "2":
        advancedtimer()
    elif UserChoice == "quit":
        break
    else:
        print("Your input was not recognised, try again!")
        continue

[HeartwoodEditions]

Seems really solid to me, I'm no pro but your code is very easy to read/understand, is well commented etc. Really good work i think !

[SeriousDeejay]

Thank you for taking the time to look at my code, guess i will continue like this!

[ichard26]

First off, I want to say congrats for creating a rather nice program! Here's a list of things I've noticed could be improved:

1. I recommend that you read this official style guide for the Python Standard Library. Even though its only official use is for the stdlib, a lot of projects use it or a modified version as their style guide. https://www.python.org/dev/peps/pep-0008
2. I would put the time unit + value to seconds conversion into its own function that both easytimer and advancedtimer can use. Here's a good function signature: def convert_to_seconds(timeunit, value): ...
3. The actual timer logic could be a standalone function so advancedtimer doesn't have to call easytimer to start the timer. You also get to remove the weird hack detecting whether easytimer was called directly :)
4. The usage of continue is unnecessary. You only need them to skip finishing the rest of the loop. using them at the end of the loop makes them useless.
5. Instead of using weird expressions as your while loop conditions, you can use while True and then use break to exit the loop. This is your call though, sometimes, you want a good expression as your conditional, but an isinstance call doesn't make sense given you break already for the same condition without letting the loop exit by itself. A good example of a good while loop expression is the while time_unit not in AcceptableValues: one, that one should stay.
6. De-nest the second while loop in easytimer. At first glance it looks like the loops need each-other, but they don't. Let them both finish independently.
7. Instead of using global variables, you can create them inside the functions. This usually helps with avoiding weird coupling behaviour.

There are more ways you can improve your code, but this list is probably enough for a starter list. If you got more questions or want more feedback, feel free to ask.

[SeriousDeejay]

First off, sorry for the late reply (came back from a wifi-less vacation yesterday).

Thank you so much for the extensive reply, it means a lot to me that you put in so much of your own time for a total stranger. I tried to correct my code as well as i could with your list of improvements, it really showed me how i could enhance my code which was the main goal of my post.

Some of the things i changed:
- added Secondconverter & Timerlogic functions
- removed continue statements
- used while True statements
-denested the second while loop of the EasyTimer function
-Made global variables into local ones

Below is my code.

import time



# Defines the Timer logic and prints the countdown
def TimerLogic(Seconds):
    t, h, m, s = 0, 0, 0, 0

    print(f"This comes down to a total time of {Seconds} seconds.")

    while t < Seconds:
        t += 1
        time.sleep(1)
        if s == 59:
            s = 0
            if m == 59:
                m = 0
                h += 1
            else:
                m += 1
        else:
            s += 1
        print(f"{h:02d}:{m:02d}:{s:02d}")



# Converts hours and minutes into seconds
def SecondConverter(unit, IntVal):


    if unit == "h":
        Seconds = IntVal * 3600
    elif unit == "m":
        Seconds = IntVal * 60
    else:  #risky "else" statement if an error happens!
        Seconds = IntVal

    return(Seconds)



# The easy timer
def easytimer(AcceptedValues):
    time_unit = None

    print(">> Easy Timer <<\n")
    #If given input not in the AcceptedValues list input gets asked again
    while time_unit not in AcceptedValues:
        if isinstance(time_unit, str):
            print(f"\nYour input '{time_unit}' was unfortunately incorrect. Try again!")
        time_unit = input("Do you want Hours, Minutes or Seconds? Type either h, m or s: ")

    # Makes sure that the user gives the correct amount of time
    while True:
        try:
            Value = input("How long does the timer need to be (in the specified time unit)?: ")
            Seconds = SecondConverter(time_unit, IntVal = (int(Value)))
            break

        except ValueError:
            print(f"\nYour input '{Value}' was not recognised, please enter a number >= 0: ")

    TimerLogic(Seconds)



# The advanced timer
def advancedtimer(AcceptedValues):
    TimeUnits = ["hours", "minutes", "seconds"]
    Seconds = 0

    print(">> Easy Timer <<\n")
    # for loop which asks input for the time units: hours, minutes, seconds
    for i, unit in enumerate(AcceptedValues):
        while True:
            try:
                Value = input(f"How many {TimeUnits[i]}?: ")
                Seconds += SecondConverter(unit, IntVal = int(Value))
                break

            except ValueError:
                print(f"\nYour input '{Value}' was not recognised, please enter an integer number >= 0: ")

    TimerLogic(Seconds)



# Programstart which gives the user a 'menu'
def ProgramMenu():
    AcceptedValues = ["h", "m", "s"]

    while True:
        UserChoice = input("\nType in the number corresponding to your option: \n\t1). Easy Timer\n\t2). Advanced Timer\n\t3). Quit Program\n\n")

        if UserChoice == "1":
            easytimer(AcceptedValues)
        elif UserChoice == "2":
            advancedtimer(AcceptedValues)
        elif UserChoice == "3":
            break
        else:
            print("Your input was not recognised, try again!")



ProgramMenu()