5
\$\begingroup\$

I wrote a timer app in python using both twilio and the timer module; sends an sms to my phone after a certain amount of time has passed. While it works without a doubt, I wanted to gather some ideas/opinions on how to improve my program for efficiency, readability, etc...

I'm open to all suggestions.

import time
from twilio.rest import Client

client = Client("KEY", "KEY")

work_time = 20 * 60
break_time = 5 * 60

def _timer(workState = True):
    messageBody = ["Time to work", "Time to break"]
    if workState == True:
        client.messages.create(to="MYNUMBER", from_="TWILIONUMBER", body="Time To Work!")
        time.sleep(work_time)
        _timer(not workState)
    else:
        client.messages.create(to="MYNUMBER", from_="TWILIONUMBER", body="Time To Break!")
        time.sleep(break_time)
        _timer(not workState)
_timer(False)

Please note: Several values have been changed for privacy sake (e.g. MYNUMBER, TWILIONUMBER, KEY)

\$\endgroup\$

2 Answers 2

6
\$\begingroup\$

Ways of improving:

  • work_time and break_time as constant values deserve to be defined as CONSTANTS
  • _timer function:

    • workState should be renamed to work_state to follow naming conventions

    • messageBody = ["Time to work", "Time to break"] is not actually used (can be eliminated)

    • if workState == True: is just a verbose version of if workState:
    • both conditional branches if/else have the same common statement _timer(not workState) but differ in action name work/break and delay time work_time/break_time.
      The whole condition can be restructured to reduce repetitive code and move the common statement(s) out

The final optimized version:

import time
from twilio.rest import Client

client = Client("KEY", "KEY")

WORK_TIME = 20 * 60
BREAK_TIME = 5 * 60


def _timer(work_state=True):
    if work_state:
        action, delay_time = 'work', WORK_TIME
    else:
        action, delay_time = 'break', BREAK_TIME

    client.messages.create(to="MYNUMBER", from_="TWILIONUMBER", 
                           body=f"Time To {action.title()}!")
    time.sleep(delay_time)
    _timer(not work_state)


_timer(False)
\$\endgroup\$
1
  • 2
    \$\begingroup\$ I appreciate the effort you put forward, thank you! \$\endgroup\$
    – Red Mango
    Nov 15, 2019 at 21:57
6
\$\begingroup\$

RomanPerekhrest has brought up several good points that you should definitely follow.

One point that was not mentioned however, is the recursive way you call your _timer(...) function. As you may or may not know, Python has an upper limit on how many recursion levels are allowed. You can query the limit on your system using sys.getrecursionlimit() after import sys. Once you hit that limit, an exception will occur and end your program.

Luckily for you, time.sleep(...) with the given intervals in your code is going to prevent you from hitting that limit during a normal workday. If I've not miscalculated, you would need to run your code for over 208h before getting in trouble (recursion limit of 1000 here on my machine, i.e. 500 cycles á 25min = 208,...h).

But there are more good news here. You don't really need recursion! A simple iterative approach (based off of RomanPerekhrest's answer) should work equally well:

import time
from twilio.rest import Client

WORK_TIME = 20 * 60
BREAK_TIME = 5 * 60


def _timer(work_state=True):
    client = Client("KEY", "KEY")  # client does not need to be global any more
    while True:
        if work_state:
            action, delay_time = 'work', WORK_TIME
        else:
            action, delay_time = 'break', BREAK_TIME
        work_state = not work_state

        client.messages.create(to="MYNUMBER", from_="TWILIONUMBER", 
                               body=f"Time To {action.title()}!")
        time.sleep(delay_time)


if __name__ == "__main__":
    _timer(False)

A few subtleties that I also changed:

  • Since we are now always in the same function scope, client does not need to be a global variable anymore and can be moved into the function definition.

  • Starting the timer is now surrounded by if __name__ == "__main__":. This line will make sure, that the code is only run if used as a script, but not in case you would try to import something from that file.

\$\endgroup\$

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.