-
-
Notifications
You must be signed in to change notification settings - Fork 713
Add solution for Challenge 3 by yz4230 #743
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
Conversation
WalkthroughThis PR adds a template solution for challenge-3 implementing Go structures for employee management. The solution defines an Employee struct, a Manager struct with employee collection, and public methods to add/remove employees, calculate average salary, and retrieve employees by ID. A main function demonstrates basic usage. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
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 |
dba8426 to
332a988
Compare
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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
challenge-3/submissions/yz4230/solution-template.go (1)
22-37: Consider simplifying the removal logic.The current implementation works correctly but is verbose. Go has a more idiomatic pattern for removing elements from slices.
Apply this diff for a more concise and idiomatic implementation:
func (m *Manager) RemoveEmployee(id int) { - idx := -1 + idx := -1 for i, e := range m.Employees { - if e.ID == id { - idx = i - break - } + if e.ID == id { + idx = i + break + } } if idx == -1 { - return + return } - next := make([]Employee, 0, len(m.Employees)-1) - next = append(next, m.Employees[:idx]...) - next = append(next, m.Employees[idx+1:]...) - m.Employees = next + m.Employees = append(m.Employees[:idx], m.Employees[idx+1:]...) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
challenge-3/submissions/yz4230/solution-template.go(1 hunks)
🔇 Additional comments (4)
challenge-3/submissions/yz4230/solution-template.go (4)
5-10: LGTM! Well-structured Employee type.The Employee struct is clean and contains appropriate fields for the challenge requirements.
12-14: LGTM! Clean Manager structure.The Manager struct appropriately encapsulates the employee collection.
16-19: LGTM! Straightforward implementation.The AddEmployee method correctly appends to the slice.
39-49: LGTM! Correct average calculation with proper edge case handling.The method correctly handles the empty employee list and computes the average accurately.
| // FindEmployeeByID finds and returns an employee by their ID. | ||
| func (m *Manager) FindEmployeeByID(id int) *Employee { | ||
| for i := range m.Employees { | ||
| if m.Employees[i].ID == id { | ||
| return &m.Employees[i] | ||
| } | ||
| } | ||
| return nil | ||
| } |
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.
Returning pointer to slice element creates potential dangling pointer issue.
Returning a pointer to a slice element (&m.Employees[i]) is risky because:
- If the slice is later modified (e.g., via
RemoveEmployeeor ifAddEmployeetriggers reallocation), the returned pointer may become invalid or point to incorrect data. - This is a common Go pitfall that can lead to subtle, hard-to-debug issues.
Consider returning by value or making a copy instead.
Apply this diff to return by value with a found indicator:
-// FindEmployeeByID finds and returns an employee by their ID.
-func (m *Manager) FindEmployeeByID(id int) *Employee {
+// FindEmployeeByID finds and returns an employee by their ID.
+// Returns the employee and true if found, zero value and false otherwise.
+func (m *Manager) FindEmployeeByID(id int) (Employee, bool) {
for i := range m.Employees {
- if m.Employees[i].ID == id {
- return &m.Employees[i]
- }
+ if m.Employees[i].ID == id {
+ return m.Employees[i], true
+ }
}
- return nil
+ return Employee{}, false
}Then update the caller in main:
- employee := manager.FindEmployeeByID(2)
+ employee, found := manager.FindEmployeeByID(2)
fmt.Printf("Average Salary: %f\n", averageSalary)
- if employee != nil {
- fmt.Printf("Employee found: %+v\n", *employee)
+ if found {
+ fmt.Printf("Employee found: %+v\n", employee)
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In challenge-3/submissions/yz4230/solution-template.go around lines 51 to 59,
the function currently returns a pointer to a slice element which can become
invalid if the slice is modified; change the function to return the Employee by
value plus a boolean found flag (e.g., (Employee, bool)), iterate to find the
match and return a copy of the employee and true, and return the zero-value
Employee and false when not found; then update all call sites (e.g., in main) to
handle the two-value return, check the boolean before using the Employee, and
avoid storing or relying on pointers to slice elements.
Challenge 3 Solution
Submitted by: @yz4230
Challenge: Challenge 3
Description
This PR contains my solution for Challenge 3.
Changes
challenge-3/submissions/yz4230/solution-template.goTesting
Thank you for reviewing my submission! 🚀