10
\$\begingroup\$

I had to build a cash register that included a Coin class that allowed for all types of coins to be used. Please note that for this, no control structures can be used and no data structures such as arrays, lists, and what not. Any constructive criticism is welcome.

public class CashRegisterTest
{
    //Test the methods in CashRegister class
    //@param args not used
    public static void main(String[] args)
    {
        //European Coins
        final double EURO = 1;
        final double FIFTY_CENT = .5;
        final double TWENTY_CENT = .2;
        final double TEN_CENT = .1;
        final double FIVE_CENT = .05;
        final double ONE_CENT = .01;

        //American Coins
        final double DOLLAR = 1;
        final double QUARTER = 0.25;
        final double DIME = 0.10;
        final double NICKEL = 0.05;
        final double PENNY = 0.01;

        CashRegister register = new CashRegister();
        register.recordPurchase(10.99);
        register.recordPurchase(10.01);
        register.enterPayment(20, new Coin(EURO));
        register.enterPayment(2, new Coin(FIFTY_CENT));

        System.out.println(register.returnChange());

        CashRegister registerTwo = new CashRegister();
        registerTwo.recordPurchase(10.99);
        registerTwo.recordPurchase(10.01);
        registerTwo.enterPayment(20, new Coin(DOLLAR));
        registerTwo.enterPayment(4, new Coin(QUARTER));

        System.out.println(register.returnChange());
    }
}
public class CashRegister
{
    private double purchase;
    private double payment;

    private int items;

    //Construct a cash register with no money.
    public CashRegister()
    {
        payment = 0;
        purchase = 0;
        items = 0;
    }

    //Records the purchase price of an item
    //@param amount the price of the purchased item
    public void recordPurchase(double amount)
    {
        purchase += amount;
        items++;
    }

    //Enter payment recieved.
    //@param amount the amount of coins given
    //@param value the coin that has been given.
    public void enterPayment(int amount, Coin value)
    {
        payment += amount * value.getValue();
    }

    //Compute the change due and set the machine for the next customer.
    //@return change the amount due to the customer.
    public double returnChange()
    {
        return payment - purchase;
    }

    //Get the item count
    //@return items the amount of items in this transaction
    public int getItemCount()
    {
        return items;
    }
}
public class Coin
{
    private double value;

    public Coin(double value)
    {
        this.value = value;
    }

    public double getValue()
    {
        return value;
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Technically speaking, American currency has half-dollar coins too. They're not common, but they are still officially produced and in circulation presently. \$\endgroup\$ – Southpaw Hare Dec 1 '16 at 21:56
8
\$\begingroup\$

You really shouldn't use double for currency. It's probably fine for this, but in a real world application, the accuracy problems you'd experience would be terrible to deal with. It's recommended that you use the currency class for money.


Also, just a small note. I like your brace style, but most Java folks use a different convention. Consistency within a code base is most important, but folks get kind of crazy about this kind of trivial stuff. Best to do as the Romans when in Rome.

public Coin(double value){
    this.value = value;
}
\$\endgroup\$
  • 2
    \$\begingroup\$ I didn't even notice the brace style, I'm so used to C# \$\endgroup\$ – ArtOfCode Dec 1 '16 at 20:37
  • 1
    \$\begingroup\$ IDEs should enforce a standard. There is no good reason why devs have to spend any significant percentage of their time dealing with code formatting issues \$\endgroup\$ – Brad Thomas Dec 1 '16 at 22:02
  • 1
    \$\begingroup\$ The currency class stores metadata about a currency (eg symbol, number of decimal places); not actual amounts of currency. IF you're minimizing 3rd party library use, you'd want to use the BigDecimal class to store your values in. If 3rd party lobs are OK, there's Joda Money. stackoverflow.com/questions/8148684/… \$\endgroup\$ – Dan Neely Dec 2 '16 at 6:22
7
\$\begingroup\$

The CashRegister should be handling a Purchase - that's another class. As written your CashResgister is a purchase, it is not a cash register. This is because there are class-level variables for a single purchase and payment. A CR should hold a list of all the items bought.

When I go shopping I buy an item. I don't buy a purchase. But my "Purchase" is an item + its price.

A "Cash Register" records the purchase object, handles the money transaction, calculates change, taxes, updating inventory counts perhaps (as suggested by the items property).

Another CashRegister function is end of day reconciliation. I mean simply adding up all the purchase.Price, purchase.Tax collected, and clearing it's memory to get ready for the next day.

Even if you cannot have a List<Purchase> (list of purchases), your CR should have a Purchase object, not separate properties. Use objects.

\$\endgroup\$
5
\$\begingroup\$

class or enum

This may be out of scope of you're assignment, however it's worth considering using an enum rather than a class to represent the different coins.

Currency

As it stands, you're representing coins from different currencies, however the coin only has a value, it doesn't have a currency associated with it. Similarly the cashregister doesn't take the currency of the coins into account.

Naming

Naming often suprisingly hard. It is however also important for making your code easier to read.

Generally, try to name things for what they represent, not what you're interested in about them. Consider this code:

public void enterPayment(int amount, Coin value)
{
    payment += amount * value.getValue();
}

You're passing in a coin, not a value, naming it as such gets rid of the nasty value.getValue.

public void enterPayment(int amount, Coin coint)
{
    payment += amount * coin.getValue();
}

Change

Return change is returning the value of the change to be paid. Given the way payment was made, this feels wrong. It feels like it should perhaps be returning the coins that would make up the change. This would be a pain without using control structures, however you could do it by returning a coin at a time and subtracting the coin's value, until the changeDue was 0.

Reset

Your comment on returnChange says:

Compute the change due and set the machine for the next customer.

You do the computation, however you don't reset the machine ready for the next customer. It seems like this should be zero'ing out the currentPurchase, items, purchase (and possibly adding it to a totalPurchases etc variables). Since it doesn't reset, you're currently having to create a new CashRegister for each transaction.

\$\endgroup\$
  • 1
    \$\begingroup\$ Possibly you might want to rename returnChange into a endTransaction and add a getCurrentBalance function \$\endgroup\$ – Catprog Dec 2 '16 at 4:09

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.