14
\$\begingroup\$

I've decided to go tech and learn a language. I have been reading Java for a week and here is my first attempt. I am making a habit to write JUnit test cases so that I start on correct path. Here is an attempt to write program that dispenses coins in the denomination in the range of $20 to 1 cent inclusive.

package org.moolah.javadollah;

import java.math.BigDecimal;
import java.util.HashMap;
import java.util.Map;

public class ChangeDispenser {

        private static final BigDecimal TWENTYDOLLARS = new BigDecimal("20.00");
        private static final BigDecimal TENDOLLARS = new BigDecimal("10.00");
        private static final BigDecimal FIVEDOLLARS = new BigDecimal("5.00");
        private static final BigDecimal ONEDOLLAR = new BigDecimal("1.00");
        private static final BigDecimal TWENTYFIVECENTS = new BigDecimal("0.25");
        private static final BigDecimal TENCENTS = new BigDecimal("0.10");
        private static final BigDecimal FIVECENTS = new BigDecimal("0.05");
        private static final BigDecimal ONECENT = new BigDecimal("0.01");
        private static final Integer ZERO = new Integer(0);

        private Map<BigDecimal, Integer> denominationMap = new HashMap<BigDecimal, Integer>();

        public ChangeDispenser() {
            denominationMap.put(TWENTYDOLLARS, ZERO);
            denominationMap.put(TENDOLLARS, ZERO);
            denominationMap.put(FIVEDOLLARS, ZERO);
            denominationMap.put(ONEDOLLAR, ZERO);
            denominationMap.put(TWENTYFIVECENTS, ZERO);
            denominationMap.put(TENCENTS, ZERO);
            denominationMap.put(FIVECENTS, ZERO);
            denominationMap.put(ONECENT, ZERO);
        }

        // Method that does the logical work
        public void dispense(BigDecimal amount) {
            System.out.println("Entering dispense method");
            System.out.println("Amount submitted: " + amount.toString());

            if (amount.compareTo(TWENTYDOLLARS) == 0 || amount.compareTo(TWENTYDOLLARS) == 1) {
                System.out.println("$20 block");
                amount = calculateRemainder(amount, TWENTYDOLLARS);
            }

            if (amount.compareTo(TENDOLLARS) == 0 || amount.compareTo(TENDOLLARS) == 1) {
                System.out.println("$10 block");
                amount = calculateRemainder(amount, TENDOLLARS);
            }

            if (amount.compareTo(FIVEDOLLARS) == 0 || amount.compareTo(FIVEDOLLARS) == 1) {
                System.out.println("$5 block");
                amount = calculateRemainder(amount, FIVEDOLLARS);
            }

            if (amount.compareTo(ONEDOLLAR) == 0 || amount.compareTo(ONEDOLLAR) == 1) {
                System.out.println("$1 block");
                amount = calculateRemainder(amount, ONEDOLLAR);
            }

            if (amount.compareTo(TWENTYFIVECENTS) == 0 || amount.compareTo(TWENTYFIVECENTS) == 1) {
                System.out.println("25c block");
                amount = calculateRemainder(amount, TWENTYFIVECENTS);
            }

            if (amount.compareTo(TENCENTS) == 0 || amount.compareTo(TENCENTS) == 1) {
                System.out.println("10c block");
                amount = calculateRemainder(amount, TENCENTS);
            }

            if (amount.compareTo(FIVECENTS) == 0 || amount.compareTo(FIVECENTS) == 1) {
                System.out.println("5c block");
                amount = calculateRemainder(amount, FIVECENTS);
            }

            if (amount.compareTo(ONECENT) == 0 || amount.compareTo(ONECENT) == 1) {
                System.out.println("1c block");
                amount = calculateRemainder(amount, ONECENT);
            }
        }

        private BigDecimal calculateRemainder(BigDecimal dollarAmount, BigDecimal denomination) {
            System.out.println("Entering calculateRemainder method");
            int count = 0;
            while (dollarAmount.compareTo(denomination) == 0 || dollarAmount.compareTo(denomination) == 1) {
                dollarAmount = dollarAmount.subtract(denomination);
                count++;
            }
            denominationMap.put(denomination, count);
            return dollarAmount;
        }

        public Map<BigDecimal, Integer> getDenominationMap() {
            return denominationMap;
        }

        public void setDenominationMap(Map<BigDecimal, Integer> denominationMap) {
            this.denominationMap = denominationMap;
        }


}

Unit test

package org.moolah.javadollah.test;

import static org.junit.Assert.*;

import java.math.BigDecimal;

