Pomodoro timer in Go

Question

My goal is to implement a simple pomodoro timer using Go: channels, goroutines.

I'm a newbie in Go world and have some misunderstanding about naming convention. I read a lot of Docker's code on GitHub and there I saw that people sometimes use very short names. These names are not clear for me (a man who comes from C++ world).

func main() {
 app := pomodoro.NewPomodoro()
 app.Start()
}

Give me a review about structure's name

type (
 PomodoroManager struct {
 dur Duration
 interv Interval
 ch Chanel
 serv PomodoroService
 }
 Duration struct {
 working int
 shortBreak int
 longBreak int
 }
 Interval struct {
 shortInterval int
 longInterval int
 }
 Chanel struct {
 start chan bool
 endShortBreak chan bool
 endLongBreak chan bool
 end chan bool
 }
)
func NewPomodoro() *PomodoroManager {
 dur := Duration{
 working: 25,
 shortBreak: 5,
 longBreak: 15,
 }
 interv := Interval{
 shortInterval: 1,
 longInterval: 5,
 }
 ch := Chanel{
 start: make(chan bool),
 endShortBreak: make(chan bool),
 endLongBreak: make(chan bool),
 end: make(chan bool),
 }
 serv := PomodoroService{}
 return &PomodoroManager{dur, interv, ch, serv}
}
func (p *PomodoroManager) Start() {
 go p.serv.StartWorking(p.ch.start, p.dur.working)
 go p.startServManager(p.ch.start, p.ch.endShortBreak, p.ch.endLongBreak, p.ch.end)
 <-p.ch.end
}
func (p *PomodoroManager) startServManager(start, endBreak, endLong, end chan bool) {
 for {
 select {
 case afterWorking := <-start:
 _ = afterWorking
 if p.interv.shortInterval == p.interv.longInterval {
 go p.serv.StartLongBreak(endLong, p.dur.longBreak)
 } else {
 p.interv.shortInterval += 1
 go p.serv.StartBreak(endBreak, p.dur.shortBreak)
 }
 case endShortRelax := <-endBreak:
 _ = endShortRelax
 go p.serv.StartWorking(start, p.dur.working)
 case endLongRelax := <-endLong:
 _ = endLongRelax
 userAnswer := askUser()
 if userAnswer == "Y" {
 p.interv.shortInterval = 1
 go p.serv.StartWorking(start, p.dur.working)
 } else {
 end <- true
 }
 }
 }
}
func askUser() string {
 fmt.Println("Do you want to coninue? Y/N")
 var answer string
 fmt.Scanln(&answer)
 return answer
}

And Last component of the program.

import (
 "os/exec"
 "time"
)
type (
 Service interface {
 StartWorking(chan bool, int)
 FinishBreak(chan bool, int)
 FinishLongBreak(chan bool, int)
 }
 PomodoroService struct {
 }
)
func (p *PomodoroService) StartWorking(start chan bool, duration int) {
 exec.Command("say", "Start working").Output()
 time.Sleep(time.Millisecond * time.Duration(duration))
 exec.Command("say", "Time to Relax").Output()
 start <- true
}
func (p *PomodoroService) StartBreak(endBreak chan bool, duration int) {
 exec.Command("say", "Start short relax").Output()
 time.Sleep(time.Millisecond * time.Duration(duration))
 exec.Command("say", "Relax has finished, getting back to job").Output()
 endBreak <- true
}
func (p *PomodoroService) StartLongBreak(endLong chan bool, duration int) {
 exec.Command("say", "Start long relax").Output()
 time.Sleep(time.Millisecond * time.Duration(duration))
 exec.Command("say", "Long pause has finished").Output()
 endLong <- true
}

