-
-
Notifications
You must be signed in to change notification settings - Fork 709
Add solution for Challenge 3 by AlexO-85 #684
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 introduces a Go solution for Challenge 3 that implements Employee and Manager types with methods for managing employee records. The Employee struct stores employee data, while Manager maintains a slice of employees and provides operations to add, remove, query, and compute average salary. A main function demonstrates the complete workflow. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes
Possibly related PRs
Pre-merge checks✅ 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 |
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 (2)
challenge-3/submissions/AlexO-85/solution-template.go (2)
21-29: Remove unnecessary parentheses and semicolon.The logic is correct, but the code includes Go style deviations:
- Line 24: Parentheses around the
ifcondition are unnecessary and not idiomatic in Go.- Line 26: The semicolon after
breakis unnecessary and not idiomatic in Go.Apply this diff for idiomatic Go style:
- if(e.ID == id){ + if e.ID == id { m.Employees = append(m.Employees[:i], m.Employees[i+1:]...) - break; + break }
31-48: Use pre-calculated length and remove unnecessary parentheses.The logic is correct, but there are style improvements:
- Line 37: Unnecessary parentheses around the
ifcondition.- Line 45: Recalculates
len(m.Employees)instead of using thecountEmployeesvariable from Line 34.Apply this diff for consistency:
- if(countEmployees == 0){ + if countEmployees == 0 { return sumSalary } for _, e := range m.Employees { sumSalary += e.Salary } - avgSalary := sumSalary / float64(len(m.Employees)) + avgSalary := sumSalary / float64(countEmployees) return avgSalary
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
challenge-3/submissions/AlexO-85/solution-template.go(1 hunks)
🔇 Additional comments (4)
challenge-3/submissions/AlexO-85/solution-template.go (4)
5-10: LGTM!The
Employeestruct is well-defined with appropriate field types and naming.
12-14: LGTM!The
Managerstruct is appropriately defined.
16-19: LGTM!The
AddEmployeemethod correctly uses a pointer receiver and appends to the slice idiomatically.
60-72: LGTM!The
mainfunction provides a good demonstration of the Manager API with appropriate test cases and output formatting.
| // 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Critical: Fix loop variable address capture bug.
Line 54 contains a critical bug: returning &e captures the address of the loop variable, not the actual employee in the slice. In Go, the loop variable e is reused for each iteration, so its address remains the same and points to a temporary copy. This will lead to incorrect or unpredictable behavior.
Additionally, Line 53 has unnecessary parentheses around the if condition.
Apply this diff to fix both issues:
func (m *Manager) FindEmployeeByID(id int) *Employee {
- for _, e := range m.Employees {
- if(e.ID == id){
- return &e
+ for i := range m.Employees {
+ if m.Employees[i].ID == id {
+ return &m.Employees[i]
}
}
return nil🤖 Prompt for AI Agents
In challenge-3/submissions/AlexO-85/solution-template.go around lines 50 to 58,
the function returns &e which captures the loop variable's address and also
contains unnecessary parentheses in the if; to fix, iterate with index (for i :=
range m.Employees) or use for i, e := range m.Employees and return
&m.Employees[i] instead of &e, and remove the extraneous parentheses around the
if condition so it reads if e.ID == id { ... }.
Challenge 3 Solution
Submitted by: @AlexO-85
Challenge: Challenge 3
Description
This PR contains my solution for Challenge 3.
Changes
challenge-3/submissions/AlexO-85/solution-template.goTesting
Thank you for reviewing my submission! 🚀