8
\$\begingroup\$

I wrote a simple Pomodoro Timer in Python 3:

from datetime import datetime, timedelta
from gi import require_version
require_version('Notify', '0.7')
from gi.repository import Notify
import sys 
class State:
 #length: the length of the state in minutes
 def __init__(self, length):
 if not length == None:
 self.length = length
class Clock:
 work = None
 sBreak = None 
 lBreak = None
 numWork = 4
 remWork = numWork #number of work sessions remaining
 remBreaks = numWork - 1 #number of short break sessions remaining
 current = None #the current state
 endtime = None #datetime object for end time of current 
 def __init__(self, workTime, sBreakTime, lBreakTime):
 if workTime == None or sBreakTime == None or lBreakTime == None:
 exit(1)
 self.work = State(workTime)
 self.sBreak = State(sBreakTime)
 self.lBreak = State(lBreakTime)
 self.states = [self.work, self.sBreak, self.lBreak]
 def change(self): 
 if not self.current == None and not self.current in self.states:
 exit(1)
 title = ''
 timeStr = 'End Time: '
 #current doesn't exist: start the first work period
 if self.current == None:
 self.current = self.work 
 title = 'Work Period'
 #in work state now: either short or long break
 elif self.current == self.work:
 self.remWork -= 1
 if self.remBreaks == 0:
 self.current = self.lBreak
 title = 'Long Break'
 else:
 self.current = self.sBreak
 title = 'Short Break'
 #in short break now: start work
 elif self.current == self.sBreak:
 self.remBreaks -= 1
 self.current = self.work
 title = 'Work Period'
 #in long break now: reset number of work and short break periods
 elif self.current == self.lBreak:
 self.current = self.work
 title = 'Work Period'
 self.remWork = self.numWork
 self.remBreaks = self.numWork - 1
 self.endtime = datetime.now() + timedelta(seconds = self.current.length)
 timeStr += self.endtime.strftime("%H:%M")
 Notify.init("a")
 notifyme=Notify.Notification.new(title,timeStr)
 notifyme.show()
 def tickTock(self):
 self.change()
 while True:
 if datetime.now() >= self.endtime:
 self.change()
def main():
 try:
 lengths = sys.argv[1:]
 workLen = int(lengths[0])
 sbLen = int(lengths[1])
 lbLen = int(lengths[2])
 clock = Clock(workLen, sbLen, lbLen)
 clock.tickTock()
 except:
 print("One or more arguments were invalid.")
 exit(1)
if __name__ == '__main__':
 main()

First of all, is this a good way to simulate the timer? Also, is there a way to get around using self.variable_name every time I want to use a variable in the Clock class? Finally, what would be a good package to look into if I wanted to extend this into a GUI application?

200_success
145k22 gold badges190 silver badges478 bronze badges
asked Jun 9, 2016 at 4:55
\$\endgroup\$
3
  • \$\begingroup\$ Can you explain what this program is supposed to do? What is a pomodoro timer and what features does your program implement? \$\endgroup\$ Commented Jun 9, 2016 at 9:49
  • \$\begingroup\$ The pomodoro technique is a way of working, in which you work for a certain amount of time (usually 25 minutes) and then rest for a short amount of time (usually 5 minutes). A Pomodoro cycle has 4 work periods, each followed by a short break and the cycle ends with a long break. \$\endgroup\$ Commented Jun 9, 2016 at 13:20
  • \$\begingroup\$ I get a print parenthesis error in the gi module, so it seems python 2.7, not 3. \$\endgroup\$ Commented Aug 31, 2016 at 7:39

4 Answers 4

6
\$\begingroup\$

Just some quick tips, I might append some later. You might disagree with me, it's just my vision.

1) AFAIK, Pomodoro has some "classic" times for work and break sessions. It might be good to create them as default ones so the user wouldn't have to enter them manually via command line each time. I suggest using argparse module for that. A really quick and dirty rewriting of your main function:

def main():
 try:
 argparser = argparse.ArgumentParser()
 argparser.add_argument("--workLen", help="the length of a work session", type=int, default=25)
 argparser.add_argument("--sbLen", help="the length of a short break", type=int, default=5)
 argparser.add_argument("--lbLen", help="the length of a long break", type=int, default=30)
 # parse the arguments. They can be accessed in form args.argument
 args = argparser.parse_args()
 clock = Clock(args.workLen, args.sbLen, args.lbLen)
 clock.tickTock()
 except Exception as e:
 print("One or more arguments were invalid.", str(e))
 exit(1)

2) No need to put your work, sBreak and lBreak (and, probably, others too) variables right after class Clock: because that way you're declaring them as static. When you're declaring them in __init__, you basically declare other variables - the variables for this particular instance. And the static ones just remain None and are useless. So you may safely remove them. More info

3) Not sure if it is a common practice, but I would do if workTime == None or sBreakTime == None or lBreakTime == None: as if None in (workTime,sBreakTime,lBreakTime,):. You're testing for the same value anyway (None). I'm not sure if you want this behaviour, but if you want your program to exit if any of these values is 0 as well, you may use if not all((workTime,sBreakTime,lBreakTime,)):, because 0 is treated as false by Python.