import org.junit.Before;
import org.junit.Ignore;
import org.junit.Test;
import org.moolah.javadollah.ChangeDispenser;

public class ChangeDispenserTestCase {

    private ChangeDispenser cdUnit;

    @Before
    public void setUp() {
        cdUnit = new ChangeDispenser();
    }

    @Ignore
    @Test
    public void test() {
        fail("Not yet implemented");
    }

    @Test
    public void testDispenserTwentyExact() {
        cdUnit.dispense(new BigDecimal("20.00"));
        assertEquals(cdUnit.getDenominationMap().get(new BigDecimal("20.00")), new Integer(1));
        assertEquals(cdUnit.getDenominationMap().get(new BigDecimal("0.10")), new Integer(0));
        assertEquals(cdUnit.getDenominationMap().get(new BigDecimal("0.01")), new Integer(0));
    }

    @Test
    public void testDispenserZeroExact() {
        cdUnit.dispense(new BigDecimal("0.00"));
        assertEquals(cdUnit.getDenominationMap().get(new BigDecimal("20.00")), new Integer(0));
        assertEquals(cdUnit.getDenominationMap().get(new BigDecimal("0.10")), new Integer(0));
        assertEquals(cdUnit.getDenominationMap().get(new BigDecimal("0.01")), new Integer(0));
    }

    @Test
    public void testDispenserTwenty() {
        cdUnit.dispense(new BigDecimal("20.23"));
        assertEquals(cdUnit.getDenominationMap().get(new BigDecimal("20.00")), new Integer(1));
        assertEquals(cdUnit.getDenominationMap().get(new BigDecimal("0.10")), new Integer(2));
        assertEquals(cdUnit.getDenominationMap().get(new BigDecimal("0.01")), new Integer(3));
    }

    @Test
    public void testDispenserTen() {
        cdUnit.dispense(new BigDecimal("10.23"));
        assertEquals(cdUnit.getDenominationMap().get(new BigDecimal("10.00")), new Integer(1));
        assertEquals(cdUnit.getDenominationMap().get(new BigDecimal("0.10")), new Integer(2));
        assertEquals(cdUnit.getDenominationMap().get(new BigDecimal("0.01")), new Integer(3));
    }

    @Test
    public void testDispenserFive() {
        cdUnit.dispense(new BigDecimal("5.23"));
        assertEquals(cdUnit.getDenominationMap().get(new BigDecimal("5.00")), new Integer(1));
        assertEquals(cdUnit.getDenominationMap().get(new BigDecimal("0.10")), new Integer(2));
        assertEquals(cdUnit.getDenominationMap().get(new BigDecimal("0.01")), new Integer(3));
    }


    @Test
    public void testDispenserOneDollar() {
        cdUnit.dispense(new BigDecimal("3.23"));
        assertEquals(cdUnit.getDenominationMap().get(new BigDecimal("1.00")), new Integer(3));
        assertEquals(cdUnit.getDenominationMap().get(new BigDecimal("0.10")), new Integer(2));
        assertEquals(cdUnit.getDenominationMap().get(new BigDecimal("0.01")), new Integer(3));
    }

}
\$\endgroup\$

4 Answers 4

10
\$\begingroup\$

As @Caridorc already said, the biggest problem is the repetitive code. That should be improved first. I will add a bunch of things on top that are also very important.

A simpler internal representation of amounts

BigDecimal seems overkill for this simple exercise. Since you're not using smaller units then a cent, I suggest to represent the amount to dispense as cents instead. Unless you really need to be able to dispense gargantuan amounts of cash, this could be a good and greatly simplified alternative approach.

For this alternative approach in practice, see my answer to a similar question.

Unit testing

First of all, it's great that you're writing JUnit tests from the start. It's a great practice that will put you the right track and get you far. Stick to it! I have a couple of recommendations to improve your testing technique.


In JUnit tests, the recommended practice in assertions is to put the expected value on the left side, and the actual value on the right. So instead of:

assertEquals(cdUnit.getDenominationMap().get(new BigDecimal("20.00")), new Integer(1));

This is better:

assertEquals(new Integer(1), cdUnit.getDenominationMap().get(new BigDecimal("20.00")));

And instead of creating an Integer object, it will be simpler to compare primitive int values:

assertEquals(1, cdUnit.getDenominationMap().get(new BigDecimal("20.00")).intValue());

This kind of testing is not exactly easy to read:

cdUnit.dispense(new BigDecimal("20.23"));
assertEquals(cdUnit.getDenominationMap().get(new BigDecimal("20.00")), new Integer(1));
assertEquals(cdUnit.getDenominationMap().get(new BigDecimal("0.10")), new Integer(2));
assertEquals(cdUnit.getDenominationMap().get(new BigDecimal("0.01")), new Integer(3));

