1
\$\begingroup\$

I have written a program that, given an amount, returns the interest you would receive on that figure. There are 3 interest rate bands as follows:

  • 1% - £0 to £1000
  • 2% - £1000 to £5000
  • 3% - £5000+

Can anyone tell me if the following is a good solution or are the if statements too restrictive?

public BigDecimal calc(BigDecimal amt) {

    if(amt.compareTo(new BigDecimal(1001))==-1){
        interest = amt.divide(new BigDecimal("100"));

    }else if(amt.compareTo(new BigDecimal(5001))==-1){
        interest = amt.multiply(0.02);

    }else{
        interest = amt.multiply(0.03);
    }

    return interest;
}
\$\endgroup\$
2
\$\begingroup\$

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))

\$\endgroup\$
  • \$\begingroup\$ Thanks. Would your design change if at a later date an additional band was added, e.g 6000+ is 8%? \$\endgroup\$ – TheCoder Apr 3 '13 at 19:07
  • \$\begingroup\$ @heyya99 - Potentially, if you're going to keep adding bands, you want some sort of mapped setup where you can find the 'relevant' entry, then use that to find the relevant percentage. This can be done with two (sorted by amount!) arrays, and a binary search method (and comparator). \$\endgroup\$ – Clockwork-Muse Apr 3 '13 at 20:42
  • \$\begingroup\$ What would the solution be if the follow was added? Customers who have been with the bank for over a certain amount of time will be offered a different set of rates. The new bands are as follows: After one year: 1% - £0 to £1000 2.5% - £1000 to £5000 4% - £5000+ After two years: 2% - £0 to £1000 3% - £1000 to £5000 4% - £5000 to £10000 5% - £10000+ \$\endgroup\$ – TheCoder Apr 3 '13 at 20:55
  • \$\begingroup\$ I have modified my answer to reflect the comments. first question: yes, see answer from clockword-muse, too. This could be done with a database. Second question: this "just" creates one level more of branching. \$\endgroup\$ – tb- Apr 4 '13 at 11:43
  • \$\begingroup\$ In reality, you would never define a constant as TWO_PERCENT or even as TWO. These would be TIER_1 , TIER_2 &c. Nor would they be constants as "transfer pricing" may dictate rates be set according to market, which could be twice a day. It would be a very special case of a 1 year fixed term deposit we are coding for here otherwise. But I suppose it needed to be a simple example and requirements may have been explicit. Beware of business analysts writing code. \$\endgroup\$ – mckenzm Jul 15 '15 at 5:40

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.