Add solution for Challenge 1-4 by KanaDoodle #958
Conversation
WalkthroughThis PR adds Go implementations for four coding challenges: integer summation with a Changes
Possibly Related PRs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 3
🧹 Nitpick comments (4)
challenge-3/submissions/KanaDoodle/solution-template.go (1)
32-42: LGTM!The method correctly handles the empty slice case and computes the average salary accurately.
Optional style refinements.
Lines 35 and 37 could be simplified:
return float64(0)→return 0(implicit conversion)var sum float64 = 0→var sum float64orsum := 0.0challenge-4/submissions/KanaDoodle/solution-template.go (3)
1-10: LGTM!The function documentation clearly explains the parameters, return value, and concurrency requirement.
Optional: Simplify the import statement.
For a single import, the typical Go style is
import "sync"without parentheses.🔎 Suggested style improvement
-import ("sync") +import "sync"
21-39: LGTM!The BFS implementation is correct and properly maintains the visited set and traversal order.
Consider a more efficient queue implementation.
Using
queue = queue[1:]to dequeue creates a new slice on each iteration, which can be inefficient for large graphs. For better performance, consider using a circular buffer orcontainer/list.🔎 Example with indices for circular queue pattern
bfs := func(start int) []int { visited := make(map[int]bool) order := []int{} queue := []int{start} visited[start] = true head := 0 for head < len(queue) { node := queue[head] head++ order = append(order, node) for _, neighbor := range graph[node] { if !visited[neighbor] { visited[neighbor] = true queue = append(queue, neighbor) } } } return order }
66-76: LGTM!The synchronization pattern correctly ensures all workers complete before closing the results channel, and the result collection properly aggregates the BFS orders.
Note: Duplicate queries behavior.
If
queriescontains duplicate start nodes, each will be processed independently and the last result will overwrite earlier ones in the map. This may be acceptable, but if you want to deduplicate queries first, consider using a map to filter them before processing.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
challenge-1/submissions/KanaDoodle/solution-template.gochallenge-2/submissions/KanaDoodle/solution-template.gochallenge-3/submissions/KanaDoodle/solution-template.gochallenge-4/submissions/KanaDoodle/solution-template.go
🧰 Additional context used
🧬 Code graph analysis (1)
challenge-3/submissions/KanaDoodle/solution-template.go (1)
challenge-13/submissions/Gandook/solution-template.go (1)
ID(144-144)
🔇 Additional comments (13)
challenge-1/submissions/KanaDoodle/solution-template.go (3)
1-5: LGTM!Standard package declaration and imports for a simple CLI program.
7-19: LGTM!The main function correctly reads two integers from standard input, handles errors appropriately, and prints the sum. The implementation is straightforward and appropriate for a challenge solution.
21-23: LGTM!The Sum function is correctly implemented.
challenge-2/submissions/KanaDoodle/solution-template.go (2)
1-7: LGTM!Appropriate imports for reading from standard input and printing output.
23-32: LGTM!The ReverseString function correctly handles Unicode by converting to runes and uses an efficient two-pointer in-place reversal algorithm.
challenge-3/submissions/KanaDoodle/solution-template.go (3)
1-10: LGTM!The Employee struct is well-defined with appropriate field types. Using
intfor ID is acceptable for this challenge.
12-19: LGTM!The Manager struct and AddEmployee method are correctly implemented.
54-66: LGTM!The main function correctly demonstrates the Manager API with appropriate test data.
challenge-4/submissions/KanaDoodle/solution-template.go (5)
11-19: LGTM!The validation for
numWorkersis appropriate, and the Result struct is well-defined for internal use.
41-45: LGTM!Channels and WaitGroup are properly initialized for the worker pool pattern.
47-57: LGTM!The worker function and goroutine launch follow the correct worker pool pattern with proper WaitGroup handling.
59-64: LGTM!The job producer correctly feeds queries to the workers and closes the jobs channel to signal completion.
79-81: LGTM!An empty main is acceptable for a challenge submission where the focus is on the exported function.
| func main() { | ||
| // Read input from standard input | ||
| scanner := bufio.NewScanner(os.Stdin) | ||
| if scanner.Scan() { | ||
| input := scanner.Text() | ||
|
|
||
| // Call the ReverseString function | ||
| output := ReverseString(input) | ||
|
|
||
| // Print the result | ||
| fmt.Println(output) | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider checking for scanner errors.
While scanner.Scan() returns false on both EOF and errors, the code doesn't distinguish between them. For robustness, check scanner.Err() after Scan() completes.
🔎 Suggested improvement
func main() {
// Read input from standard input
scanner := bufio.NewScanner(os.Stdin)
if scanner.Scan() {
input := scanner.Text()
// Call the ReverseString function
output := ReverseString(input)
// Print the result
fmt.Println(output)
}
+ if err := scanner.Err(); err != nil {
+ fmt.Println("Error reading input:", err)
+ }
}📝 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 main() { | |
| // Read input from standard input | |
| scanner := bufio.NewScanner(os.Stdin) | |
| if scanner.Scan() { | |
| input := scanner.Text() | |
| // Call the ReverseString function | |
| output := ReverseString(input) | |
| // Print the result | |
| fmt.Println(output) | |
| } | |
| } | |
| func main() { | |
| // Read input from standard input | |
| scanner := bufio.NewScanner(os.Stdin) | |
| if scanner.Scan() { | |
| input := scanner.Text() | |
| // Call the ReverseString function | |
| output := ReverseString(input) | |
| // Print the result | |
| fmt.Println(output) | |
| } | |
| if err := scanner.Err(); err != nil { | |
| fmt.Println("Error reading input:", err) | |
| } | |
| } |
🤖 Prompt for AI Agents
In challenge-2/submissions/KanaDoodle/solution-template.go around lines 9 to 21,
the main function uses scanner.Scan() but does not check scanner.Err() so it
cannot distinguish EOF from a read error; after calling scanner.Scan() (or after
the scan loop if you refactor), call scanner.Err() and if non-nil print the
error to stderr (or log it) and exit with a non-zero status so read errors are
surfaced instead of silently ignored.
| // RemoveEmployee removes an employee by ID from the manager's list. | ||
| func (m *Manager) RemoveEmployee(id int) { | ||
| // TODO: Implement this method | ||
| for i := 0; i < len(m.Employees); i++ { | ||
| if m.Employees[i].ID == id { | ||
| m.Employees = append(m.Employees[:i], m.Employees[i+1:]...) | ||
| break | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Remove the TODO comment.
The method is fully implemented, so the TODO comment on line 23 should be removed.
The removal logic itself is correct and uses the standard Go slice manipulation pattern.
🔎 Suggested fix
// RemoveEmployee removes an employee by ID from the manager's list.
func (m *Manager) RemoveEmployee(id int) {
- // TODO: Implement this method
for i := 0; i < len(m.Employees); i++ {
if m.Employees[i].ID == id {
m.Employees = append(m.Employees[:i], m.Employees[i+1:]...)
break
}
}
}📝 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.
| // RemoveEmployee removes an employee by ID from the manager's list. | |
| func (m *Manager) RemoveEmployee(id int) { | |
| // TODO: Implement this method | |
| for i := 0; i < len(m.Employees); i++ { | |
| if m.Employees[i].ID == id { | |
| m.Employees = append(m.Employees[:i], m.Employees[i+1:]...) | |
| break | |
| } | |
| } | |
| } | |
| // RemoveEmployee removes an employee by ID from the manager's list. | |
| func (m *Manager) RemoveEmployee(id int) { | |
| for i := 0; i < len(m.Employees); i++ { | |
| if m.Employees[i].ID == id { | |
| m.Employees = append(m.Employees[:i], m.Employees[i+1:]...) | |
| break | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In challenge-3/submissions/KanaDoodle/solution-template.go around lines 21 to
30, remove the leftover TODO comment on line 23 because the RemoveEmployee
method is already implemented; simply delete the "// TODO: Implement this
method" line and keep the existing for-loop and slice manipulation intact.
| // FindEmployeeByID finds and returns an employee by their ID. | ||
| func (m *Manager) FindEmployeeByID(id int) *Employee { | ||
| for i := 0; i < len(m.Employees); i++ { | ||
| if m.Employees[i].ID == id { | ||
| return &m.Employees[i] | ||
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Returning a pointer to a slice element is unsafe.
Returning &m.Employees[i] creates a pointer into the slice's backing array. If the slice is subsequently modified (e.g., via AddEmployee causing reallocation or RemoveEmployee shifting elements), the pointer may become invalid or point to incorrect data.
🔎 Safer alternatives
Option 1: Return by value
-func (m *Manager) FindEmployeeByID(id int) *Employee {
+func (m *Manager) FindEmployeeByID(id int) (Employee, bool) {
for i := 0; i < len(m.Employees); i++ {
if m.Employees[i].ID == id {
- return &m.Employees[i]
+ return m.Employees[i], true
}
}
- return nil
+ return Employee{}, false
}Option 2: Return a copy via pointer
func (m *Manager) FindEmployeeByID(id int) *Employee {
for i := 0; i < len(m.Employees); i++ {
if m.Employees[i].ID == id {
- return &m.Employees[i]
+ emp := m.Employees[i]
+ return &emp
}
}
return nil
}📝 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.
| // FindEmployeeByID finds and returns an employee by their ID. | |
| func (m *Manager) FindEmployeeByID(id int) *Employee { | |
| for i := 0; i < len(m.Employees); i++ { | |
| if m.Employees[i].ID == id { | |
| return &m.Employees[i] | |
| } | |
| } | |
| return nil | |
| } | |
| // FindEmployeeByID finds and returns an employee by their ID. | |
| func (m *Manager) FindEmployeeByID(id int) *Employee { | |
| for i := 0; i < len(m.Employees); i++ { | |
| if m.Employees[i].ID == id { | |
| emp := m.Employees[i] | |
| return &emp | |
| } | |
| } | |
| return nil | |
| } |
🤖 Prompt for AI Agents
In challenge-3/submissions/KanaDoodle/solution-template.go around lines 44 to
52, the method currently returns &m.Employees[i], which is unsafe because it
returns a pointer into the slice's backing array that can be invalidated by
future slice modifications; change the API to return an Employee by value (func
(m *Manager) FindEmployeeByID(id int) (Employee, bool)) or, if you must keep a
pointer return, create a local copy of the element (e.g., emp := m.Employees[i];
return &emp) so the pointer refers to a heap-allocated copy; update all callers
to handle the new return type (use the value or check the bool for found)
accordingly.
1 participant
:p