It's also not very strict: what if cdUnit contains some garbage? It would be better to test the values of all the other denominations too. Here's a trick that's admittedly a bit lazy, but it's more strict and I think more readable too than the original:

assertEquals("{0.01=3, 0.05=0, 0.10=2, 0.25=0, 1.00=0, 5.00=0, 10.00=0, 20.00=1}",
        new TreeMap<>(cdUnit.getDenominationMap()).toString());

The reason to wrap the map in a TreeMap is to ensure the ordering.

A cleaner approach would be to build the expected denomination map manually. Use a helper method to create a map with all zero values, and update it with the expected values, and then compare that map with cdUnit.getDenominationMap().

Other minor things:

This is more complicated than it needs to be:

private static final Integer ZERO = new Integer(0);

You could do simply:

private static final Integer ZERO = 0;
\$\endgroup\$
2
  • 1
    \$\begingroup\$ Being pernickety here, but "recommended practice in assertions is to put the expected value on the left side" depends on the assertion library one uses... For Hamcrest matchers, they usually read assertThat(computedValue, is(expectedValue)). :) \$\endgroup\$
    – h.j.k.
    Feb 26, 2015 at 23:39
  • \$\begingroup\$ @h.j.k. valid point. I updated to clarify that this is a JUnit convention, thanks! \$\endgroup\$
    – janos
    Feb 27, 2015 at 21:34
5
\$\begingroup\$

I will give you pseudo-code and leave to you the fun of implementing it:

You wrote:

denominationMap.put(TWENTYDOLLARS, ZERO);
denominationMap.put(TENDOLLARS, ZERO);
denominationMap.put(FIVEDOLLARS, ZERO);
denominationMap.put(ONEDOLLAR, ZERO);
denominationMap.put(TWENTYFIVECENTS, ZERO);
denominationMap.put(TENCENTS, ZERO);
denominationMap.put(FIVECENTS, ZERO);
denominationMap.put(ONECENT, ZERO);

Do not repeat yourself.

MONEY_TYPES = [TWENTYDOLLARS,TENDOLLARS....ONECENT]
foreach (money_type in MONEY_TYPES) {
     denominationMap.put(money_type, ZERO);
}

Down here you have tremendous code duplication: you should use a dictionary.

    if (amount.compareTo(TWENTYDOLLARS) == 0 || amount.compareTo(TWENTYDOLLARS) == 1) {
        System.out.println("$20 block");
        amount = calculateRemainder(amount, TWENTYDOLLARS);
    }

    if (amount.compareTo(TENDOLLARS) == 0 || amount.compareTo(TENDOLLARS) == 1) {
        System.out.println("$10 block");
        amount = calculateRemainder(amount, TENDOLLARS);
    }

    if (amount.compareTo(FIVEDOLLARS) == 0 || amount.compareTo(FIVEDOLLARS) == 1) {
        System.out.println("$5 block");
        amount = calculateRemainder(amount, FIVEDOLLARS);
    }

    if (amount.compareTo(ONEDOLLAR) == 0 || amount.compareTo(ONEDOLLAR) == 1) {
        System.out.println("$1 block");
        amount = calculateRemainder(amount, ONEDOLLAR);
    }

    if (amount.compareTo(TWENTYFIVECENTS) == 0 || amount.compareTo(TWENTYFIVECENTS) == 1) {
        System.out.println("25c block");
        amount = calculateRemainder(amount, TWENTYFIVECENTS);
    }

    if (amount.compareTo(TENCENTS) == 0 || amount.compareTo(TENCENTS) == 1) {
        System.out.println("10c block");
        amount = calculateRemainder(amount, TENCENTS);
    }

    if (amount.compareTo(FIVECENTS) == 0 || amount.compareTo(FIVECENTS) == 1) {
        System.out.println("5c block");
        amount = calculateRemainder(amount, FIVECENTS);
    }

    if (amount.compareTo(ONECENT) == 0 || amount.compareTo(ONECENT) == 1) {
        System.out.println("1c block");
        amount = calculateRemainder(amount, ONECENT);
    }
}

pseudo-code dictionary:

// Small helper method that improves readability and reduces duplication.
int zeroOrOne(money) {
     return amount.compareTo(TWENTYDOLLARS) == 0 || amount.compareTo(TWENTYDOLLARS) == 1
}



