7
\$\begingroup\$

I'm new to java and was just wondering if you could see anything wrong with my code. The program works but I have a feeling it's not the right way to do it.

public class ShoppingItem {

private String ItemName;
    private double ItemPrice;
    private int ItemQty;

public ShoppingItem()
{
        ItemName = "Fruit";
        ItemPrice = 100;
        ItemQty = 1;

}

    public ShoppingItem(String ItemName, double ItemPrice, int ItemQty )
{
        this.ItemName = ItemName;
        this.ItemPrice = ItemPrice;
        this.ItemQty = ItemQty;

}


public String getItemName() {
    return ItemName;
}

public double getItemPrice() {
    return ItemPrice;
}

public double getItemTotalPrice() {
    return ItemPrice * ItemQty;
}

public int getItemQty() {
    return ItemQty;
}

public void setItemName(String ItemName)
{
    this.ItemName = ItemName;
}

    public void setItempPrice(double ItemPrice)
{
    this.ItemPrice = ItemPrice;
}

    public void setItemQty(int ItemQty)
{
    this.ItemQty = ItemQty;
}


    @Override
public String toString()
{
    String state = ItemName + " -  €" + ItemPrice + " x " + ItemQty;
    return state;
}
}

import java.util.ArrayList;
import java.util.Scanner;

public class ShoppingList {

    ArrayList<ShoppingItem> list = new ArrayList<ShoppingItem>();
    //Add a new ShoppingItem to the list
         public void addItem()
    { 
            System.out.println();
            System.out.println("enter in the name of your item");
            Scanner keyboard = new Scanner(System.in);
            String ItemName = keyboard.nextLine();

            System.out.println("enter in the price of your item");
            double ItemPrice = keyboard.nextDouble();

            System.out.println("enter in the Qty of your item");
            int ItemQty = keyboard.nextInt();

            ShoppingItem Item = new ShoppingItem(ItemName, ItemPrice,
                        ItemQty);
            list.add(Item);
            System.out.println("Item Added");
    }

    //Display list and total number of items. 
         public void displayItem(){
             System.out.println( list.size()+ " items. ");
                for (ShoppingItem x : list) {
                    System.out.println(x.toString());
                }

                 }



 }

import java.util.Scanner;
public class Application {

    public static void main( String [] args) { 
        Scanner input = new Scanner(System.in);
        ShoppingList myList = new ShoppingList();
        int userOpt = 0;
        while (userOpt != 3) {
            System.out.println("");
            System.out.println("----- Shopping List------");
            System.out.println("(1) Add an item to the list. ");
            System.out.println("(2) Display list and total number of items. ");
            System.out.println("(3) Exit. ");
            userOpt = input.nextInt();  

            if (userOpt == 1) {
            myList.addItem();

            }

            if (userOpt == 2) {
            myList.displayItem();
            }
        }
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to Code Review! I made a change to fix the code blocks of your code, hopefully it is correct but please double-check to make sure. It would make your question more productive if you provided a little bit more context on what your code is doing. I hope you get some good reviews! \$\endgroup\$ – Phrancis Dec 3 '14 at 16:14
6
\$\begingroup\$

There are several coding style issues that hurt the readability of this program:

  • The indentation is incorrect. Use your IDE's reformat function to correct it (for example Control Shift f in Eclipse)
  • The naming doesn't follow the convention: use camelCase for variable names. CamelCase is reserved for class names.

The naming can be further improved. In a ShoppingItem class, there's no need to prefix each field with "item". Drop that prefix, use simply name, price, and instead of qty, I suggest to spell it out as quantity. The same goes for all the setters and getters of this class.

The parameterless constructor doesn't serve much purpose here. Why should a default ShoppingItem be a "Fruit" with price = 100? This is unnecessary, I suggest to remove it.

ShoppingList also has its own naming issues:

  • Why call a Scanner instance "keyboard"? A scanner is a scanner, input can come from anywhere, not necessarily the keyboard. So I'd call it a "scanner".
  • It would make sense to make displayItem plural, as it's used to display a list of items.
  • When iterating over the list of items, instead of calling the loop variable x, I'd call it item, more natural.

Instead of this:

ArrayList<ShoppingItem> list = new ArrayList<ShoppingItem>();

Declare variables with their interface type:

List<ShoppingItem> list = new ArrayList<ShoppingItem>();

Also, in Java 7 and above, use the diamond operator:

List<ShoppingItem> list = new ArrayList<>();

(Java 7 is the currently supported version, so you should use at least that or above.)

\$\endgroup\$
  • 2
    \$\begingroup\$ This looks like homework, so I would suggest that one of the school "rules" is always implement a default ctor. I however, would use "Null Item", 0.00 and 0 as the values. Or chocolate. You can always use more chocolate. \$\endgroup\$ – Chris Cudmore Dec 3 '14 at 21:55
7
\$\begingroup\$

janos already pointed out some important issues. I'm just going to add a few

Separate concerns

A ShoppingList is a shopping list, nothing more. It could be simply a wrapper around a List with maybe some additional convenience functions. It shouldn't be responsible for creating items from user input, much less asking the user for that input. Therefore, most of the code in method addItem() belongs to another class.

Explicitly declare menu options

You are using magic numbers to identify your menu options. Wait, I just said menu option. This is a very important entity in your program, yet it doesn't appear explicitly anywhere, only implicitly in your println()s and ifs. Let's make an enum for it. This will also allow us to get rid of the magic numbers.

enum MenuOption {
    ADD_ITEM("Add an item to the list."),
    DISPLAY_ITEMS("Display list and total number of items."),
    EXIT("Exit.");

    private final String message;

    private MenuOption(String message){
        this.message = message;
    }

    public String getMessage(){
        return message;
    }
}
ShoppingList myList = new ShoppingList();
Scanner scanner = new Scanner(System.in);
MenuOption selectedOption = null;

while(selectedOption != MenuOption.EXIT){

    selectedOption = askUserOption(scanner);

    switch(selectedOption){
        case ADD_ITEM:                
            myList.addItem();
            break;
        case DISPLAY_ITEMS:
            myList.displayItem();
            break;
    }
}
private MenuOption askUserOption(Scanner scanner){
    System.out.println("");
    System.out.println("----- Shopping List------");
    for(int i=0; i<MenuOption.values().length; i++){
        MenuOption option = MenuOption.values()[i];
        System.out.println("(" + i + ") " + option.getMessage());
    }

    int numericInput = scanner.nextInt();
    // TODO check for invalid inputs
    return MenuOption.values()[numericInput]; 
}

Wrap user prompting

Instead of writing System.out.println() at every point where you need to comunicate to your user, it might be a good idea to write a class for this purpose. For example:

class UserPrompter {
    public void prompt(String message){
        System.out.println(message);
    }
}

This has at least two benefits:

  1. It makes the intent of your code more clear. You are not just printing to stdout (maybe for debugging purposes) - you are actually sending a message to the user of your application.
  2. It makes it easier to change the means of communication in the future. At one time you may want to display the messages on a web page instead of printing to stdout.

This last piece of advise, however, will add additional indirection that may not be needed. You may omit it if you feel that you'll be able to refactor your code at any time and just replace the println()s with something else.

\$\endgroup\$

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.