HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaModerate

Coin dispenser program

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
coinprogramdispenser

Problem

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 denominationMap = new HashMap();

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.

Solution

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;

Code Snippets

assertEquals(cdUnit.getDenominationMap().get(new BigDecimal("20.00")), new Integer(1));
assertEquals(new Integer(1), cdUnit.getDenominationMap().get(new BigDecimal("20.00")));
assertEquals(1, cdUnit.getDenominationMap().get(new BigDecimal("20.00")).intValue());
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));
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());

Context

StackExchange Code Review Q#82689, answer score: 10

Revisions (0)

No revisions yet.