2
\$\begingroup\$

Just started learning Java three days ago and am currently doing a problem in my textbook where I do some basic cash register functionality. So far my code can factor in taxes, let the user get change, and tell them the amount the actual tax is.

public class Register {
    private double purchase;
    private double payment;
    private double tax;
    public Register()
    {
    }
    public Register(double tax)
    {
        this.tax = tax;
    }

    public void recordPurchase(double amount)
    {
        purchase = purchase + amount;
    }
    public void enterPayment(double amount)
    {
        payment = amount;
    }

    public void recordTaxablePurchase()
    {
        purchase = purchase  + (purchase *(tax/100));
    }
    public double getTotalTax()
    {
        return (purchase*0.10);
    }
    public void getSalesTotal()
    {

    }
    public double giveChange()
    {
        double change = payment - purchase;
        purchase = 0;
        payment = 0;
        return change;  
    }

}

Running it

public class RegisterTest {

    public static void main(String [] args)
    {
        Register pay = new Register(10);
        pay.recordPurchase(100);
        pay.recordTaxablePurchase();
        pay.enterPayment(200);

        double change = pay.giveChange();
        System.out.println(change);
    }   
}

Now the next part is to do:

After closing time, the store manager would like to know how much business was transacted during the day. Modify the CashRegister class to enable this functionality. Supply methods getSalesTotal and getSalesCount to get the total amount of all sales and the number of sales. Supply a method reset that resets any counters and totals so that the next day’s sales start from zero.

My thoughts: let's add two instance variables which should always be private:

private double totalPurchase;
private int count;

Change recordPurchase to:

public void recordPurchase(double amount)
    {
        purchase = purchase + amount;
        totalPurchase = totalPurchase + amount;
        count = count+1;
    }

Also do the same for tax.

Then simply do:

public double giveSalesTotal()
    {
        return totalPurchase;
    }

    public int getSalesCount()
    {
        return count;
    }

Does this seem like an ok way of doing it? It works, but seems really bad to create another new variable like totalPurchase, instead of using the original purchase.

\$\endgroup\$

closed as off-topic by mdfst13, syb0rg, Malachi, Vogel612, Quill Jul 21 '16 at 0:40

This question appears to be off-topic. The users who voted to close gave this specific reason:

If this question can be reworded to fit the rules in the help center, please edit the question.

  • \$\begingroup\$ Welcome, good first question! If I understand your code correctly, the calculation for the tax is done in a separate method call that can be commented out or omitted altogether and I have to wonder if the textbook you are using is from Panama. ;) \$\endgroup\$ – I'll add comments tomorrow Jul 20 '16 at 13:29
  • \$\begingroup\$ Its called Big Java. I have experience in javascript and php so the logic is easy for me, I am just rolling through syntax and how java works. On chapter 4 currently just finishing up this problem from ch 3 \$\endgroup\$ – Muntasir Alam Jul 20 '16 at 13:35
  • 6
    \$\begingroup\$ Hi, as it stand, this question is on the verge of being off-topic for hypothetical code; especially as regard to the second half of it. It would be way better if you could integrate your thoughts into the original code and ask your questions about the final code (as long as it works as expected). Starting with the problem description could also be a good idea. \$\endgroup\$ – Mathias Ettinger Jul 20 '16 at 13:41
2
\$\begingroup\$

consistent indentation style

You have the opening bracket of your class blocks on the same line:

