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?

\$\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\$
    – darora
    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\$
    – jiggunjer
    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.

\$\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\$
    – darora
    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.

\$\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()
\$\endgroup\$
0
\$\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()
\$\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.