patternjavaMinor
Coin change algorithm
Viewed 0 times
coinalgorithmchange
Problem
I've implemented the coin change algorithm using Dynamic Programming and Greedy Algorithm w/ backtracking. The description is as follows:
Given an amount of change (n) list all of the possibilities of coins that can be used to satisfy the amount of change
It would be nice to have a code review to show me where I can improve on readability (along with other things you may find!). I've been trying to clean it up and refactor a bit, but a fresh set of eyes would be nice. I went a bit crazy on comments because I wanted to make sure I would be able to look at this in a few months and not forget why I did something the way I did. But perhaps putting some of that info in a github wiki would have been better.
```
import java.util.List;
import java.util.ArrayList;
import java.util.Arrays;
/**
* Given an amount of change (n) list all of the possibilities of coins that can
* be used to satisfy the amount of change.
*
* Example: n = 12
*
* 1,1,1,1,1,1,1,1,1,1,1,1
* 1,1,1,1,1,1,1,5
* 1,1,5,5
* 1,1,10
*
* @author Eric
*
*/
public class CoinChange {
private int[] coins = { 1, 5, 10, 25 };
/**
* Uses a greedy algorithm with backtracking to find the
* possible combinations
*
* @param sum - the sum of the current combination
* @param n - the target
* @param pos - position for which coin to start at
* @param combination - the current combination
*/
public void findPossibleCombinations(int sum, int n, int pos, List combination) {
// Begin at pos. We use pos so that we don't have duplicate combinations
// in different orders ex: 1,1,10 is the same as 1,10,1 or 10,1,1
// This is possible because when we are at a larger coin, we know that
// combinations with smaller coins and the larger/current coin
// have already been found, so we no longer need to consider them.
// If we did consider them, we would have duplicates.
// Therefore, pos allows you to only lo
Given an amount of change (n) list all of the possibilities of coins that can be used to satisfy the amount of change
It would be nice to have a code review to show me where I can improve on readability (along with other things you may find!). I've been trying to clean it up and refactor a bit, but a fresh set of eyes would be nice. I went a bit crazy on comments because I wanted to make sure I would be able to look at this in a few months and not forget why I did something the way I did. But perhaps putting some of that info in a github wiki would have been better.
```
import java.util.List;
import java.util.ArrayList;
import java.util.Arrays;
/**
* Given an amount of change (n) list all of the possibilities of coins that can
* be used to satisfy the amount of change.
*
* Example: n = 12
*
* 1,1,1,1,1,1,1,1,1,1,1,1
* 1,1,1,1,1,1,1,5
* 1,1,5,5
* 1,1,10
*
* @author Eric
*
*/
public class CoinChange {
private int[] coins = { 1, 5, 10, 25 };
/**
* Uses a greedy algorithm with backtracking to find the
* possible combinations
*
* @param sum - the sum of the current combination
* @param n - the target
* @param pos - position for which coin to start at
* @param combination - the current combination
*/
public void findPossibleCombinations(int sum, int n, int pos, List combination) {
// Begin at pos. We use pos so that we don't have duplicate combinations
// in different orders ex: 1,1,10 is the same as 1,10,1 or 10,1,1
// This is possible because when we are at a larger coin, we know that
// combinations with smaller coins and the larger/current coin
// have already been found, so we no longer need to consider them.
// If we did consider them, we would have duplicates.
// Therefore, pos allows you to only lo
Solution
After 6 months still not having an answer on codereview is probably a good sign.
Meaning nobody sees any major problems with your code and can't be bothered with the small things.
Perhaps getting this answer could be a good time to review your own code and see what you would do differently now yourself.
The only thing that I can say is "wrong" with this code is your choice of variable names and then the reason for adding lots of comments.
Most of your comments are actually well placed. More specifically those explaining WHY you do something. Especially the importance of your ordering of coins from small to large.
Some of the comments like this one:
are kinda useless if you would choose better variable names (although combination isn't really that bad either).
So my only real change would be from
to
Or even better names if you can find them. Which hopefully makes some of your comments redundant.
Meaning nobody sees any major problems with your code and can't be bothered with the small things.
Perhaps getting this answer could be a good time to review your own code and see what you would do differently now yourself.
The only thing that I can say is "wrong" with this code is your choice of variable names and then the reason for adding lots of comments.
Most of your comments are actually well placed. More specifically those explaining WHY you do something. Especially the importance of your ordering of coins from small to large.
Some of the comments like this one:
combination.add(coin); // Add the coin to the current combinationare kinda useless if you would choose better variable names (although combination isn't really that bad either).
So my only real change would be from
public void findPossibleCombinations(
int sum, int n, int pos, List combination) {to
public void findPossibleCombinations(
int currentTotal,
int targetAmmount,
int currentCoinIndex,
List currentChange) {Or even better names if you can find them. Which hopefully makes some of your comments redundant.
Code Snippets
combination.add(coin); // Add the coin to the current combinationpublic void findPossibleCombinations(
int sum, int n, int pos, List<Integer> combination) {public void findPossibleCombinations(
int currentTotal,
int targetAmmount,
int currentCoinIndex,
List<Integer> currentChange) {Context
StackExchange Code Review Q#141853, answer score: 3
Revisions (0)
No revisions yet.