• Grajdeanu Alex: Welcome to codereview. I hope you get some nice answers.
• Bruno Costa: Where is the tomato? :(

Answer 1

Your NewPomodoro function should simply be called New (the fact that you're creating a Pomodoro thing is inherent in "you're calling pomodoro.New" and "there's only one New", if there were more, it would make sense to have, say, NewTimer and NewManager).

I don't see what the PomodoroService is actually useful for, it seems to be abstraction for the sake of abstraction. I would simply put the StartBreak, StartLongBreak, and StartWorking methods directly on the timer/manager, as long as this is essentially all there is.

I would probably have called PomodoroManager something else. Probably Pomodoro, PomodoroTimer or (most probably) just Timer (the fact that it's a Pomodoro timer is inherent from being in the pomodoro package).

I am ambivalent about having the various timers and channels in sub-structs. There's few enough that I am not 100% sure not doing so makes the code mode readable. However, since the members are not exported, their types do not need to be exported, since they can't be accessed from outside the package anyway. I would also name the structs in plural, since they clearly contain "channelS", "durationS" and "intervalS".

Since you're never seemingly using the values passed across your channels, you might as well make then chan struct{} instead of chan bool.

I would consider making your intervals to be of type time.Duration instead of int.

I would consider wrapping all the exec.Command("say", ...).Output()" calls in a non-exported utility function (called, say, say).

Answer 2

I took a moment to rewrite your proposal to give you an alternative to compare with.

In this version the Timer consumes a length indefinite list of task (a channel), context are used extensively to solve timeout management and exit sequences. The switching to short/long break is a simple modulo statement of a counter.

package main
import (
 "context"
 "fmt"
 "log"
 "time"
)
type Pomodoro struct {
 Task chan Task
 ShortBreak Task
 LongBreak Task
}
// Run loops until Pomodoro.Task channel is closed or the context is cancelled.
func (p Pomodoro) Run(ctx context.Context) {
 var i int
 // loop forever
 for {
 // wait for a task or exit
 var task Task
 var ok bool
 select {
 case <-ctx.Done():
 return
 case task, ok = <-p.Task:
 if !ok {
 return
 }
 }
 //run the task
 p.doTask(ctx, task)
 //increment the current counter
 i++
 // Once in a while, perform a long break
 if i%4 == 0 {
 p.doTask(ctx, p.LongBreak)
 continue
 }
 // otherwise, make a short break
 p.doTask(ctx, p.ShortBreak)
 }
}
//doTask execute a task until the context was cancelled or the timeout expired
func (p Pomodoro) doTask(ctx context.Context, task Task) {
 if task.Do == nil {
 return
 }
 // setup a cancellable context
 taskCtx, cancel := context.WithCancel(ctx)
 // with a deadline, if any
 if task.Timeout > 0 {
 taskCtx, cancel = context.WithTimeout(ctx, task.Timeout)
 }
 // start the task
 go func() {
 task.Do(taskCtx)
 cancel()
 }()
 // wait for the context to end
 <-taskCtx.Done()
}
// Task is a function with a timeout. Timeout can be set to zero for a task without deadline.
type Task struct {
 Do func(ctx context.Context)
 Timeout time.Duration
}
func main() {
 ctx, cancel := context.WithCancel(context.Background())
 // setup the timer
 var timer Pomodoro
 timer.Task = make(chan Task)
 // short/long break are regular task.
 timer.ShortBreak = Task{
 Timeout: time.Second,
 Do: func(_ context.Context) {
 log.Println("short break")
 },
 }
 timer.LongBreak = Task{
 Timeout: 0, // this task does not use a deadline, it waits for the input.
 Do: func(_ context.Context) {
 fmt.Println("Do you want to continue? Y/N")
 var answer string
 fmt.Scanln(&answer)
 if answer != "Y" {
 // cancel the context to trigger exit sequence
 cancel()
 }
 },
 }
 // push your tasks
 go func() {
 for i := 0; i < 5; i++ {
 e := i
 timer.Task <- Task{
 Timeout: time.Second * 5,
 Do: func(ctx context.Context) {
 w := time.Second*3 + time.Second*time.Duration(e)
 log.Printf("begin task #%v, wait=%v\n", e, w)
 select {
 case <-time.After(w):
 log.Printf("task #%v done\n", e)
 case <-ctx.Done():
 log.Printf("task #%v not ended quick enough", e)
 }
 },
 }
 }
 close(timer.Task) // close this channel to trigger exit sequence.
 }()
 // wait on the timer to finish
 timer.Run(ctx)
 log.Println("all done.")
}