I agree with Matthias on exit. As for naming conventions, I prefer being more liberal: do as you find comfortable, especially if it is your personal project. I would also use the PEP8 convention for variables (small_letters_with_underscore), but when it comes to function names, I'm more used to camelCaseStartingWithSmallLetter ;)

UPD1: 4) Add a sleep(0.1) at the end of your while loop in tickTock, because otherwise it uses up too much CPU.

answered Jun 9, 2016 at 11:53
\$\endgroup\$
2
  • \$\begingroup\$ As for the static variables, is there a way to declare the variables before the init method in Python? I'm used to coding in Java, where the variables can be declared and/or initialized before the Constructor. \$\endgroup\$ Commented Jun 9, 2016 at 13:23
  • 1
    \$\begingroup\$ @darora There is absolutely no need to do so in Python, since it is a dynamic language. If you want to initialize attribute foo, just assign self.foo in __init__. \$\endgroup\$ Commented Jun 9, 2016 at 13:41
4
\$\begingroup\$

First of all, by Python coding standards (PEP8) you should write variable names all lowercase, separated by underscore.

Secondly you can rewrite the code in your main function to look like this:

def main():
 try:
 work_len, sb_len, lb_len = map(int, sys.argv[1:4])

Though I think this depends on taste, it's shorter but may look more complicated to a beginner.

Thirdly, don't use "exit", use "self.run = False" and "return" and in your tickTock method, replace "while True" with "while self.run".

Apart from that I think it's pretty good. I saw some other PEP8 related issues, so I would definitely recommend looking into that. Please keep in mind that what I said is opinion. I know I can sound commanding sometimes, but that's not how I mean to sound :)

Also, I didn't know what Pomodoro Timer is, so thank you for that.

answered Jun 9, 2016 at 11:08
\$\endgroup\$
3
\$\begingroup\$

Instead of trying to manage end times yourself and constantly checking that the current date is not past the end date of an activity, you should rely on threading.Timers. You can create a Timer that will display a notification when an activity starts/ends.

Since you can not do that for an infinite amount of cycles, you can create a function whose sole purpose is to create a timer for all tasks of a complete cycle and then wait for the long break to end. Next logical step is to call this function in a while True loop. This function will replace your Clock class as you won't have to manage a state yourself anymore.

The biggest advantage of this approach is that your process is not doing anything when waiting for a notification to be displayed.

In the following code, I reuse some of @Highstaker's advices. I also modified the State class to hold information about the type of tasks and to simplify notifications management: each State is responsible for managing its own notification messages. The namedtuple base class is only there to simplify __init__. Lastly, I moved the Notify.init call at the top-level as there is absolutely no need of calling it more than once.

import threading
import argparse
import itertools
from datetime import datetime, timedelta
from collections import namedtuple
from gi import require_version
require_version('Notify', '0.7')
from gi.repository import Notify
NOW = datetime.now
Notify.init("Pomodoro")
class State(namedtuple('State', 'length activity')):
 def notify_start(self):
 self._notify(
 'Started. End time: {:%H:%M}'
 .format(NOW() + timedelta(minutes=self.length)))
 def notify_end(self):
 self._notify('Ended')
 def _notify(self, status):
 message = '[{:%H:%M}] {}'.format(NOW(), status)
 notifyme = Notify.Notification.new(self.activity, message)
 notifyme.show()
def pomodoro_cycle(work_time, s_break_time, l_break_time):
 if None in (work_time, s_break_time, l_break_time):
 raise ValueError('No time provided')
 work = State(work_time, 'Work Period')
 s_break = State(s_break_time, 'Short Break')
 l_break = State(l_break_time, 'Long Break')
 cycle = itertools.chain(
 itertools.chain.from_iterable(
 itertools.repeat((work, s_break), 4)),
 (l_break,))
 milestone = timedelta(0)
 for activity in cycle:
 threading.Timer(milestone.total_seconds(),
 activity.notify_start).start()
 milestone += timedelta(minutes=activity.length)
 last_task = threading.Timer(milestone.total_seconds(),
 activity.notify_end)
 last_task.start()
 last_task.join()
def main():
 parser = argparse.ArgumentParser('Pomodoro clock')
 parser.add_argument('--work_time', type=int, default=25)
 parser.add_argument('--short_break', type=int, default=5)
 parser.add_argument('--long_break', type=int, default=30)
 args = parser.parse_args()
 while True:
 pomodoro_cycle(args.work_time, args.short_break, args.long_break)
if __name__ == '__main__':
 main()

Or you can manage the repeating loop within pomodoro_cycle entirely and wait (join) each activity individually for when it ends:

