Add solution for Challenge 3 by nzamulov #693
Conversation
WalkthroughA new Go program is added implementing Employee and Manager types with methods to manage employee collections: AddEmployee, RemoveEmployee, GetAverageSalary, and FindEmployeeByID. The main function demonstrates creating a manager, removing an employee, calculating average salary, and locating an employee by ID. Changes
Sequence DiagramsequenceDiagram
participant Main
participant Manager
participant EmployeeList
Main->>Manager: Create Manager
Main->>Manager: AddEmployee(Employee{ID:1, Name:"Alice", Salary:50000})
Manager->>EmployeeList: Append
Main->>Manager: AddEmployee(Employee{ID:2, Name:"Bob", Salary:60000})
Manager->>EmployeeList: Append
Main->>Manager: RemoveEmployee(1)
Manager->>EmployeeList: Filter out ID 1
Main->>Manager: GetAverageSalary()
Manager->>EmployeeList: Calculate mean
Manager-->>Main: Return average
Main->>Manager: FindEmployeeByID(2)
Manager->>EmployeeList: Search by ID
Manager-->>Main: Return employee pointer
Main->>Main: Print results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Areas requiring extra attention:
Possibly related PRs
Pre-merge checks❌ Failed checks (1 warning)
✅ 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: 1
🧹 Nitpick comments (2)
challenge-3/submissions/nzamulov/solution-template.go (2)
21-29: Consider returning a success indicator.The removal logic is correct, but the method doesn't indicate whether an employee was found and removed. Consider returning a
boolto signal success, which can help callers handle cases where the ID doesn't exist.Apply this diff to add a return value:
-func (m *Manager) RemoveEmployee(id int) { +func (m *Manager) RemoveEmployee(id int) bool { for i, e := range m.Employees { if e.ID == id { m.Employees = append(m.Employees[:i], m.Employees[i+1:]...) - break + return true } } + return false }
31-41: Consider simplifying variable initialization.The average salary calculation is correct and includes good defensive programming for the empty case. Line 36 could be simplified since the zero value of
float64is already0.Apply this diff:
- var total float64 = 0 + var total float64
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
challenge-3/submissions/nzamulov/solution-template.go(1 hunks)
🔇 Additional comments (4)
challenge-3/submissions/nzamulov/solution-template.go (4)
5-10: LGTM!The Employee struct is well-defined with appropriate field types and follows Go naming conventions.
12-14: LGTM!The Manager struct appropriately encapsulates a collection of employees.
16-19: LGTM!The AddEmployee method correctly uses a pointer receiver and idiomatic append to add employees to the manager's list.
53-65: LGTM!The main function effectively demonstrates all the Manager methods and includes proper nil checking before dereferencing the employee pointer.
| // FindEmployeeByID finds and returns an employee by their ID. | ||
| func (m *Manager) FindEmployeeByID(id int) *Employee { | ||
| for _, e := range m.Employees { | ||
| if e.ID == id { | ||
| return &e | ||
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Critical bug: returning address of loop variable.
Line 47 returns &e where e is the range loop variable. In Go, the loop variable is reused across iterations, so this returns the address of a temporary copy rather than the actual slice element. This pointer may become invalid or point to incorrect data after the loop.
Apply this diff to fix the issue by returning the address of the slice element:
func (m *Manager) FindEmployeeByID(id int) *Employee {
- for _, e := range m.Employees {
+ for i, e := range m.Employees {
if e.ID == id {
- return &e
+ return &m.Employees[i]
}
}
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 _, e := range m.Employees { | |
| if e.ID == id { | |
| return &e | |
| } | |
| } | |
| return nil | |
| } | |
| // FindEmployeeByID finds and returns an employee by their ID. | |
| func (m *Manager) FindEmployeeByID(id int) *Employee { | |
| for i, e := range m.Employees { | |
| if e.ID == id { | |
| return &m.Employees[i] | |
| } | |
| } | |
| return nil | |
| } |
🤖 Prompt for AI Agents
In challenge-3/submissions/nzamulov/solution-template.go around lines 43 to 51,
the function FindEmployeeByID returns &e (address of the range loop variable)
which is a reused temporary; replace that with a pointer to the actual slice
element (for example iterate with index and return &m.Employees[i] or capture
the element by index) so the returned pointer references the real slice element
rather than the loop variable.
1 participant
Challenge 3 Solution
Submitted by: @nzamulov
Challenge: Challenge 3
Description
This PR contains my solution for Challenge 3.
Changes
challenge-3/submissions/nzamulov/solution-template.goTesting
Thank you for reviewing my submission! 🚀