Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

How would one optimize this code?

import java.util.*;
public class VendingMachineAssignement {

    public static void main(String[] args) {
        Scanner keyboard = new Scanner(System.in);

        double item1 = 1.25;
        double item2 = .75;
        double item3 = .90;
        double item4 = .75;
        double item5 = 1.50;
        double item6 = .75;

        System.out.print("Enter an item number: ");
        int item = keyboard.nextInt();

        System.out.print("Enter the amount paid: ");
        double paid = keyboard.nextDouble();

        if (item == 2 || item == 4 || item == 6)
        {
            if (paid >= item2)
            {
                System.out.println("Thank you for buying item " + item + ", your change is $" + (paid-item2) + ". Please come again!");
            }
            if (paid < item2)
            {
                System.out.println("Please insert another " + "$" + (item2-paid));
            }
        }

        else if (item == 1)
        {
            if (paid >= item1) 
            {
                System.out.println("Thank you for buying item " + item + ", your change is $" + (paid-item1) + ". Please come again!");
            }
            if (paid < item1)
            {
                System.out.println("Please insert another " + "$" + (item1-paid));
            }
        }
        else if (item == 3)
        {
            if (paid >= item3) 
            {
                System.out.println("Thank you for buying item " + item + ", your change is $" + (paid-item3) + ". Please come again!");
            }
            if (paid < item3)
            {
                System.out.println("Please insert another " + "$" + (item3-paid));
            }
        }
        else if (item == 5)
        {
            if (paid >= item5) 
            {
                System.out.println("Thank you for buying item " + item + ", your change is $" + (paid-item5) + ". Please come again!");
            }
            if (paid < item5)
            {
                System.out.println("Please insert another " + "$" + (item5-paid));
            }
        }


    }

}

And also, for example, if I enter 5 for item and .10 for paid, then why does it say Please insert another $1.4 instead of 1.40? Even though 1.50 - 1.4 and 1.50 - 1.40 would be the same, how come it does not display 1.40?

share|improve this question

4 Answers 4

up vote 18 down vote accepted

Things I would fix:

All of your condition test statements do exactly the same thing.

else if (item == 1)
{
    if (paid >= item1) 
    {
        System.out.println("Thank you for buying item " + item + ", your change is $" + (paid-item1) + ". Please come again!");
    }
    if (paid < item1)
    {
        System.out.println("Please insert another " + "$" + (item1-paid));
    }
}
else if (item == 3)
{
    if (paid >= item3) 
    {
        System.out.println("Thank you for buying item " + item + ", your change is $" + (paid-item3) + ". Please come again!");
    }
    if (paid < item3)
    {
        System.out.println("Please insert another " + "$" + (item3-paid));
    }
}
...

You can use arrays to help solve this issue.

double[] prices = new double[] { 1.25, 0.75, 0.9, 0.75, 1.5, 0.75 };

Then you can access those specific values with prices[item - 1]. Remember that arrays start with the index 0, so without subtracting 1 you will end up with an off-by-one error. The comparison is now simplified.

if (paid >= (prices[item - 1]))
{
    System.out.println("Thank you for buying item " + item + ", your change is " + (paid - prices[item - 1]) + ". Please come again!");
}

You ask the user to insert more money, but never actually allow him to.

if (paid < item2)
{
    System.out.println("Please insert another " + "$" + (item2-paid));
}

This can easily be solved with a while loop and another pair of input request statements.


You never close your Scanner when you are finished with it (a memory issue).

keyboard.close();

You never check if the item you buy is actually within the bounds of the array length.

if (!(item <= prices.length))
{
    System.out.println("Sorry, we don't have that item in stock.");
    keyboard.close();
    return;
}

When working with money, it is hard to work with doubles, they are too imprecise. An example of this is that 0.42 - .21 is likely something like 0.209999999999999997 rather than 0.21 exactly.

We are going to use the NumberFormat class to make sure that the our double values will stay accurate.