public class Register {
    private double purchase;

public class RegisterTest {

but other brackets that open blocks are on the next line. No matter what style you choose, be consistent. I suggest you put the brackets on the next line all the time as this is what the majority of your code looks like already.

initialisation of variables

In your constructor, only one variable is initialised according to a parameter.

public Register(double tax)
{
    this.tax = tax;
}

Sure, numeric types have default values, but what are they? It's best to assign values for variables to make things clear.

public Register(double tax)
{
    this.tax = tax;
    purchase = 0;
    payment = 0;
}

that other constructor

Your empty default constructor should be calling your other constructor. This way you can set the default value for the parameter, make sure everything is initialised and reuse the existing code:

public Register()
{
    this(0.0);       
}

public Register(double tax)
{
    this.tax = tax;
    purchase = 0;
    payment = 0;
}

combine operators

Do this

a += b;

instead of that

a = a + b;

calculate the factor for the tax only once

You do this calculation (tax/100) over and over again, although it has a constant result. Do it only once in the constructor to calculate the right factor and later just multiply by that factor:

public Register(double tax)
{
    this.tax = 1 + tax/100;

welcome to Panama

As I tried to point out in a comment, doing the tax calculation is totally optional in your code. If you forget to call the method, it simply doesn't happen.

Your Register is easily manipulated to save taxes. Financial systems are often complicated and people hate to deal with them. Now if your class tries to be helpful with doing financial calculations, it should ensure that the calculations are done right. But as you expose many public methods that have to be called in a certain order, somebody using your class could easily screw that up (either to their advantage or disadvantage). In a real world application, you'd have to run checks on all values if they are within acceptable boundaries.

So far this review is mostly suggesting things that could be considered good practices, but with that last paragraph, it's clear that a line has to be crossed and more fundamental changes are necessary that are likely incompatible with the book you are reading and how it intends to develop more complex concepts proceed at your own ris... uh oooh here it coo...


....oooomes. Whoa, what a bumpy ride! Everybody still on board? Hey, where's the... hmmm looks like that Register class went missing. Whoops! But no worries I'm sure we can come up with a new one.

My expectation from a cash register is to do one main operation on it: I tell it the price to pay, the amount of money given and it will tell me how much change should be given. That would be a method then that I call pay which has the following signature:

double pay (double price, double payment)

I can do this all over again with different values for the next customer in the queue. The values change whenever I interact with the register. The only thing that stays constant is the tax. This means that only tax should be a member of the class, because it persists throughout multiple method calls. The rest of the information comes in as parameter values.

Here's how that looks:

public class Register
{
    private final double tax;

    public Register()
    {
        this(0.0);       
    }

    public Register(double tax)
    {
        this.tax = 1 + tax/100;
    }

    public double pay(double price, double payment)
    {
        return payment - price * tax;
    }

    public static void main (String[] args)
    {
        Register register = new Register(10);

        double change = register.pay(100, 200);
        System.out.println(change);
    }
}

I got lazy and added main to Register. I find myself doing that a lot to create a small executable example program for a specific class contained in that class. In some sense, this is not just self documenting code, but executable documentation. Of course having a separate class is the better choice for more sophisticated testing (unit tests etc.).

tax calculation

tell them the amount the actual tax is

I guess you are referring to that method:

public double getTotalTax()
{
    return (purchase*0.10);
}

The problem with that is that it performs a calculation with a fixed tax factor. If the number passed to the Register is a different tax, this number would be wrong.

To add this method to the new class, it needs to receive the price as a parameter, because purchase is not a member of the class any more. An advantage is that the tax can be calculated, without making a purchase.

public double calculateTax(double price)
{
    return price * (tax - 1);
}

As tax is a factor > 1, 1 has to be subtracted. Wait!, now this calculation has to be done over and over again. Isn't that bad? Yes, but I suspect that calculating the tax happens a lot less often. If need be, this factor could also be stored in a variable of the class.

purchase counting

I would not add up all purchases in the register itself. In programming, if you have many of something, there's often a way to "hold" many things. In general these are called data structures and come in a variety of flavours from simple arrays (that String[] args is one such example) to generic collections.

If somebody wants to purchase many items, they come with such an array for example. Both in real life and online stores, people shop around filling a shopping basket and throw that at the cashier when they are finished. (some roll it, but throwing is most common)

Now imagine your shopping basket is held by the cashier all the time and whenever you want to add or remove something, you have to go to that guy to do it for you. Give him a break, fill your basket yourself.

Here's how a method taking many prices could look like:

public double pay(double price, double payment)
{
    return payment - price * tax;
}

public double pay(double[] prices, double payment)
{
    double total = 0;

    for (double price : prices)
        total += price;

    return pay(total, payment);
}

You can do this with more bling bling in Java 8, but the more important thing to take away is calling the other method to do the calculation there, to keep it in one place and not duplicate it.

I'd say that's it, but guess who turns up with even more stuff when you think "that's it"? Yes, it's

the god d*** store manager

Yeah, it's your third day here and he's already complaining. One problematic thing is that his requests are not very good:

Modify the CashRegister class

I wouldn't do that. The Register class is working fine. Never touch a running system. There are two better approaches to this:

  1. Create a subclass CountingRegister extends Register, do the counting in the overridden pay methods.
  2. Create a decorator/wrapper for the Register class that does the counting in its pay methods before delegating them to it's decoratee/wrappee1.

You haven't heard about inheritance, composition or patterns yet, but this is how I would do it.


Full example code:

public class Register
{
    private final double tax;

    public Register()
    {
        this(0.0);       
    }

    public Register(double tax)
    {
        this.tax = 1 + tax/100;
    }

    public double pay(double price, double payment)
    {
        return payment - price * tax;
    }

    public double pay(double[] prices, double payment)
    {
        double total = 0;

        for (double price : prices)
            total += price;

        return pay(total, payment);
    }

    public double calculateTax(double price)
    {
        return price * (tax - 1);
    }

    public static void main (String[] args)
    {
        Register register = new Register(10);
        double change;

        change = register.pay(100, 200);
        System.out.println(change);

        double[] prices = new double[]{25, 4, 39};  
        change = register.pay(prices, 200);
        System.out.println(change);

        System.out.println("tax for 100 " + register.calculateTax(100));
    }
}

1 the thing that's decorated/wrapped, like employer - employee.

\$\endgroup\$
1
\$\begingroup\$

Am a C# fanatic but looking at your code I can see things that need to be improved

  1. This below code could be refactored

    public void recordPurchase(double amount)
    {
            purchase = purchase + amount;
    }
    

    to

    public void recordPurchase(double amount)
    {
            purchase += amount;
    }
    

    or if you want the purchase = purchase +amount then initialise purchase = 0.0 purchase += amount; could be used

  2. You've use a method for one liner code e.g

    public void recordPurchase(double amount)
    {
        purchase = purchase + amount;
    }
    
    public void enterPayment(double amount)
    {
        payment = amount;
    }
    

    could be refactored as

    public void recordPurchase(double amount)
    {
        purchase = purchase + amount;
        payment = amount;
    }
    
  3. Get Methods in Java have return type except Set Methods

    public void getSalesTotal()
    {.....}
    

    could be refactored as

    public double getSalesTotal()
    {...}
    
  4. The name giveChange is a bad name for a method that returns a value - I suggest you use getChange instead

    public double giveChange()
    {
        double change = payment - purchase;
        purchase = 0;
        payment = 0;
        return change;  
    }
    
\$\endgroup\$
  • \$\begingroup\$ I suggested an edit as it looked like you were trying to include code snippets to the entries in your list of points. In order to do this, you need to add 4 spaces for the list indentation and then another 4 for the code formatting. Doing inline snippets (those with ` and `) over multiple lines looks odd, as it introduces areas between the lines that are filled with white background as opposed to the grey background for code snippets. Hope that helps. \$\endgroup\$ – I'll add comments tomorrow Jul 20 '16 at 15:46
  • \$\begingroup\$ Yes it does - thanks a lot for the suggestion \$\endgroup\$ – Siobhan Jul 20 '16 at 15:56
1
\$\begingroup\$

The main problem of your solution is that your methods have temporal dependencies.

Your are manipulating the "purchase" variable on in total different states of the object without checking if it is allowed. What if someone calls the method to calculate the tax twice beforw you even have a purchase?

My suggestion without providing code but providing a strategy:

Try to keep your object inner state consistent whatever the call order is.... OR

If your structure may be inconsistent do not encapsulate behaviour on it. Let it be a simple Value object. This will be a more functional approach.

\$\endgroup\$

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