-
-
Notifications
You must be signed in to change notification settings - Fork 709
Add solution for Challenge 22 by Johrespi #654
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
WalkthroughAdds two exported Go functions implementing a greedy coin-change algorithm: 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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
challenge-22/submissions/Johrespi/solution-template.go(1 hunks)
🔇 Additional comments (2)
challenge-22/submissions/Johrespi/solution-template.go (2)
7-27: LGTM!The main function provides clear demonstration of both functions with reasonable test cases.
31-52: Remove the algorithmic critique and focus only on valid code quality issues.This challenge explicitly requires a greedy approach for U.S. coin denominations [1, 5, 10, 25, 50], where it produces optimal results. The challenge documentation already acknowledges that greedy doesn't work for arbitrary denominations but explicitly states: "Your solution should implement the greedy approach, which always chooses the largest coin possible."
The following issues need addressing:
- Remove TODO comment on line 32
- Fix formatting on line 34: change
sum :=0tosum := 0The algorithm implementation itself aligns with the challenge requirements. Dynamic programming is not needed for this specific challenge.
Likely an incorrect or invalid review comment.
| func CoinCombination(amount int, denominations []int) map[int]int { | ||
| // TODO: Implement this function | ||
| c := make(map[int]int) | ||
| sum := 0 | ||
|
|
||
| for i:= len(denominations) - 1; i >= 0; i--{ | ||
| for{ | ||
| if sum + denominations[i] <= amount{ | ||
| sum += denominations[i] | ||
| c[denominations[i]]++ | ||
| } else{ | ||
| break | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if sum != amount { | ||
| return map[int]int{} | ||
| } | ||
|
|
||
| return c | ||
| } |
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.
🛠️ Refactor suggestion | 🟠 Major
Significant code duplication with MinCoins.
This function duplicates the core greedy logic from MinCoins (lines 36-45 vs lines 63-71). The same greedy algorithm issues and input validation concerns apply here.
Consider refactoring to eliminate duplication:
func CoinCombination(amount int, denominations []int) map[int]int {
combination := make(map[int]int)
remaining := amount
// Iterate from largest to smallest denomination
for i := len(denominations) - 1; i >= 0; i-- {
coin := denominations[i]
if coin <= remaining {
count := remaining / coin
if count > 0 {
combination[coin] = count
remaining -= count * coin
}
}
}
if remaining != 0 {
return map[int]int{}
}
return combination
}This refactored version:
- Uses division instead of nested loops for better efficiency
- Eliminates the duplicate loop logic
- More clearly expresses intent
- Removes the TODO comment (line 59)
Note: The greedy algorithm limitation mentioned in the MinCoins review also applies here.
🤖 Prompt for AI Agents
In challenge-22/submissions/Johrespi/solution-template.go around lines 58 to 79,
CoinCombination duplicates the greedy loop logic from MinCoins and uses
inefficient nested loops; replace the nested loop with a single-pass
division-based greedy approach: iterate denominations from largest to smallest,
compute count := remaining / coin, set combination[coin] = count if >0, subtract
count*coin from remaining, and after the loop return an empty map if remaining
!= 0; remove the TODO comment and consider extracting shared greedy logic into a
helper to eliminate duplication with MinCoins.
Challenge 22 Solution
Submitted by: @Johrespi
Challenge: Challenge 22
Description
This PR contains my solution for Challenge 22.
Changes
challenge-22/submissions/Johrespi/solution-template.goTesting
Thank you for reviewing my submission! 🚀