05 - Feat/sorting exercise - #96
Conversation
|
Warning Rate limit exceeded@kaushalyap has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 27 minutes and 55 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughAdds five new concept entries (defer, go routines, channels, mutexes, sorting) to the catalog and introduces corresponding solution and template implementations with tests for each. New code covers file handling with defer, goroutines, channels messaging, mutex-protected counting, and slice sorting. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor T as Test
participant C as channels.ReadMessage
participant G as Goroutine(Sender)
Note over C: Create channel
T->>C: ReadMessage()
activate C
C->>G: start goroutine
activate G
G-->>C: send "Hi!" on chan
deactivate G
C-->>T: return "Hi!"
deactivate C
sequenceDiagram
autonumber
actor T as Test
participant M as mutexes.Counting
participant W as Workers (x10k)
T->>M: Counting()
activate M
Note over M: Initialize counter, mutex, wg
M->>W: start workers
rect rgba(200,230,255,0.3)
loop for each worker
W->>M: lock, counter++, unlock
W-->>M: done
end
end
M-->>T: return final count
deactivate M
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (8)
internal/exercises/templates/28_defer/utils.go (1)
8-23: Singleton pattern correctly implemented, but consider exposing cleanup.The
sync.Onceusage ensures thread-safe singleton initialization. However, the package exposes no cleanup mechanism to closefileInstance. While this is an exercise template where students will practicedefer, consider adding aCloseFile()helper or documenting that callers must manage cleanup.Optional: Add a cleanup helper for completeness:
+// CloseFile closes the singleton file if it exists +func CloseFile() error { + if fileInstance != nil { + return fileInstance.Close() + } + return nil +}internal/exercises/templates/32_sorting/sorting_test.go (1)
8-22: Consider test isolation to avoid shared state issues.Both subtests mutate the global
YearsandPetsslices. If tests run in different orders or are expanded, this shared state could cause flaky results.Consider resetting state between tests:
func TestSorting(t *testing.T) { t.Run("Sorting numbers", func(t *testing.T) { + Years = []int{2017, 2003, 2026} SortYears() if !slices.IsSorted(Years) { - t.Fatal("Strings are not sorted!") + t.Fatal("Numbers are not sorted!") } }) t.Run("Sorting strings", func(t *testing.T) { + Pets = []string{"Dog", "Cat", "Parrot"} SortPets() if !slices.IsSorted(Pets) { t.Fatal("Strings are not sorted!") } }) }internal/exercises/solutions/28_defer/defer.go (1)
10-11: Consider handling errors from Close() and Fprintln().Both
f.Close()andfmt.Fprintln()can return errors that are currently ignored. For teaching purposes, demonstrating proper error handling would be valuable:func WriteToFile(*os.File) { f := CreateFile() - defer f.Close() - fmt.Fprintln(f, "data") + defer func() { + if err := f.Close(); err != nil { + // Handle or log error + } + }() + if _, err := fmt.Fprintln(f, "data"); err != nil { + // Handle error + } }Alternatively, if this is intentionally simplified for the exercise, the current approach is acceptable.
internal/exercises/templates/29_go_routines/utils.go (1)
5-11: Fix naming inconsistency: singular vs plural "Millisecond".The first function uses singular "Millisecond" while the second uses plural "Milliseconds". For consistency, both should use the same form.
-func sleepForOneHundredMillisecond() { +func sleepForOneHundredMilliseconds() { time.Sleep(100 * time.Millisecond) }internal/exercises/solutions/29_go_routines/go_routines.go (1)
7-13: Naming inconsistency: "Millisecond" vs "Milliseconds".Same issue as in the templates/utils.go file - the first function uses singular "Millisecond" while the second uses plural "Milliseconds".
-func sleepForOneHundredMillisecond() { +func sleepForOneHundredMilliseconds() { time.Sleep(100 * time.Millisecond) }internal/exercises/solutions/30_channels/channels.go (1)
3-13: LGTM! Consider simplifying the return statement.The channel implementation is correct. The unbuffered channel properly synchronizes the goroutine and main routine, and there are no goroutine leaks.
For slightly more idiomatic Go, you could simplify lines 10-12:
func ReadMessage() string { chat := make(chan string) go func() { senderMessage := "Hi!" chat <- senderMessage }() - msg := <-chat - - return msg + return <-chat }internal/exercises/solutions/31_mutexes/mutexes.go (1)
7-31: Consider simplifying synchronization for pedagogical clarity.The code correctly uses a mutex to protect the shared counter, which is the primary learning objective. However, it demonstrates two redundant synchronization mechanisms:
- WaitGroup (lines 11, 15, 17, 29) - tracks goroutine completion
- Buffered done channel (lines 10, 21, 25-27) - also tracks completion
Either mechanism alone is sufficient. For a mutex-focused exercise, the extra synchronization complexity may distract learners from the core concept.
Simplify by removing the redundant channel:
func Counting() int { count := 0 - done := make(chan bool, numWorkers) var wg sync.WaitGroup var mu sync.Mutex for i := 1; i <= numWorkers; i++ { wg.Add(1) go func() { defer wg.Done() mu.Lock() count += 1 mu.Unlock() - done <- true }() } - for i := 1; i <= numWorkers; i++ { - <-done - } - wg.Wait() return count }Alternatively, if you want to teach channels for synchronization, remove the WaitGroup and keep the channel. However, WaitGroup is more conventional for this pattern.
internal/exercises/templates/31_mutexes/mutexes.go (1)
10-26: Consider simplifying the synchronization pattern.The function uses both a
sync.WaitGroupand a buffered channel for coordination, which is redundant. Idiomatic Go typically uses one or the other:
- Option 1 (simpler): Use only
WaitGroup.Wait()and remove the channel entirely.- Option 2: Use only the channel and remove the
WaitGroup.Option 1: Use only WaitGroup (simpler and more common):
func Counting() int { count := 0 - done := make(chan bool, numWorkers) var wg sync.WaitGroup for i := 1; i <= numWorkers; i++ { wg.Add(1) go func() { defer wg.Done() count += 1 - done <- true }() } - for i := 1; i <= numWorkers; i++ { - <-done - } - wg.Wait() return count }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
internal/exercises/catalog.yaml(1 hunks)internal/exercises/solutions/28_defer/defer.go(1 hunks)internal/exercises/solutions/29_go_routines/go_routines.go(1 hunks)internal/exercises/solutions/30_channels/channels.go(1 hunks)internal/exercises/solutions/31_mutexes/mutexes.go(1 hunks)internal/exercises/solutions/32_sorting/sorting.go(1 hunks)internal/exercises/templates/28_defer/defer.go(1 hunks)internal/exercises/templates/28_defer/defer_test.go(1 hunks)internal/exercises/templates/28_defer/utils.go(1 hunks)internal/exercises/templates/29_go_routines/go_routines.go(1 hunks)internal/exercises/templates/29_go_routines/go_routines_test.go(1 hunks)internal/exercises/templates/29_go_routines/utils.go(1 hunks)internal/exercises/templates/30_channels/channels.go(1 hunks)internal/exercises/templates/30_channels/channels_test.go(1 hunks)internal/exercises/templates/31_mutexes/mutexes.go(1 hunks)internal/exercises/templates/31_mutexes/mutexes_test.go(1 hunks)internal/exercises/templates/32_sorting/sorting.go(1 hunks)internal/exercises/templates/32_sorting/sorting_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (15)
internal/exercises/templates/29_go_routines/go_routines_test.go (2)
internal/exercises/solutions/29_go_routines/go_routines.go (1)
RunConcurrently(15-26)internal/exercises/templates/29_go_routines/go_routines.go (1)
RunConcurrently(3-6)
internal/exercises/templates/29_go_routines/go_routines.go (1)
internal/exercises/solutions/29_go_routines/go_routines.go (1)
RunConcurrently(15-26)
internal/exercises/templates/32_sorting/sorting.go (1)
internal/exercises/solutions/32_sorting/sorting.go (4)
SortYears(10-13)Years(7-7)SortPets(15-18)Pets(8-8)
internal/exercises/templates/28_defer/defer_test.go (3)
internal/exercises/templates/28_defer/utils.go (1)
CreateFile(14-23)internal/exercises/solutions/28_defer/defer.go (1)
WriteToFile(8-12)internal/exercises/templates/28_defer/defer.go (1)
WriteToFile(8-11)
internal/exercises/templates/31_mutexes/mutexes.go (1)
internal/exercises/solutions/31_mutexes/mutexes.go (1)
Counting(7-31)
internal/exercises/templates/32_sorting/sorting_test.go (1)
internal/exercises/templates/32_sorting/sorting.go (4)
SortYears(6-8)Years(3-3)SortPets(10-12)Pets(4-4)
internal/exercises/solutions/31_mutexes/mutexes.go (1)
internal/exercises/templates/31_mutexes/mutexes.go (1)
Counting(7-28)
internal/exercises/templates/28_defer/defer.go (2)
internal/exercises/solutions/28_defer/defer.go (1)
WriteToFile(8-12)internal/exercises/templates/28_defer/utils.go (1)
CreateFile(14-23)
internal/exercises/templates/31_mutexes/mutexes_test.go (2)
internal/exercises/solutions/31_mutexes/mutexes.go (1)
Counting(7-31)internal/exercises/templates/31_mutexes/mutexes.go (1)
Counting(7-28)
internal/exercises/solutions/28_defer/defer.go (2)
internal/exercises/templates/28_defer/defer.go (1)
WriteToFile(8-11)internal/exercises/templates/28_defer/utils.go (1)
CreateFile(14-23)
internal/exercises/templates/30_channels/channels_test.go (2)
internal/exercises/solutions/30_channels/channels.go (1)
ReadMessage(3-13)internal/exercises/templates/30_channels/channels.go (1)
ReadMessage(5-15)
internal/exercises/solutions/30_channels/channels.go (1)
internal/exercises/templates/30_channels/channels.go (1)
ReadMessage(5-15)
internal/exercises/solutions/29_go_routines/go_routines.go (1)
internal/exercises/templates/29_go_routines/go_routines.go (1)
RunConcurrently(3-6)
internal/exercises/templates/30_channels/channels.go (1)
internal/exercises/solutions/30_channels/channels.go (1)
ReadMessage(3-13)
internal/exercises/solutions/32_sorting/sorting.go (1)
internal/exercises/templates/32_sorting/sorting.go (4)
SortYears(6-8)Years(3-3)SortPets(10-12)Pets(4-4)
🔇 Additional comments (13)
internal/exercises/templates/32_sorting/sorting.go (1)
1-12: Exercise template correctly structured.The template provides the correct scaffold for students to implement sorting. Functions currently return unsorted slices, which is the expected starting point. Students should add
slices.Sort()calls to complete the exercise, as shown in the solution file.internal/exercises/solutions/32_sorting/sorting.go (1)
7-18: Solution implementation is correct.The solution properly uses
slices.Sortfor in-place sorting of both integer and string slices. The in-place modification of global variables is consistent with the exercise template design.internal/exercises/templates/30_channels/channels_test.go (1)
5-12: Test correctly validates channel communication.The test straightforwardly verifies that
ReadMessage()returns the expected message sent via channel. The assertion is clear and the error message is descriptive.internal/exercises/templates/28_defer/defer_test.go (1)
9-10: Potential test isolation issue with singleton CreateFile().
CreateFile()usessync.Onceto return a singleton file instance. If tests run in parallel or if multiple tests callCreateFile(), they'll all share the same file handle, which could lead to:
- Race conditions on file operations
- Tests interfering with each other
- Flaky test behavior
Verify that:
- Tests in this package don't run in parallel, or
- The singleton pattern is intentional for the exercise's teaching purpose
internal/exercises/templates/31_mutexes/mutexes_test.go (1)
5-12: Test may be flaky on template version due to race condition.The test expects
Counting()to return exactlynumWorkers. However, the template implementation (shown in snippets) incrementscountwithout mutex protection, creating a race condition.With 10,000 workers, the race will likely manifest as
count < numWorkers, but the test could sporadically pass if lucky timing aligns. This non-determinism may confuse learners.Consider adding a comment in the test or exercise description explaining:
- The template version demonstrates a race condition
- The test is expected to fail (or produce inconsistent results) until learners add mutex protection
- Running with
-raceflag will definitively catch the issueAlternatively, verify this is the intended teaching approach.
internal/exercises/templates/29_go_routines/go_routines.go (1)
3-6: LGTM! Template is correctly structured for learning.This template provides appropriate starter code for learners to practice adding the
gokeyword. The sequential implementation is intentional.Note: Ensure the test file (go_routines_test.go) correctly validates concurrent execution—there are critical issues with the test logic flagged separately.
internal/exercises/templates/30_channels/channels.go (1)
5-15: LGTM! Template appropriately scaffolds the learning exercise.The starter code provides clear structure with a helpful TODO comment. Learners will naturally eliminate the
fmt.Printlnside effect when they implement the channel communication.Note: There's a potential race condition where the program might return before "Hi!" is printed, but this will be resolved when learners complete the exercise by using a channel for synchronization.
internal/exercises/templates/28_defer/defer.go (2)
8-11: The missingdeferstatement is the intended learning exercise.As an exercise template, the absence of
defer f.Close()is intentional—learners should add it based on the catalog hint. The template structure is appropriate for teaching deferred execution.
8-11: Remove the unused parameter from the function signature.The
*os.Fileparameter is declared but never used. The function creates its own file viaCreateFile()and ignores the parameter entirely. This creates confusion about the function's intent.Apply this diff to remove the unused parameter:
-func WriteToFile(*os.File) { +func WriteToFile() { f := CreateFile() fmt.Fprintln(f, "ABC") }Note: If this signature is required by a test interface, consider naming the parameter with
_to explicitly mark it as unused:func WriteToFile(_ *os.File).Likely an incorrect or invalid review comment.
internal/exercises/templates/31_mutexes/mutexes.go (1)
17-17: The missing mutex protection is the intended learning exercise.The unsynchronized increment
count += 1creates a race condition when multiple goroutines execute concurrently. This is intentional—learners should add mutex protection as indicated by the catalog hint. The template structure correctly demonstrates the problem that mutexes solve.internal/exercises/catalog.yaml (3)
137-161: Verify the scope alignment with PR objectives.The PR description states "Adds sorting exercises for sorting slices" (issue #72), but this catalog update introduces five new concepts (defer, go_routines, channels, mutexes, sorting). Confirm whether:
- All five concepts are intentionally part of this PR, or
- Only the sorting exercise should be included and the others were added inadvertently.
If all five are intentional, consider updating the PR description to reflect the broader scope.
137-161: The new concept entries and hints are well-structured.The five new concept entries (28_defer through 32_sorting) follow the established catalog pattern consistently. Each includes appropriate titles, test patterns, and helpful hints that guide learners effectively.
164-213: Project renumbering to 100s range improves organization.Moving projects from the 28-36 range to 101-109 creates clear separation between concepts and projects, making the catalog structure more maintainable and allowing room for additional concepts.
| func WriteToFile(*os.File) { | ||
| f := CreateFile() | ||
| defer f.Close() | ||
| fmt.Fprintln(f, "data") | ||
| } |
There was a problem hiding this comment.
Remove unused parameter or use it instead of calling CreateFile().
The function signature accepts *os.File but ignores it, calling CreateFile() instead. This is confusing and creates an inconsistent API.
Consider one of these approaches:
-
Remove the parameter if
CreateFile()should always be used:func WriteToFile() { f := CreateFile() defer f.Close() fmt.Fprintln(f, "data") }
-
Use the parameter if the file should be provided by the caller:
func WriteToFile(f *os.File) { defer f.Close() fmt.Fprintln(f, "data") }
The second approach is more flexible and matches the function signature, but verify that the test expectations align with the chosen design.
🤖 Prompt for AI Agents
internal/exercises/solutions/28_defer/defer.go lines 8-12: the function
signature declares a *os.File parameter but ignores it and always calls
CreateFile(); update the implementation to use the provided parameter instead of
calling CreateFile() — remove the CreateFile() call, defer closing the passed-in
file (defer f.Close()), and write to that file with fmt.Fprintln(f, "data");
ensure tests expecting the caller to supply the file still pass.
| func RunConcurrently() { | ||
| go sleepForOneHundredMillisecond() | ||
| go sleepForTwoHundredMilliseconds() | ||
|
|
||
| // Alternatively : Anonymous Goroutines | ||
| // go func() { | ||
| // sleepForOneHundredMillisecond() | ||
| // }() | ||
| // go func() { | ||
| // sleepForTwoHundredMilliseconds() | ||
| // }() | ||
| } |
There was a problem hiding this comment.
Critical: Goroutines launched without synchronization.
Lines 16-17 launch goroutines with go keyword but RunConcurrently() returns immediately without waiting for them to complete. This means:
- The goroutines may not execute before the program exits
- Tests calling this function may have flaky results
- The behavior is non-deterministic
For a solution/reference implementation, proper synchronization is essential:
+import (
+ "sync"
+ "time"
+)
+
func RunConcurrently() {
- go sleepForOneHundredMillisecond()
- go sleepForTwoHundredMilliseconds()
+ var wg sync.WaitGroup
+ wg.Add(2)
+
+ go func() {
+ defer wg.Done()
+ sleepForOneHundredMillisecond()
+ }()
+ go func() {
+ defer wg.Done()
+ sleepForTwoHundredMilliseconds()
+ }()
+
+ wg.Wait()
-
- // Alternatively : Anonymous Goroutines
- // go func() {
- // sleepForOneHundredMillisecond()
- // }()
- // go func() {
- // sleepForTwoHundredMilliseconds()
- // }()
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func RunConcurrently() { | |
| go sleepForOneHundredMillisecond() | |
| go sleepForTwoHundredMilliseconds() | |
| // Alternatively : Anonymous Goroutines | |
| // go func() { | |
| // sleepForOneHundredMillisecond() | |
| // }() | |
| // go func() { | |
| // sleepForTwoHundredMilliseconds() | |
| // }() | |
| } | |
| import ( | |
| "sync" | |
| "time" | |
| ) | |
| func RunConcurrently() { | |
| var wg sync.WaitGroup | |
| wg.Add(2) | |
| go func() { | |
| defer wg.Done() | |
| sleepForOneHundredMillisecond() | |
| }() | |
| go func() { | |
| defer wg.Done() | |
| sleepForTwoHundredMilliseconds() | |
| }() | |
| wg.Wait() | |
| } |
🤖 Prompt for AI Agents
In internal/exercises/solutions/29_go_routines/go_routines.go around lines 15 to
26, the function launches two goroutines and returns immediately, causing
non-deterministic behavior and flaky tests; fix by synchronizing: create a
sync.WaitGroup, Add(2) before starting goroutines, have each goroutine call
Done() when finished, and call wg.Wait() at the end of RunConcurrently to block
until both goroutines complete (alternatively use a two-value channel and block
reading two results) so the function only returns after both sleep routines
finish.
| func TestClosingFileAfterWriting(t *testing.T) { | ||
| f := CreateFile() | ||
| WriteToFile(f) | ||
|
|
||
| // Trying to write to the file after closing | ||
| _, err := fmt.Fprintln(f, "123") | ||
| if err == nil { | ||
| t.Fatal("File wasn't closed!") | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Test relies on confusing API where WriteToFile ignores its parameter.
The test passes a file handle to WriteToFile(f) on line 10, but as noted in the solution file review, WriteToFile ignores this parameter and calls CreateFile() internally. This makes the test's intent unclear.
Additionally, since CreateFile() uses a singleton pattern (sync.Once), this test depends on:
- Being the first/only test to run, or
WriteToFileclosing the singleton file instance
Consider clarifying the API:
- If
WriteToFileshould accept and use the file parameter, update the implementation - If
WriteToFileshould always useCreateFile(), remove the parameter from both the function signature and this test call
🏁 Script executed:
#!/bin/bash
# Locate WriteToFile and CreateFile implementations
rg -n --type=go 'func WriteToFile' -g '*.go'
rg -n --type=go 'func CreateFile' -g '*.go'Length of output: 234
🏁 Script executed:
#!/bin/bash
# Extract WriteToFile implementation
sed -n '1,50p' internal/exercises/templates/28_defer/defer.go | sed -n '1,50p'
# Extract CreateFile and sync.Once usage
sed -n '1,200p' internal/exercises/templates/28_defer/utils.go | sed -n '1,200p'Length of output: 424
Clarify WriteToFile API to use or remove its *os.File parameter
- WriteToFile (internal/exercises/templates/28_defer/defer.go:8) ignores its
*os.Fileargument and unconditionally callsCreateFile(), so the test’s call on line 10 is misleading. - CreateFile() uses a singleton (
sync.Once), making the test’s behavior dependent on execution order or implicit closing.
Choose one:
• Have WriteToFile write to the passed-in file, or
• Drop the file parameter (and its use in the test) to reflect its singleton design.
🤖 Prompt for AI Agents
In internal/exercises/templates/28_defer/defer_test.go around lines 8 to 17 the
test calls WriteToFile(f) but WriteToFile currently ignores its *os.File
parameter and instead calls CreateFile() (which itself is a singleton via
sync.Once), making the test misleading and order-dependent; fix by choosing one
of two options and applying matching changes: either (A) make WriteToFile
actually write to the provided *os.File (remove its internal CreateFile call)
and keep the test as-is so it creates a file, passes it to WriteToFile, then
closes it and asserts writes fail; or (B) remove the *os.File parameter from
WriteToFile and update the test to call WriteToFile() (no arg) to reflect the
singleton CreateFile usage; in either case update the function signature,
implementation, and the test call sites consistently and ensure file closing
behavior in the test remains correct.
| func TestGoRoutines(t *testing.T) { | ||
| start := time.Now() | ||
| RunConcurrently() | ||
| elapsed := time.Since(start) | ||
|
|
||
| // according to benchmark it is about ~1.5 microseconds | ||
| // x2 for more breathing room, so 3 | ||
| if elapsed.Microseconds() >= 3 { | ||
| t.Fatal("Go routines was not used!") | ||
| } | ||
| } |
There was a problem hiding this comment.
Test logic is critically flawed and will produce false positives.
The test has multiple issues:
-
Incorrect timing threshold: The test expects concurrent execution to complete in <3 microseconds, but goroutines that sleep for 100ms and 200ms will take at minimum ~200ms (the duration of the longer sleep) when properly concurrent. The test will pass for the wrong reason—if
RunConcurrently()spawns goroutines without waiting for them (as the template version does), the function returns immediately (~microseconds), and the test passes even though the work isn't complete. -
Missing synchronization: The template's
RunConcurrently()(lines 3-6 of go_routines.go) runs sequentially. The solution spawns goroutines but doesn't wait for them to complete. A correct concurrent implementation needs atime.Sleep()orsync.WaitGroupto ensure goroutines finish before returning. -
Inverted expectation: Sequential execution should take ~300ms (100ms + 200ms). Concurrent should take ~200ms (max of the two). The test should verify
elapsed < sequentialTime, not check microsecond timing.
Apply this diff to fix the test logic:
func TestGoRoutines(t *testing.T) {
start := time.Now()
RunConcurrently()
elapsed := time.Since(start)
- // according to benchmark it is about ~1.5 microseconds
- // x2 for more breathing room, so 3
- if elapsed.Microseconds() >= 3 {
- t.Fatal("Go routines was not used!")
+ // Sequential execution would take ~300ms (100ms + 200ms)
+ // Concurrent execution should take ~200ms (max of the two)
+ // Allow 250ms threshold to account for scheduling overhead
+ if elapsed.Milliseconds() >= 250 {
+ t.Fatalf("Go routines was not used! Elapsed: %v", elapsed)
}
}Note: The solution's RunConcurrently() also needs synchronization (e.g., time.Sleep(250*time.Millisecond) at the end) to wait for goroutines to complete.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestGoRoutines(t *testing.T) { | |
| start := time.Now() | |
| RunConcurrently() | |
| elapsed := time.Since(start) | |
| // according to benchmark it is about ~1.5 microseconds | |
| // x2 for more breathing room, so 3 | |
| if elapsed.Microseconds() >= 3 { | |
| t.Fatal("Go routines was not used!") | |
| } | |
| } | |
| func TestGoRoutines(t *testing.T) { | |
| start := time.Now() | |
| RunConcurrently() | |
| elapsed := time.Since(start) | |
| // Sequential execution would take ~300ms (100ms + 200ms) | |
| // Concurrent execution should take ~200ms (max of the two) | |
| // Allow 250ms threshold to account for scheduling overhead | |
| if elapsed.Milliseconds() >= 250 { | |
| t.Fatalf("Go routines was not used! Elapsed: %v", elapsed) | |
| } | |
| } |
🤖 Prompt for AI Agents
internal/exercises/templates/29_go_routines/go_routines_test.go lines 8-18: the
test’s timing and synchronization are wrong — it currently asserts elapsed < 3µs
which yields false positives if RunConcurrently returns before goroutines
complete. Fix by measuring the sequential baseline (e.g., sequential :=
100*time.Millisecond + 200*time.Millisecond), ensure RunConcurrently waits for
work to finish (prefer sync.WaitGroup or a final sleep in the implementation),
then assert the concurrent elapsed is less than the sequential baseline (use a
tolerant threshold like elapsed < 250*time.Millisecond) and fail otherwise.
| t.Run("Sorting numbers", func(t *testing.T) { | ||
| SortYears() | ||
| if !slices.IsSorted(Years) { | ||
| t.Fatal("Strings are not sorted!") | ||
| } | ||
| }) |
There was a problem hiding this comment.
Fix incorrect error message.
Line 12 reports "Strings are not sorted!" when testing numbers. The message should accurately reflect that integers are being validated.
Apply this diff:
t.Run("Sorting numbers", func(t *testing.T) {
SortYears()
if !slices.IsSorted(Years) {
- t.Fatal("Strings are not sorted!")
+ t.Fatal("Numbers are not sorted!")
}
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| t.Run("Sorting numbers", func(t *testing.T) { | |
| SortYears() | |
| if !slices.IsSorted(Years) { | |
| t.Fatal("Strings are not sorted!") | |
| } | |
| }) | |
| t.Run("Sorting numbers", func(t *testing.T) { | |
| SortYears() | |
| if !slices.IsSorted(Years) { | |
| t.Fatal("Numbers are not sorted!") | |
| } | |
| }) |
🤖 Prompt for AI Agents
In internal/exercises/templates/32_sorting/sorting_test.go around lines 9 to 14,
the test's failure message incorrectly says "Strings are not sorted!" while
validating integer sorting; update the t.Fatal call to use an accurate message
such as "Numbers are not sorted!" or "Years are not sorted!" so the error
reflects that integers (Years) are being tested.
a0eab8e to
94fd4b3
Compare
|
Rebased and updated! |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
internal/exercises/solutions/29_go_routines/go_routines.go (1)
15-25: Critical: Goroutines launched without synchronization.
RunConcurrently()launches goroutines at lines 16-17 but returns immediately without waiting for them to complete. This causes:
- Goroutines may not execute before program exit
- Flaky test results
- Non-deterministic behavior
For a solution/reference implementation, proper synchronization is essential.
Apply this diff to fix:
+import ( + "sync" + "time" +) + func RunConcurrently() { + var wg sync.WaitGroup + wg.Add(2) + + go func() { + defer wg.Done() + sleepForOneHundredMillisecond() + }() + go func() { + defer wg.Done() + sleepForTwoHundredMilliseconds() + }() + + wg.Wait() - go sleepForOneHundredMillisecond() - go sleepForTwoHundredMilliseconds() - - // go func() { - // sleepForOneHundredMillisecond() - // }() - // go func() { - // sleepForTwoHundredMilliseconds() - // }() }internal/exercises/templates/29_go_routines/go_routines_test.go (1)
8-18: Test logic is critically flawed and will produce false positives.The test has multiple issues:
Incorrect timing threshold: Line 15 expects <4 microseconds, but goroutines sleeping for 100ms and 200ms will take at minimum ~200ms when properly concurrent. The test passes for the wrong reason—if
RunConcurrently()returns immediately without waiting (as both template and solution currently do), the test passes even though work isn't complete.Missing synchronization: The solution's
RunConcurrently()spawns goroutines but doesn't wait. A correct concurrent implementation needssync.WaitGroupto ensure goroutines finish before returning.Inverted expectation: Sequential execution takes ~300ms (100ms + 200ms). Concurrent should take ~200ms (max of the two). Test should verify
elapsed < sequentialTime, not check microsecond timing.Apply this diff to fix:
func TestGoRoutines(t *testing.T) { start := time.Now() RunConcurrently() elapsed := time.Since(start) - // according to benchmark it is about ~2 microseconds - // x2 for more breathing room, so 4 - if elapsed.Microseconds() >= 4 { - t.Fatal("Go routines was not used!") + // Sequential execution would take ~300ms (100ms + 200ms) + // Concurrent execution should take ~200ms (max of the two) + // Allow 250ms threshold to account for scheduling overhead + if elapsed.Milliseconds() >= 250 { + t.Fatalf("Go routines was not used! Elapsed: %v", elapsed) } }Note: The solution's
RunConcurrently()also needs synchronization (see review comment on solutions/29_go_routines/go_routines.go).internal/exercises/templates/28_defer/defer_test.go (1)
8-17: Test parameter usage remains pedagogically unclear despite intentional design.The test passes
ftoWriteToFile(f)on line 10, butWriteToFileignores this parameter and callsCreateFile()internally. While the retrieved learning indicates this is intentional pedagogy for teaching defer concepts, the API remains confusing for learners who expect the passed parameter to be used.For a defer-focused exercise, consider whether the learning objective is better served by either:
- Having
WriteToFile()accept no parameters (emphasizing singleton + defer pattern), or- Having
WriteToFile(f)actually use the provided file (emphasizing defer with caller-provided resources)Based on learnings.
internal/exercises/solutions/28_defer/defer.go (1)
25-29: Solution demonstrates unused parameter pattern despite pedagogical intent.The function signature declares
*os.Filebut ignores it, always callingCreateFile()instead. Past reviews flagged this as confusing API design. While the retrieved learning indicates this is intentionally part of the defer pedagogy, teaching this pattern in a solution file may reinforce practices that would be problematic in production code.Based on learnings.
internal/exercises/templates/28_defer/defer.go (1)
25-29: Template preserves the unused parameter pattern from solution.The function accepts
*os.Filebut usesCreateFile()instead on line 26. The TODO on line 27 guides students to add defer cleanup, but the ignored parameter may confuse learners about Go function parameter conventions. While the retrieved learning indicates this design is intentional for defer pedagogy, the pattern remains potentially misleading for teaching idiomatic Go.Based on learnings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
internal/exercises/catalog.yaml(1 hunks)internal/exercises/solutions/28_defer/defer.go(1 hunks)internal/exercises/solutions/29_go_routines/go_routines.go(1 hunks)internal/exercises/solutions/30_channels/channels.go(1 hunks)internal/exercises/solutions/31_mutexes/mutexes.go(1 hunks)internal/exercises/solutions/32_sorting/sorting.go(1 hunks)internal/exercises/templates/28_defer/defer.go(1 hunks)internal/exercises/templates/28_defer/defer_test.go(1 hunks)internal/exercises/templates/29_go_routines/go_routines.go(1 hunks)internal/exercises/templates/29_go_routines/go_routines_test.go(1 hunks)internal/exercises/templates/30_channels/channels.go(1 hunks)internal/exercises/templates/30_channels/channels_test.go(1 hunks)internal/exercises/templates/31_mutexes/mutexes.go(1 hunks)internal/exercises/templates/31_mutexes/mutexes_test.go(1 hunks)internal/exercises/templates/32_sorting/sorting.go(1 hunks)internal/exercises/templates/32_sorting/sorting_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- internal/exercises/catalog.yaml
- internal/exercises/solutions/30_channels/channels.go
- internal/exercises/templates/31_mutexes/mutexes_test.go
- internal/exercises/solutions/31_mutexes/mutexes.go
- internal/exercises/templates/32_sorting/sorting_test.go
- internal/exercises/templates/32_sorting/sorting.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-02T06:30:47.143Z
Learnt from: kaushalyap
PR: zhravan/golearn#110
File: internal/exercises/templates/28_defer/defer.go:14-19
Timestamp: 2025-10-02T06:30:47.143Z
Learning: In the defer exercise template (internal/exercises/templates/28_defer/defer.go), the unused *os.File parameter in WriteToFile is intentional and part of the pedagogical design for teaching defer concepts.
Applied to files:
internal/exercises/solutions/28_defer/defer.gointernal/exercises/templates/28_defer/defer.gointernal/exercises/templates/28_defer/defer_test.go
🧬 Code graph analysis (8)
internal/exercises/templates/30_channels/channels_test.go (2)
internal/exercises/solutions/30_channels/channels.go (1)
ReadMessage(3-13)internal/exercises/templates/30_channels/channels.go (1)
ReadMessage(5-15)
internal/exercises/templates/30_channels/channels.go (1)
internal/exercises/solutions/30_channels/channels.go (1)
ReadMessage(3-13)
internal/exercises/solutions/29_go_routines/go_routines.go (1)
internal/exercises/templates/29_go_routines/go_routines.go (1)
RunConcurrently(15-19)
internal/exercises/templates/29_go_routines/go_routines.go (1)
internal/exercises/solutions/29_go_routines/go_routines.go (1)
RunConcurrently(15-25)
internal/exercises/templates/28_defer/defer.go (1)
internal/exercises/solutions/28_defer/defer.go (2)
CreateFile(14-23)WriteToFile(25-29)
internal/exercises/templates/28_defer/defer_test.go (2)
internal/exercises/solutions/28_defer/defer.go (2)
CreateFile(14-23)WriteToFile(25-29)internal/exercises/templates/28_defer/defer.go (2)
CreateFile(14-23)WriteToFile(25-29)
internal/exercises/templates/29_go_routines/go_routines_test.go (2)
internal/exercises/solutions/29_go_routines/go_routines.go (1)
RunConcurrently(15-25)internal/exercises/templates/29_go_routines/go_routines.go (1)
RunConcurrently(15-19)
internal/exercises/templates/31_mutexes/mutexes.go (1)
internal/exercises/solutions/31_mutexes/mutexes.go (1)
Counting(7-31)
🔇 Additional comments (9)
internal/exercises/templates/30_channels/channels_test.go (1)
5-12: LGTM! Clear test for the channels exercise.The test correctly validates that
ReadMessage()returns "Hi!" from the channel-based goroutine communication. The test will intentionally fail against the template (which returns "") until learners complete the exercise, providing clear feedback through the error message.internal/exercises/templates/30_channels/channels.go (1)
5-15: LGTM! Template appropriately demonstrates the problem learners must solve.The incomplete implementation correctly sets up the learning scenario:
- The goroutine prints directly to stdout instead of using a channel
- The function returns "" immediately, creating a race condition
- The TODO clearly guides learners to introduce channel-based communication
This design effectively illustrates why channels are needed for goroutine communication. Once learners implement the solution (as shown in the reference solution), the goroutine will send "Hi!" through a channel, and the main routine will receive and return it.
internal/exercises/solutions/29_go_routines/go_routines.go (1)
7-13: LGTM!The helper functions correctly implement the required sleep durations using
time.Sleep.internal/exercises/templates/29_go_routines/go_routines_test.go (1)
20-24: LGTM!The benchmark function is now correctly named
BenchmarkRunConcurrentlyand properly exercises the function under test.internal/exercises/templates/29_go_routines/go_routines.go (2)
7-13: LGTM!The helper functions correctly implement the required sleep durations and match the solution implementation.
15-19: LGTM!The template correctly provides a sequential starting point with a clear TODO comment. Learners will update this to use goroutines as part of the exercise.
internal/exercises/solutions/32_sorting/sorting.go (3)
1-5: LGTM!Package declaration and import are appropriate. Using the
slicespackage is the modern, idiomatic approach for sorting in Go 1.21+.
10-13: Sorting logic is correct.The use of
slices.Sortis appropriate and will correctly sort integers in ascending order. However, the mutation of global state remains a concern (flagged separately).
15-18: Sorting logic is correct.The use of
slices.Sortis appropriate and will correctly sort strings in lexicographic order. The sample data demonstrates sorting of multi-character strings as specified in the PR objectives.
| var Years = []int{2017, 2003, 2026} | ||
| var Pets = []string{"Dog", "Cat", "Parrot"} |
There was a problem hiding this comment.
Refactor to avoid mutable global state.
Exported package-level variables that are mutated by the sorting functions create several issues:
- Non-idempotent behavior: After the first call, the data is already sorted, making subsequent calls redundant.
- Test reliability: If tests run multiple times or in parallel, shared mutable state can cause unpredictable results.
- Poor practice demonstration: For an educational exercise, this pattern teaches anti-patterns rather than best practices.
Consider refactoring to accept input parameters and return new sorted slices:
-var Years = []int{2017, 2003, 2026}
-var Pets = []string{"Dog", "Cat", "Parrot"}
-
-func SortNumbers() []int {
- slices.Sort(Years)
- return Years
+func SortNumbers(numbers []int) []int {
+ sorted := make([]int, len(numbers))
+ copy(sorted, numbers)
+ slices.Sort(sorted)
+ return sorted
}
-func SortAnimals() []string {
- slices.Sort(Pets)
- return Pets
+func SortAnimals(animals []string) []string {
+ sorted := make([]string, len(animals))
+ copy(sorted, animals)
+ slices.Sort(sorted)
+ return sorted
}Alternatively, if in-place sorting is the intended teaching point, document this behavior clearly and accept the slice as a parameter.
🤖 Prompt for AI Agents
internal/exercises/solutions/32_sorting/sorting.go lines 7-8: the file currently
defines exported package-level mutable variables Years and Pets which causes
non-idempotent behavior and test flakiness; refactor by removing these exported
globals and instead implement functions that accept a slice parameter and return
a sorted copy (or, if demonstrating in-place sorting, accept a slice parameter
and clearly document/make the function name reflect that it mutates the input),
ensure you do not mutate package state, and update callers/tests to pass slices
and receive sorted results (or pass slices explicitly when in-place).
| for i := 1; i <= numWorkers; i++ { | ||
| wg.Add(1) | ||
| go func() { | ||
| defer wg.Done() | ||
| count += 1 | ||
| done <- true | ||
| }() |
There was a problem hiding this comment.
Synchronize the shared counter to avoid races.
count += 1 runs concurrently without a mutex, so increments are lost and Counting() can return less than numWorkers, making the template fail its own test. Please guard the increment with a sync.Mutex.
count := 0
done := make(chan bool, numWorkers)
var wg sync.WaitGroup
+ var mu sync.Mutex
for i := 1; i <= numWorkers; i++ {
wg.Add(1)
go func() {
defer wg.Done()
- count += 1
+ mu.Lock()
+ count++
+ mu.Unlock()
done <- true
}()
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for i := 1; i <= numWorkers; i++ { | |
| wg.Add(1) | |
| go func() { | |
| defer wg.Done() | |
| count += 1 | |
| done <- true | |
| }() | |
| count := 0 | |
| done := make(chan bool, numWorkers) | |
| var wg sync.WaitGroup | |
| var mu sync.Mutex | |
| for i := 1; i <= numWorkers; i++ { | |
| wg.Add(1) | |
| go func() { | |
| defer wg.Done() | |
| mu.Lock() | |
| count++ | |
| mu.Unlock() | |
| done <- true | |
| }() | |
| } |
🤖 Prompt for AI Agents
In internal/exercises/templates/31_mutexes/mutexes.go around lines 15 to 21, the
concurrent increment "count += 1" is not synchronized which causes data races
and lost increments; declare a sync.Mutex (e.g., var mu sync.Mutex) in the
surrounding scope and inside the goroutine lock the mutex before incrementing
and unlock it afterward (mu.Lock(); count += 1; mu.Unlock()), leaving wg.Done
and done <- true as-is so each worker still signals completion.
|
Rebased and updated! |
1 participant
Summary
Adds sorting exercises for sorting slices. Contain two cases for both number and string slices, to show sorting strings (multiple characters) also possible, not just numbers.
Checklist
make verifyorgolearn verify <slug>(CLI does not work as said in an issue, but tested locally usinggo test.)Screenshots / Output (if CLI UX)
Paste before/after where helpful.
Related issues
Fixes #72
Summary by CodeRabbit