-
-
Notifications
You must be signed in to change notification settings - Fork 709
Add solution for Challenge 3 by kiramux #663
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
WalkthroughA new Go file introduces an Employee management system with Employee and Manager types. The Manager provides methods to add, remove, query employees by ID, and compute average salary, demonstrated in a main function. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 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: 2
🧹 Nitpick comments (1)
challenge-3/submissions/kiramux/solution-template.go (1)
40-53: Consider using range loop for more idiomatic Go.The implementation is correct. However, using a range loop would be more idiomatic.
Apply this diff:
- var sum float64 - for i := 0; i < size; i++ { - sum += m.Employees[i].Salary - } - avg := sum / float64(size) - return avg + var sum float64 + for _, emp := range m.Employees { + sum += emp.Salary + } + return sum / float64(size)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
challenge-3/submissions/kiramux/solution-template.go(1 hunks)
🔇 Additional comments (1)
challenge-3/submissions/kiramux/solution-template.go (1)
17-20: LGTM!The implementation correctly appends the new employee to the manager's list.
| func (m *Manager) RemoveEmployee(id int) { | ||
| // TODO: Implement this method | ||
| size := len(m.Employees) | ||
| flag := -1 | ||
| for i := range m.Employees { | ||
| if m.Employees[i].ID == id { | ||
| flag = i | ||
| break | ||
| } | ||
| } | ||
| if flag != -1 { | ||
| m.Employees[flag] = m.Employees[size-1] | ||
| m.Employees = m.Employees[:size-1] | ||
| } | ||
| } |
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.
Fix indentation to use tabs.
The code uses spaces for indentation instead of tabs, which violates Go conventions. Run gofmt or go fmt to automatically fix the indentation throughout the file.
Apply gofmt to fix:
#!/bin/bash
# Run gofmt to fix indentation
go fmt challenge-3/submissions/kiramux/solution-template.go🤖 Prompt for AI Agents
challenge-3/submissions/kiramux/solution-template.go lines 23-37: the function
uses spaces for indentation instead of tabs; run gofmt (or go fmt) on this file
to replace spaces with Go-standard tabs and normalize formatting (e.g., run
`gofmt -w challenge-3/submissions/kiramux/solution-template.go` or `go fmt
./challenge-3/submissions/kiramux`) so the indentation and overall formatting
conform to Go conventions.
| func (m *Manager) FindEmployeeByID(id int) *Employee { | ||
| // TODO: Implement this method | ||
| for i := range m.Employees { | ||
| if m.Employees[i].ID == id { | ||
| return &(m.Employees[i]) | ||
| } | ||
| } | ||
| fmt.Println("Cannot find employee by ID: ", id) | ||
| 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: Returning pointer to slice element is unsafe.
Returning &m.Employees[i] creates a dangling pointer risk. When RemoveEmployee modifies the slice (swap-and-truncate), any existing pointers become stale and may point to wrong data or invalid memory.
Additionally, printing the error message directly (line 63) violates separation of concerns. The caller should decide how to handle a not-found case.
Apply this diff to return a copy instead and remove direct printing:
-func (m *Manager) FindEmployeeByID(id int) *Employee {
+func (m *Manager) FindEmployeeByID(id int) (Employee, bool) {
// TODO: Implement this method
for i := range m.Employees {
if m.Employees[i].ID == id {
- return &(m.Employees[i])
+ return m.Employees[i], true
}
}
- fmt.Println("Cannot find employee by ID: ", id)
- 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)
+ } else {
+ fmt.Printf("Employee not found\n")
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In challenge-3/submissions/kiramux/solution-template.go around lines 56–65, do
not return a pointer to a slice element or print from inside the method; change
the signature to return a copy and a found flag (e.g., FindEmployeeByID(id int)
(Employee, bool)), iterate to find the employee and return the Employee value
with true, and return the zero-value Employee with false when not found (remove
the fmt.Println). Then update the caller(s) in main to handle the (Employee,
bool) result: check the bool, log or handle the not-found case there, and use
the returned copy when found.
Challenge 3 Solution
Submitted by: @kiramux
Challenge: Challenge 3
Description
This PR contains my solution for Challenge 3.
Changes
challenge-3/submissions/kiramux/solution-template.goTesting
Thank you for reviewing my submission! 🚀