import threading
import argparse
import itertools
from datetime import datetime, timedelta
from gi import require_version
require_version('Notify', '0.7')
from gi.repository import Notify
NOW = datetime.now
Notify.init("Pomodoro")
class State:
 def __init__(self, length, activity):
 self.activity = activity
 self.length = timedelta(minutes=length)
 def notify_start(self):
 self._notify(
 'Started. End time: {:%H:%M}'
 .format(NOW() + self.length))
 def notify_end(self):
 self._notify('Ended')
 def _notify(self, status):
 message = '[{:%H:%M}] {}'.format(NOW(), status)
 notifyme = Notify.Notification.new(self.activity, message)
 notifyme.show()
def pomodoro_clock(work_time, s_break_time, l_break_time):
 if None in (work_time, s_break_time, l_break_time):
 raise ValueError('No time provided')
 work = State(work_time, 'Work Period')
 s_break = State(s_break_time, 'Short Break')
 l_break = State(l_break_time, 'Long Break')
 cycle = (
 work, s_break,
 work, s_break,
 work, s_break,
 work, s_break,
 l_break,
 )
 for activity in itertools.chain.from_iterable(itertools.repeat(cycle)):
 activity.notify_start()
 end = threading.Timer(activity.length.total_seconds(),
 activity.notify_end)
 end.start()
 end.join()
def main():
 parser = argparse.ArgumentParser('Pomodoro clock')
 parser.add_argument('--work_time', type=int, default=25)
 parser.add_argument('--short_break', type=int, default=5)
 parser.add_argument('--long_break', type=int, default=30)
 args = parser.parse_args()
 pomodoro_clock(args.work_time, args.short_break, args.long_break)
if __name__ == '__main__':
 main()
answered Jun 9, 2016 at 15:17
\$\endgroup\$
1
\$\begingroup\$

Here is a rewrite of your version as I would do it. Though it think the design is overcomplicated. A simple sleeper function should be able to handle this.

Notice I use less variables, name them differently and place them elsewhere. I also nested your subclass.

from datetime import datetime, timedelta
# from gi import require_version
# require_version('Notify', '0.7')
# from gi.repository import Notify
import sys
import time
class PomoClock:
 Ncycles = 4
 class State: # dictionary/map may be better
 def __init__(self, length, ID):
 if length is not None:
 self.length = length # in minutes
 self.ID = ID
 def __init__(self, workTime=0, sBreakTime=0, lBreakTime=0):
 n = PomoClock.Ncycles
 try:
 m = n*workTime + (n-1)*sBreakTime
 self.alarm_time = datetime.now() + timedelta(minutes=workTime)
 except TypeError:
 print('Incorrect type passed for time period(s)') # forgiving > asking
 # define the 3 periods
 self.SW = PomoClock.State(workTime, 'work')
 self.SS = PomoClock.State(sBreakTime, 'short')
 self.SL = PomoClock.State(lBreakTime, 'long')
 # set initial conditions
 self.state = self.SW # start in work period
 self.N_todo = n
 (print('Clock initialized\nAfter starting, the time until long break is: ' +
 str(m) + ' minutes\nWaiting...'))
 def change(self):
 # in work state: switch to short or long break
 if self.state.ID == self.SW.ID:
 self.N_todo -= 1
 p = str(PomoClock.Ncycles - self.N_todo)
 if self.N_todo == 0:
 self.state = self.SL
 title = 'Work period ' + p + ' over. Long Break Started: '
 else:
 self.state = self.SS
 title = 'Work period ' + p + ' over. Short Break Started: '
 elif self.state.ID == self.SS.ID: # short break
 self.state = self.SW
 p = str(1 + PomoClock.Ncycles - self.N_todo)
 title = 'Short Break period Over. Work Period ' + p + ' Started: '
 else: # long break
 self.state = self.SW
 p = str(1 + PomoClock.Ncycles - self.N_todo)
 title = 'Break period Over. Work Period ' + p + ' Started: '
 self.N_todo = PomoClock.Ncycles
 # Set new checking time and notify when it occurs
 delta = timedelta(minutes=self.state.length)
 self.alarm_time = datetime.now() + delta
 (print(title + datetime.now().strftime("%H:%M") + '\n' +
 'Next period starts at: ' + self.alarm_time.strftime("%H:%M")))
 def tickTock(self):
 (print('Pomodoro work timer started at ' + datetime.now().strftime("%H:%M") +
 '...First work period GO!'))
 while True:
 time.sleep(0.1)
 if datetime.now() >= self.alarm_time:
 self.change()
def makedigit(x): # danger: broken function!
 if '.' in x:
 return float(x)
 else:
 return int(x)
def main():
 w, s, b = sys.argv[1:4]
 w, s, b = (makedigit(w), makedigit(s), makedigit(b)) # sanitize input
 clock = PomoClock(w, s, b)
 clock.tickTock()
if __name__ == '__main__':
 main()

And here's a really simple version:

import sys
import time
def pomo():
 w, s, b = sys.argv[1:4]
 w, s, b = (int(w), int(s), int(b)) # seconds
 while True:
 for it in range(3):
 print('work!')
 time.sleep(w)
 print('short break!')
 time.sleep(s)
 print('work!')
 time.sleep(w)
 print('Break Time! ')
 time.sleep(b)
pomo()
answered Aug 31, 2016 at 12:01
\$\endgroup\$

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.