are the 'if' statements too restrictive?
I am not sure if the statements correspond the the given specification.
You have to clarify the corner cases. I would say, 1000.01 belongs to 2%.
Rest is probably mostly fine, with some small problems:
public BigDecimal calc(BigDecimal amt) {
Do not use abbreviations. Could be amount or american totals, or whatever?
amt.compareTo(new BigDecimal(1001))==-1
Try to use the compareTo in this way: x.compareTo(y) <relation> 0
Where <relation> is one of {<, <=, ==} (you could use {>=, >}, too. But you can go without them if you rearrange the variables from the compareTo call)
Do not rely on -1 or +1. This is not necessarily the general contract.
amt.multiply(0.02)
There is no double argument method here. And even if, you should avoid float/double in any way if you handle concurrency because of rounding and representating errors.
If you have only this small cases, I would use static constants (could be more difficult if you have more cases)
All together, it could look like this:
private static BigDecimal ONE_PERCENT = new BigDecimal("0.01");
private static BigDecimal TWO_PERCENT = new BigDecimal("0.02");
private static BigDecimal THREE_PERCENT = new BigDecimal("0.03");
private static BigDecimal ONE_THOUSAND = new BigDecimal("1000");
private static BigDecimal FIVE_THOUSAND = new BigDecimal("5000");
public BigDecimal calc(final BigDecimal amount) {
//]-infinity, 0[
if (amount.compareTo(BigDecimal.ZERO) < 0)
return amount;
// [0, 1000]
else if (amount.compareTo(ONE_THOUSAND) <= 0)
return amount.multiply(ONE_PERCENT);
// ]1000, 5000]
else if (amount.compareTo(FIVE_THOUSAND) <= 0)
return amount.multiply(TWO_PERCENT);
// ]5000, infinity[
else
return amount.multiply(THREE_PERCENT);
}
If your specification says to 1000 means 1000 is not included, you can easily modify the <= to <. I have added the corresponding comments.
Final word: Write some unit tests to test it. Especially the corner cases.
In response to Interest calculator :
private static BigDecimal ONE_PERCENT = new BigDecimal("0.01");
private static BigDecimal TWO_PERCENT = new BigDecimal("0.02");
private static BigDecimal TWO_POINT_FIVE_PERCENT = new BigDecimal("0.025");
private static BigDecimal THREE_PERCENT = new BigDecimal("0.03");
private static BigDecimal FOUR_PERCENT = new BigDecimal("0.04");
private static BigDecimal FIVE_PERCENT = new BigDecimal("0.05");
private static BigDecimal ONE_THOUSAND = new BigDecimal("1000");
private static BigDecimal FIVE_THOUSAND = new BigDecimal("5000");
private static BigDecimal TEN_THOUSAND = new BigDecimal("10000");
public BigDecimal calc(final BigDecimal amount, final int years) {
if (years < 0)
throw new IllegalArgumentException("years can not be negative: " + years);
if (amount.compareTo(BigDecimal.ZERO) < 0)
return BigDecimal.ZERO;
switch (years) {
case 0:
if (amount.compareTo(ONE_THOUSAND) <= 0)
return amount.multiply(ONE_PERCENT);
else if (amount.compareTo(FIVE_THOUSAND) <= 0)
return amount.multiply(TWO_PERCENT);
else
return amount.multiply(THREE_PERCENT);
case 1:
if (amount.compareTo(ONE_THOUSAND) <= 0)
return amount.multiply(ONE_PERCENT);
else if (amount.compareTo(FIVE_THOUSAND) <= 0)
return amount.multiply(TWO_POINT_FIVE_PERCENT);
else
return amount.multiply(FOUR_PERCENT);
case 2:
default:
if (amount.compareTo(ONE_THOUSAND) <= 0)
return amount.multiply(TWO_PERCENT);
else if (amount.compareTo(FIVE_THOUSAND) <= 0)
return amount.multiply(THREE_PERCENT);
else if (amount.compareTo(TEN_THOUSAND) <= 0)
return amount.multiply(FOUR_PERCENT);
else
return amount.multiply(FIVE_PERCENT);
}
}
You could put every case in a separate method, this depends a bit on the number and complexity of cases.
At some point, you will (should) use a database to do this. I assume, the numbers can be changed somewhere, then you will need them at some changeable place anyway. And a database is quite good in doing such range searches. (something like select percentage from table where year = and amount = (select max(amount) from table where < amount and year = order by amount desc))
