Add Time Exercise: 41_time_delay (#83) - #159
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds a new exercise "41_time_delay": catalog entry, solution implementation with multiple time-based utilities, template stubs, and tests covering waits, notifications, timeouts, elapsed calculation, and scheduling. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Timedelay as timedelay
participant BG as background goroutine
participant Time as time pkg
participant Chan as channel
Note over Caller,Timedelay: Async notifier flow (NotifyAfter / NotifyAt)
Caller->>Timedelay: NotifyAfter(ms) / NotifyAt(target)
Timedelay->>Chan: create buffered channel
Timedelay->>Caller: return Chan
Timedelay->>BG: spawn goroutine
alt target in future
BG->>Time: poll / Sleep(short)
Time-->>BG: tick
BG->>Chan: send true or close
else target in past
BG-->>Chan: send immediately
end
Note over Caller,Timedelay: Synchronous wait flow (WaitFor / WaitUntil)
Caller->>Timedelay: WaitFor(ms) / WaitUntil(target)
Timedelay->>Time: check target / Sleep until reached
Time-->>Timedelay: elapsed
Timedelay-->>Caller: return (or error for timeout)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@kaushalyap, please take a look at this PR and add the Hacktoberfest label. |
kawpii
added
hacktoberfest
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
internal/exercises/solutions/41_time_delay/time_delay.go (4)
10-31: Recommend using stdlib timing facilities instead of busy-wait loops.
WaitForandNotifyAfteruse inefficient busy-wait loops with 1ms sleeps. For an exercise teaching thetimepackage, consider demonstrating the idiomatic stdlib approaches:
WaitForcan simply calltime.Sleep(time.Duration(ms) * time.Millisecond)directlyNotifyAftercan usetime.After(time.Duration(ms) * time.Millisecond)ortime.NewTimer()The current implementations consume unnecessary CPU cycles and miss the opportunity to teach proper Go timing patterns.
Option 1: Direct stdlib usage
func WaitFor(ms int) { - target := time.Now().Add(time.Duration(ms) * time.Millisecond) - for time.Now().Before(target) { - time.Sleep(1 * time.Millisecond) - } + time.Sleep(time.Duration(ms) * time.Millisecond) } func NotifyAfter(ms int) chan bool { - ch := make(chan bool, 1) - target := time.Now().Add(time.Duration(ms) * time.Millisecond) - - go func() { - for time.Now().Before(target) { - time.Sleep(1 * time.Millisecond) - } - ch <- true - }() - - return ch + ch := make(chan bool, 1) + go func() { + time.Sleep(time.Duration(ms) * time.Millisecond) + ch <- true + }() + return ch }Option 2: Using time.After (even more idiomatic for NotifyAfter)
If the exercise goal is to teach channel-based timing,
NotifyAftercould directly returntime.After(), though note it returns<-chan time.Timeinstead ofchan bool.
33-52: Usetime.Until()to eliminate busy-wait loops.
WaitUntilandNotifyAtuse the same inefficient busy-wait pattern. For absolute time targets, usetime.Until(target)to calculate the duration and sleep once:func WaitUntil(target time.Time) { - for time.Now().Before(target) { - time.Sleep(1 * time.Millisecond) + if duration := time.Until(target); duration > 0 { + time.Sleep(duration) } } func NotifyAt(target time.Time) chan bool { ch := make(chan bool, 1) - go func() { - for time.Now().Before(target) { - time.Sleep(1 * time.Millisecond) + if duration := time.Until(target); duration > 0 { + time.Sleep(duration) } ch <- true }() - return ch }
59-74: Consider usingselectwithtime.Afterfor idiomatic timeout handling.The busy-wait loop works but doesn't demonstrate Go's preferred timeout patterns. Consider using
selectwith channels orcontext.WithTimeout:func WaitForOrTimeout(ms int, timeoutMs int) error { - target := time.Now().Add(time.Duration(ms) * time.Millisecond) - deadline := time.Now().Add(time.Duration(timeoutMs) * time.Millisecond) - - for { - now := time.Now() - if !now.Before(target) { - return nil - } - if !now.Before(deadline) { - return errors.New("timeout exceeded before target reached") - } - time.Sleep(1 * time.Millisecond) + if ms > timeoutMs { + return errors.New("timeout exceeded before target reached") + } + + select { + case <-time.After(time.Duration(ms) * time.Millisecond): + return nil + case <-time.After(time.Duration(timeoutMs) * time.Millisecond): + return errors.New("timeout exceeded before target reached") } }Note: The logic above is simplified. If
timeoutMs > ms, both timers fire butselectpicks arbitrarily when both are ready simultaneously. A more robust implementation would use a timeout timer and a wait timer, or checkms > timeoutMsupfront.
76-90: Usetime.AfterFuncor directtime.Sleepfor scheduling.The busy-wait loop in
ScheduleAftercan be replaced with Go's built-in scheduling facilities:Option 1: Using time.AfterFunc
func ScheduleAfter(ms int, fn func()) chan struct{} { done := make(chan struct{}) - target := time.Now().Add(time.Duration(ms) * time.Millisecond) - - go func() { - for time.Now().Before(target) { - time.Sleep(1 * time.Millisecond) - } - fn() - close(done) - }() - + time.AfterFunc(time.Duration(ms)*time.Millisecond, func() { + fn() + close(done) + }) return done }Option 2: Direct sleep (simpler, keeps explicit goroutine)
func ScheduleAfter(ms int, fn func()) chan struct{} { done := make(chan struct{}) - target := time.Now().Add(time.Duration(ms) * time.Millisecond) - go func() { - for time.Now().Before(target) { - time.Sleep(1 * time.Millisecond) - } + time.Sleep(time.Duration(ms) * time.Millisecond) fn() close(done) }() - return done }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/exercises/solutions/41_time_delay/time_delay.go(1 hunks)internal/exercises/templates/41_time_delay/time_delay.go(1 hunks)internal/exercises/templates/41_time_delay/time_delay_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/exercises/templates/41_time_delay/time_delay.go
🧰 Additional context used
🧬 Code graph analysis (1)
internal/exercises/templates/41_time_delay/time_delay_test.go (1)
internal/exercises/solutions/41_time_delay/time_delay.go (7)
WaitFor(11-16)NotifyAfter(19-31)WaitUntil(34-38)NotifyAt(41-52)ElapsedMillis(55-57)WaitForOrTimeout(60-74)ScheduleAfter(77-90)
🪛 GitHub Actions: CI
internal/exercises/templates/41_time_delay/time_delay_test.go
[error] 12-12: vet: undefined: WaitFor
🔇 Additional comments (1)
internal/exercises/solutions/41_time_delay/time_delay.go (1)
54-57: LGTM!This is a clean wrapper around
time.Since()that appropriately demonstrates duration conversion to milliseconds.
|
@kaushalyap, Let me know if there are any improvements or feedback needed. |
|
Please update |
|
@kaushalyap done updating the catalog |
|
@zhravan can you please review it |
Signed-off-by: Sidharth Chauhan <chauhansiddharth71@gmail.com>
2 participants
Summary
Added a new concept exercise for the
timepackage.Changes
41_time_delayundertemplates/andsolutions/.catalog.yamlwith metadata and hints.Concept
Covers key time concepts:
time.Sleep()for delays.time.Durationfor millisecond-based waits.Closes #83
Summary by CodeRabbit