NumberFormat.getCurrencyInstance().format(number)

Recommendations that are optional:

Don't do this:

import java.util.*;

It could be problematic for the compiler to import a bunch of packages at once. If two packages provide the same type, both are imported, and the type is used in the class, a compile-time error occurs. This is described in JLS 6.5.5.1:

Otherwise, if a type of that name is declared by more than one type-import-on-demand declaration of the compilation unit, then the name is ambiguous as a type name; a compile-time error occurs.

In addition, it also saves a bit of memory. And your IDE (if you use one), should have the ability to do this automatically.


You might note that this:

public static void main(String... args)

Is a bit different than your usual:

public static void main(String[] args)

They both do the same thing, but one uses variable arity parameters.


The Scanner.nextInt() method does not read the last newline character of your input, and thus a newline could be consumed if you call Scanner.nextLine. I prefer to use Scanner.nextLine since you can input any valid String, and then just parse it into the type you need (as long as it's a valid type to parse it to).


Final code:

import java.text.NumberFormat;
import java.util.Scanner;

public class Test
{
    public static void main(String... args)
    {
        Scanner keyboard = new Scanner(System.in);
        double[] prices = new double[] { 1.25, 0.75, 0.9, 0.75, 1.5, 0.75 };
        int item = 0;

        System.out.print("Enter an item number: ");
        item = Integer.parseInt(keyboard.nextLine());
        if (!(item <= prices.length))
        {
            System.out.println("Sorry, we don't have that item in stock.");
            keyboard.close();
            return;
        }

        System.out.print("Enter the amount paid: ");
        double paid = Double.parseDouble(keyboard.nextLine());

        while (paid < (prices[item - 1]))
        {
            System.out.print("You need " + NumberFormat.getCurrencyInstance().format(Math.abs((paid - prices[item - 1]))) + " more for item " + item);
            paid += Double.parseDouble(keyboard.nextLine());
        }
        System.out.println("Thank you for buying item " + item + ", your change is " + NumberFormat.getCurrencyInstance().format((paid - prices[item - 1])) + ". Please come again!");

        keyboard.close();
    }
}
share|improve this answer
    
keyboard.close(); - why? It'll just get collected when the function ends, which is the very next thing that happens. –  user2357112 Dec 15 '13 at 0:45
1  
You say that it's hard to work with doubles, but you don't really fix the problem; you just change how they're displayed. If the user puts in 30 cents 3 times, this program will think the user still needs to input a tiny fraction of a nanocent to pay for item 3. –  user2357112 Dec 15 '13 at 0:50
2  
@user2357112 - keyboard goes out of scope when the method terminates, but the GC choose when to clean it up. In a tiny program like this, that will occur immediately when the program itself terminates, but you never know in a longer-running program. Better to pick up good habits early. –  David Harkness Dec 15 '13 at 1:38
    
@user2357112 I don't fix the problem in this case because the program can handle it in this case, and it would be more of a hassle to implement something more dynamic, flexible, and reliable (I'm thinking BigInteger) for such a short program. –  syb0rg Dec 15 '13 at 2:16
1  
Then you can access those specific values with prices[item - 1]. Remember that arrays start with the index 0, so without subtracting 1 you will end up with an off-by-one error. - Or you can put a dummy value at price[0], so item matches index. It's what I tend to do in anything more complex than this, so you don't have to mix'n'match for comparisons. –  Izkata Dec 15 '13 at 3:05

To start of, if using amount of variables of the same data-type, the appropriate way of doing such a thing is an Array; specific case, double [] prices;

An array will allow you to keep a reference to all your values and allowing you to make a single if-else instead of each for every item. Demo:

double [] prices = {1.25, 0.75, 0.90, 0.75, 1.50, 0.75};
System.out.print("Enter an item number: ");
int item = keyboard.nextInt();
System.out.print("Enter the amount paid: ");
double paid = keyboard.nextDouble();
if (paid >= prices[item-1])
     System.out.println("Thank you for buying item " + item + ", your change is $" + (paid-prices[item-1]) + ". Please come again!");
