Add solution for Challenge 3 by alimkinpark#1637
Conversation
WalkthroughAdded a complete Go implementation of an Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.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 (1)
challenge-3/submissions/alimkinpark/solution-template.go (1)
55-58: Prefer&m.Employees[i]over&eto return a pointer to the actual slice elementWhile the returned pointer is only read in tests, returning
&m.Employees[i]preserves correct pointer semantics and points to the actual slice storage rather than a range-loop copy. This ensures the caller gets a direct reference to the slice element, maintaining the expected contract.Suggested fix
- 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] + } }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: afe4e96b-edca-4e43-bfcb-f557f63170b7
📒 Files selected for processing (1)
challenge-3/submissions/alimkinpark/solution-template.go
| for i, e := range m.Employees { | ||
| if e.ID == id { | ||
| m.Employees = append(m.Employees[:i], | ||
| m.Employees[i+1:]...) | ||
| } | ||
| return | ||
| } |
There was a problem hiding this comment.
RemoveEmployee returns too early and skips most employees
Line 30 returns on the first loop iteration even when IDs don’t match, so only index 0 is effectively checked.
Suggested fix
func (m *Manager) RemoveEmployee(id int) {
// TODO: Implement this method
- for i, e := range m.Employees {
- if e.ID == id {
- m.Employees = append(m.Employees[:i],
- m.Employees[i+1:]...)
- }
- return
+ for i, e := range m.Employees {
+ if e.ID == id {
+ m.Employees = append(m.Employees[:i], m.Employees[i+1:]...)
+ return
+ }
}
}📝 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.
| for i, e := range m.Employees { | |
| if e.ID == id { | |
| m.Employees = append(m.Employees[:i], | |
| m.Employees[i+1:]...) | |
| } | |
| return | |
| } | |
| func (m *Manager) RemoveEmployee(id int) { | |
| // TODO: Implement this method | |
| for i, e := range m.Employees { | |
| if e.ID == id { | |
| m.Employees = append(m.Employees[:i], m.Employees[i+1:]...) | |
| return | |
| } | |
| } | |
| } |
|
Could not automatically merge this PR: Resource not accessible by integration A maintainer will need to merge manually. |
2 similar comments
|
Could not automatically merge this PR: Resource not accessible by integration A maintainer will need to merge manually. |
|
Could not automatically merge this PR: Resource not accessible by integration A maintainer will need to merge manually. |
|
👋 Action needed for auto-merge This PR cannot be auto-merged because "Allow edits from maintainers" is enabled. This is a GitHub limitation with automated workflows. To fix: Please uncheck "Allow edits from maintainers" in the PR sidebar (under the reviewers section), and the PR will be auto-merged in the next cycle. Alternatively, a maintainer can merge this manually. |
1 participant
Challenge 3 Solution
Submitted by: @alimkinpark
Challenge: Challenge 3
Description
This PR contains my solution for Challenge 3.
Changes
challenge-3/submissions/alimkinpark/solution-template.goTesting
Thank you for reviewing my submission! 🚀