-
-
Notifications
You must be signed in to change notification settings - Fork 711
Add solution for Challenge 22 by AlexO-85 #703
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
WalkthroughIntroduces a new Go solution implementing a greedy coin-change algorithm under Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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 |
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 (2)
challenge-22/submissions/AlexO-85/solution-template.go (2)
79-84: Simplify map initialization.Go initializes missing
intvalues in maps to0, so the existence check is unnecessary.- _, ok := combination[val] - if !ok { - combination[val] = 0 - } - combination[val]++ + combination[val]++
39-48: Consider using division instead of repeated subtraction for efficiency.The inner loop repeatedly subtracts
valfromamount. Using integer division and modulo is more efficient and cleaner.- for amount > 0 { - - if val > amount { - break - } - - count++ - amount -= val - } + if val <= amount { + numCoins := amount / val + count += numCoins + amount = amount % val + }Apply a similar change to
CoinCombination(lines 73-85):- for amount > 0 { - - if val > amount { - break - } - - combination[val]++ - amount -= val - } + if val <= amount { + combination[val] = amount / val + amount = amount % val + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
challenge-22/submissions/AlexO-85/solution-template.go(1 hunks)
🔇 Additional comments (2)
challenge-22/submissions/AlexO-85/solution-template.go (2)
8-28: LGTM! Clear demonstration code.The main function provides a good demonstration of both functions with standard US coin denominations.
32-60: Incorrect review comment. The challenge explicitly requires a greedy implementation and acknowledges its limitations in the README. The problem statement specifies "Your solution should implement the greedy approach, which always chooses the largest coin possible" and notes "The greedy approach for coin change works optimally for the standard U.S. coin denominations". The solution correctly implements the greedy algorithm as required. All test cases are designed for the greedy approach, including non-canonical coin sets where greedy is expected to be used, not an optimal solution.Likely an incorrect or invalid review comment.
| slices.Sort(denominations) | ||
| slices.Reverse(denominations) |
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 side effect: input slice is modified.
slices.Sort and slices.Reverse modify the input slice in place, which means the caller's denominations slice will be reordered. This is an unexpected side effect.
Copy the slice before sorting to avoid modifying the caller's data:
+ // Make a copy to avoid modifying the input slice
+ denoms := make([]int, len(denominations))
+ copy(denoms, denominations)
+
- slices.Sort(denominations)
- slices.Reverse(denominations)
+ slices.Sort(denoms)
+ slices.Reverse(denoms)
- for _, val := range denominations {
+ for _, val := range denoms {Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In challenge-22/submissions/AlexO-85/solution-template.go around lines 34 to 35,
slices.Sort and slices.Reverse are mutating the input denominations slice
causing an unexpected side effect; to fix this, make a copy of the denominations
slice (e.g. allocate a new slice and copy values) and run Sort/Reverse on that
copy so the original caller slice remains unchanged, then use the sorted copy
for further processing.
| slices.Sort(denominations) | ||
| slices.Reverse(denominations) |
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 side effect: input slice is modified.
Same issue as in MinCoins—the input slice is modified in place.
Apply the same fix to copy the slice before sorting:
+ // Make a copy to avoid modifying the input slice
+ denoms := make([]int, len(denominations))
+ copy(denoms, denominations)
+
- slices.Sort(denominations)
- slices.Reverse(denominations)
+ slices.Sort(denoms)
+ slices.Reverse(denoms)
- for _, val := range denominations {
+ for _, val := range denoms {Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In challenge-22/submissions/AlexO-85/solution-template.go around lines 69 to 70,
the code sorts and reverses the denominations slice in place which mutates the
input; make a copy of the denominations slice first (e.g., use slices.Clone or
allocate a new slice and copy) and perform Sort and Reverse on that copy so the
original input is not modified.
Challenge 22 Solution
Submitted by: @AlexO-85
Challenge: Challenge 22
Description
This PR contains my solution for Challenge 22.
Changes
challenge-22/submissions/AlexO-85/solution-template.goTesting
Thank you for reviewing my submission! 🚀