patterngoMinor
Pomodoro timer in Go
Viewed 0 times
timerpomodorostackoverflow
Problem
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).
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
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
Solution
Your
I don't see what the
I would probably have called
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
I would consider making your intervals to be of type
I would consider wrapping all the
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).Context
StackExchange Code Review Q#129447, answer score: 3
Revisions (0)
No revisions yet.