ConditionActionDictionary = {(isZeroOrOne(TWENTYDOLLARS) : System.out.println("$20 block");
    amount = calculateRemainder(amount, TWENTYDOLLARS); ....
isZeroOrOne(ONECENT) :    System.out.println("1c block");
        amount = calculateRemainder(amount, ONECENT);,

foreach condition in ConditionActionDictionary {
    execute(dictionary[condition])
}

I hope my pseudo-code is clear, remember that if you write some massive code for an easy task you are doing somehing wrong... if you have any questions feel free to ask.


The Dictionary I suggested above may or may not be possible in Java so here I give you another example:

You are right, a dictionary is a key value structure, sorry if I was not clear maybe you could:

foreach possible_amount_value in ALL_AMOUNTS {
    if (amount.compareTo(possible_amount_value) <= 1) {
        print MessagesDict[amount]
        amount = calculateRemainder(amount, possible_amount_value);
    }
}

Now MessagesDict is just a dictionary containing the right messages that should be easier to do.

\$\endgroup\$
2
  • \$\begingroup\$ Thank you for your time and suggestions. I implemented the first change. I am confused about the second suggestion of Dictionary ... pls elaborate a little. My understanding is that Dictionary is a key value structure. Here above you are suggesting to use boolean value returned from the helper method as key? Sorry for n00b questions. \$\endgroup\$ Feb 26, 2015 at 23:10
  • \$\begingroup\$ @geoandroid23 Fell free to ask :) . I updated the answer, tell me if it is clearer now. \$\endgroup\$
    – Caridorc
    Feb 27, 2015 at 13:16
3
\$\begingroup\$

Use Enum

Anytime you find yourself declaring several similar final fields, you probably should be making use of enumeration. I had a similar case answered here and it's when I realized as much myself.

I would extract your final fields into a separate Currency file with the following:

import java.math.BigDecimal;

public enum Currency {
                 ZERO(new BigDecimal("0.00")),
             ONE_CENT(new BigDecimal("0.01")),
           FIVE_CENTS(new BigDecimal("0.05")),
            TEN_CENTS(new BigDecimal("0.10")),
    TWENTY_FIVE_CENTS(new BigDecimal("0.25")),
           ONE_DOLLAR(new BigDecimal("1.00")),
         FIVE_DOLLARS(new BigDecimal("5.00")),
         TEN_DOLLARS(new BigDecimal("10.00")),
      TWENTY_DOLLARS(new BigDecimal("20.00"));

    private final BigDecimal value;

    Currency(BigDecimal value) {
        this.value = value;
    }

    public BigDecimal getValue() {
        return this.value;
    }

    @Override
    public String toString() {
        return this.name().replace("_", " ");
    }
}

To extract the data you need you use an enhanced for loop, for example if you still wanted a map with the currency and their names

for (Currency c : Currency.values()) {
    exampleMap.put(c, c.toString());
}
\$\endgroup\$
1
  • \$\begingroup\$ I will possibly post a rags-to-riches some time later, but your enum will be a lot more useful (for calculations) if you start from the larger values. :) \$\endgroup\$
    – h.j.k.
    Feb 27, 2015 at 2:26
3
\$\begingroup\$

Use of Comparable.compareTo()

You have a lot of the following chunk in your code:

if (number.compareTo(another) == 0 || number.compareTo(another) == 1) {
    ...
}

What you really intend to check here is that number is not lesser than another, and I will recommend putting it this way instead:

if (number.compareTo(another) >= 0) {
    ...
}

Debugging

Your System.out.println("1c block"); debugging lines can probably be shifted into the method itself, and it will be even better to use a logging library (e.g. SLF4J).

Flashing your denominationMap

Your public getters/setters for your denominationMap lets anyone freely modify its contents, which should not be the case. This is because you are computing your results into it, so you should remove the setter and have the getter return an unmodifiable copy of it:

public Map<BigDecimal, Integer> getDenominationMap() {
    return Collections.unmodifiableMap(denominationMap);
}

Enums to the rescue(?)

I can't find the CR question right now, but I remembered a similar question being asked and one of the solutions was to use an Enum, for two reasons:

  • The Enum values have a strict ordering on it via values(), so you can use that to iterate from larger to smaller denominations instead of having to do so 'manually'.
  • Your can store your results in an EnumMap, which also maintains the ordering.

I have posted an Enum-driven approach question here: Let's Break Down the Party

Unit test method names

testDispenserTwenty should probably be written as testDispenserTwentyTwentyThree, and testDispenserOneDollar is definitely not testing for a dollar. :p You can also consider TestNG over JUnit, as I prefer TestNG's way of doing parameterized testing.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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