else
     System.out.println("Please insert another " + "$" + (prices[item-1]-paid));

you could make a helper function to add another zero to a string if you want to, like you wanted in 1.4 to become 1.40, another way is to use String.format("%.2f",(paid-prices[item-1])); to get the string.

Hope that helps. Please ask if anything isn't clear.

EDIT: changed to output to decremented value of item since an Array starts with 0, and user's requested first item to be 1.

share|improve this answer
2  
initializing the array can be done like this: double[] prices = new double[]{ 1.25, 0.75, 0.9, 0.75, 1.5, 0.75 }; –  Simon André Forsberg Dec 14 '13 at 22:21
    
Changed due to suggestion. –  user2558461 Dec 14 '13 at 22:22
    
Sorry, damn Firefox submitted the comment when I fixed spelling. See the edit of the comment :) –  Simon André Forsberg Dec 14 '13 at 22:23

Here is a block of code with comments below.

Always consider exceptional cases. That is a crucial part of programming.

// Only import what you need.
import java.util.InputMismatchException;
import java.util.Scanner;

public class Main {

    public static void main(String[] args) {

        Scanner keyboard = new Scanner(System.in);

        // Use an array if you have a list.
        double[] itemPrices = {1.25, .75, .90, .75, 1.50, .75};

        // This array starts from 0 end ends at 5.
        // Corresponding to item #1 to #6.
        int maxItem = itemPrices.length;

        // Have we been given an item that is Ok yet?
        boolean itemFound = false;

        // We have not been provided an item # yet. This will have to be non-null
        // once we exit the do-while statement below.
        Integer item = null;

        // read an item number until an existing item is found.
        do {
            System.out.print("Enter an item number: ");
            try {
                item = keyboard.nextInt();
                if ( item >= 1 && item <= maxItem ) {
                    itemFound = true;
                } else {
                    // Inform the user of the problem. Give them another chance.
                    // String.format is very neat stuff. Use it.
                    String message = String.format("Unfortunately, item #%d does not exist.", item);
                    System.out.println(message);
                }

            // note how an exception is raised when we get an invalid input.
            // you must consider what happens when the input is invalid.
            } catch (InputMismatchException e) {
                System.out.println("Bad input. Try again.");
                keyboard.next();
            }
        } while (!itemFound);


        Double paid = null;
        do {
            System.out.print("Enter the amount paid: ");
            try {
                paid = keyboard.nextDouble();
            } catch (InputMismatchException e) {
                System.out.println("Bad input. Try again.");
                keyboard.next();
            }
        } while (paid == null);

        double itemValue = itemPrices[item-1];

        double change = paid - itemValue;

        double moneyNeeded = 0 - change;

        if ( moneyNeeded > 0 ) {
            // We can inform the user what the problem was. And again the String.format magic.
            String message = String.format("Please insert another $%.2f.%n", moneyNeeded);
            System.out.println(message);

            // But this time, we will gracefully exit.
            System.exit(0);
        }

        String message = String.format("Thank you for buying item #%d, your change is $%.2f. Please come again!", item, change);

        System.out.println(message);

    }

}

Would be much prettier if done using the Scala programming language. Those loops could be replaced with tail recursion, absolutely no mutability (mutability is not pretty, but immutability is harder to do in Java).

Edit: Exercise for the reader: fix the moneyNeeded if-statement to have the correct "business"-logic. At the moment it quits having eaten all your money.

share|improve this answer

Using floating-point data types to represent money is bad practice, due to the inability to represent some rational numbers in binary. Either use java.math.BigDecimal or work in cents instead of dollars.

Parsing a string representation of dollars and cents directly into an integer number of cents, is slightly tricky. One approach is to use BigDecimal to help with the parsing and conversion.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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