-
Notifications
You must be signed in to change notification settings - Fork 20
05 - Feat/sorting exercise #96
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 5 commits
6326629
04d79c0
3696939
8f02373
3d54262
439f692
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| package deferr | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "os" | ||
| "sync" | ||
| ) | ||
|
|
||
| var ( | ||
| fileInstance *os.File | ||
| once sync.Once | ||
| ) | ||
|
|
||
| func CreateFile() *os.File { | ||
| once.Do(func() { | ||
| f, err := os.CreateTemp("", "example.txt") | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| fileInstance = f | ||
| }) | ||
| return fileInstance | ||
| } | ||
|
|
||
| func WriteToFile(*os.File) { | ||
| f := CreateFile() | ||
| defer f.Close() | ||
| fmt.Fprintln(f, "data") | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,25 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| package go_routines | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "time" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| func sleepForOneHundredMillisecond() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| time.Sleep(100 * time.Millisecond) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| func sleepForTwoHundredMilliseconds() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| time.Sleep(200 * time.Millisecond) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| func RunConcurrently() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| go sleepForOneHundredMillisecond() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| go sleepForTwoHundredMilliseconds() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // go func() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // sleepForOneHundredMillisecond() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // }() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // go func() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // sleepForTwoHundredMilliseconds() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // }() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+15
to
+25
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Critical: Goroutines launched without synchronization. Lines 16-17 launch goroutines with
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
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| package channels | ||
|
|
||
| func ReadMessage() string { | ||
| chat := make(chan string) | ||
| go func() { | ||
| senderMessage := "Hi!" | ||
| chat <- senderMessage | ||
| }() | ||
|
|
||
| msg := <-chat | ||
|
|
||
| return msg | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| package mutexes | ||
|
|
||
| import "sync" | ||
|
|
||
| const numWorkers = 10_000 | ||
|
|
||
| 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 | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| package sorting | ||
|
|
||
| import ( | ||
| "slices" | ||
| ) | ||
|
|
||
| var Years = []int{2017, 2003, 2026} | ||
| var Pets = []string{"Dog", "Cat", "Parrot"} | ||
|
Comment on lines
+7
to
+8
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Refactor to avoid mutable global state. Exported package-level variables that are mutated by the sorting functions create several issues:
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 |
||
|
|
||
| func SortNumbers() []int { | ||
| slices.Sort(Years) | ||
| return Years | ||
| } | ||
|
|
||
| func SortAnimals() []string { | ||
| slices.Sort(Pets) | ||
| return Pets | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| package deferr | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "os" | ||
| "sync" | ||
| ) | ||
|
|
||
| var ( | ||
| fileInstance *os.File | ||
| once sync.Once | ||
| ) | ||
|
|
||
| func CreateFile() *os.File { | ||
| once.Do(func() { | ||
| f, err := os.CreateTemp("", "example.txt") | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| fileInstance = f | ||
| }) | ||
| return fileInstance | ||
| } | ||
|
|
||
| func WriteToFile(*os.File) { | ||
| f := CreateFile() | ||
| // TODO: Add your code here | ||
| fmt.Fprintln(f, "data") | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| package deferr | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "testing" | ||
| ) | ||
|
|
||
| func TestClosingFileAfterWriting(t *testing.T) { | ||
| f := CreateFile() | ||
| WriteToFile(f) | ||
|
|
||
| // trying to write to the file after closing | ||
| _, err := fmt.Fprintln(f, "data") | ||
| if err == nil { | ||
| t.Fatal("File wasn't closed!") | ||
| } | ||
| } | ||
|
Comment on lines
+8
to
+17
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion | 🟠 Major 🧩 Analysis chainTest relies on confusing API where WriteToFile ignores its parameter. The test passes a file handle to Additionally, since
Consider clarifying the API:
🏁 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
🤖 Prompt for AI Agents |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| package go_routines | ||
|
|
||
| import ( | ||
| "time" | ||
| ) | ||
|
|
||
| func sleepForOneHundredMillisecond() { | ||
| time.Sleep(100 * time.Millisecond) | ||
| } | ||
|
|
||
| func sleepForTwoHundredMilliseconds() { | ||
| time.Sleep(200 * time.Millisecond) | ||
| } | ||
|
|
||
| func RunConcurrently() { | ||
| // TODO: update following code | ||
| sleepForOneHundredMillisecond() | ||
| sleepForTwoHundredMilliseconds() | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,24 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||
| package go_routines | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| import ( | ||||||||||||||||||||||||||||||||||||||||||||||||
| "testing" | ||||||||||||||||||||||||||||||||||||||||||||||||
| "time" | ||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| 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!") | ||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+8
to
+18
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Test logic is critically flawed and will produce false positives. The test has multiple issues:
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 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| func BenchmarkRunConcurrently(b *testing.B) { | ||||||||||||||||||||||||||||||||||||||||||||||||
| for i := 0; i < b.N; i++ { | ||||||||||||||||||||||||||||||||||||||||||||||||
| RunConcurrently() | ||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| package channels | ||
|
|
||
| import "fmt" | ||
|
|
||
| func ReadMessage() string { | ||
| // TODO: update following code | ||
| // to receive senderMessage in main goroutine | ||
| // and return it from ReadMessage function | ||
| go func() { | ||
| senderMessage := "Hi!" | ||
| fmt.Println(senderMessage) | ||
| }() | ||
|
|
||
| return "" | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| package channels | ||
|
|
||
| import "testing" | ||
|
|
||
| func TestReadMessage(t *testing.T) { | ||
| got := ReadMessage() | ||
| want := "Hi!" | ||
|
|
||
| if got != want { | ||
| t.Errorf("Expected %s, got %s", want, got) | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,30 @@ | ||||||||||||||||||||||||||||||||||||||||||||||
| package mutexes | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| import "sync" | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| const numWorkers = 10_000 | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| func Counting() int { | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| // TODO: update following code to avoid race conditions | ||||||||||||||||||||||||||||||||||||||||||||||
| // using sync.Mutex | ||||||||||||||||||||||||||||||||||||||||||||||
| 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 | ||||||||||||||||||||||||||||||||||||||||||||||
| }() | ||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+15
to
+21
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Synchronize the shared counter to avoid races.
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
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| for i := 1; i <= numWorkers; i++ { | ||||||||||||||||||||||||||||||||||||||||||||||
| <-done | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| wg.Wait() | ||||||||||||||||||||||||||||||||||||||||||||||
| return count | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| package mutexes | ||
|
|
||
| import "testing" | ||
|
|
||
| func TestCounter(t *testing.T) { | ||
| got := Counting() | ||
| want := numWorkers | ||
|
|
||
| if got != want { | ||
| t.Fatalf("Expected %d, got %d", want, got) | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| package sorting | ||
|
|
||
| var Years = []int{2017, 2003, 2026} | ||
| var Pets = []string{"Dog", "Cat", "Parrot"} | ||
|
|
||
| func SortNumbers() []int { | ||
| // TODO: sort Years | ||
| return Years | ||
| } | ||
|
|
||
| func SortAnimals() []string { | ||
| // TODO: sort Pets | ||
| return Pets | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,22 @@ | ||||||||||||||||||||||||||
| package sorting | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| import ( | ||||||||||||||||||||||||||
| "slices" | ||||||||||||||||||||||||||
| "testing" | ||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| func TestSorting(t *testing.T) { | ||||||||||||||||||||||||||
| t.Run("Sorting numbers", func(t *testing.T) { | ||||||||||||||||||||||||||
| SortNumbers() | ||||||||||||||||||||||||||
| if !slices.IsSorted(Years) { | ||||||||||||||||||||||||||
| t.Fatal("Strings are not sorted!") | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||
|
Comment on lines
+9
to
+14
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| t.Run("Sorting strings", func(t *testing.T) { | ||||||||||||||||||||||||||
| SortAnimals() | ||||||||||||||||||||||||||
| if !slices.IsSorted(Pets) { | ||||||||||||||||||||||||||
| t.Fatal("Strings are not sorted!") | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove unused parameter or use it instead of calling CreateFile().
The function signature accepts
*os.Filebut ignores it, callingCreateFile()instead. This is confusing and creates an inconsistent API.Consider one of these approaches:
Remove the parameter if
CreateFile()should always be used:Use the parameter if the file should be provided by the caller:
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