3
\$\begingroup\$

This is an assignment and I'm trying to improve upon the design factor. Honestly the task seems very trivial to use inheritance so I kind of did it for the sake of doing it. The problem description should be quite clear in the comments provided above the Main class, but do let me know how I can make the question clearer.

Main class

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

/**
 * Main class that models an OrderManager.
 * This class fulfills its responsibility as follows:
 * (i)Taking in inputs to create a sequence of Food or Combo objects, 
 * (ii) Prints the items in the required sequence according to their types,
 * (iii)Taking in creation ids after "end" to create an order sequence to be printed,
 * along with the total price of all items in the order sequence
 */

public class Main {    

    /** 
     * This field holds an ArrayList of burgers,
     * to allow for ease in printing burgers together.
     */
    private static ArrayList<Burger> burgers = new ArrayList<>(); 

    /** 
     * This field holds an ArrayList of snacks,
     * to allow for ease in printing snacks together.
     */
    private static ArrayList<Snack> snacks = new ArrayList<>();

    /** 
     * This field holds an ArrayList of drinks,
     * to allow for ease in printing drinks together.
     */ 
    private static ArrayList<Drink> drinks = new ArrayList<>(); 

    /** 
     * This field holds a HashMap of food objects,
     which allows easy referencing in (iii).
     */
    private static HashMap<Integer,Food> foods = new HashMap<>();

    /**
     * This field holds a HashMap of combos, 
     * which allows easy referencing in (iii).
     */
    private static HashMap<Integer, Combo> combos = new HashMap<>();

    /** 
     * This field contains an ArrayList of keys,
     * to print out multiple combos in their creation sequence.
     */ 
    private static ArrayList<Integer> comboSeq = new ArrayList<>();

    /**
     * Reads inputs using a scanner, creates Food or legal Combo objects, 
     * based on the initial inputs collected.
     */
    public static void main(String[] args) {
        Scanner sc = new Scanner(System.in);
        int counter = 0;

        while (sc.hasNext()) {
            String cmd = sc.next();
            if (cmd.equals("add")){
                String type = sc.next();
                if (type.equals("Combo")){
                    int c1 = sc.nextInt();
                    int c2 = sc.nextInt();
                    int c3 = sc.nextInt();
                    if (!foods.containsKey(c1) || !foods.containsKey(c2) ||
                        !foods.containsKey(c3)) {
                        System.out.println("Error: Invalid combo input "+c1+' '+ c2+ ' '+ c3);
                        continue; 
                    }else if (c1 >= c2 || c2 >= c3) {
                        System.out.println("Error: Invalid combo input "+c1+' '+ c2+ ' '+ c3);
                        continue; 
                    } else {
                        ArrayList<Food> trf = new ArrayList<>();
                        trf.add(foods.get(c1));
                        trf.add(foods.get(c2));
                        trf.add(foods.get(c3));

                        Combo tempCombo = new Combo(trf, counter);
                        combos.put(counter,tempCombo);
                        comboSeq.add(counter);   
                    }


                }else {
                    String desc = sc.next();
                    int price = sc.nextInt();
                    if (type.equals("Burger")){
                        Burger temp = new Burger(desc,price,counter);
                        burgers.add(temp);
                        foods.put(counter,temp);
                    } else if (type.equals("Snack")){
                        Snack temp = new Snack(desc,price,counter);
                        snacks.add(temp);
                        foods.put(counter, temp);
                    } else if (type.equals("Drink")) {
                        Drink temp = new Drink(desc,price,counter);
                        drinks.add(temp);
                        foods.put(counter, temp);
                    }
                }

                counter++;
            } else if (cmd.equals("end")){
                break;
            }
        }

        printSequence();

        printOrders(sc);
    }

    /**
     * Prints the created items in the sequence of burgers, snacks,
     * drinks and combos.
     */
    public static void printSequence(){
        for (Burger tempBurger: burgers) {
            System.out.println(tempBurger);
        }
        for (Snack tempSnack: snacks) {
            System.out.println(tempSnack);
        }
        for (Drink tempDrink: drinks) {
            System.out.println(tempDrink);
        }
        for (int tempInt: comboSeq) {
            Combo tempCombo = combos.get(tempInt);
            tempCombo.internalPrint();
        }
    }

    /**
     * Reads creation ids using a scanner,
     * and prints the objects associated with the creation id
     * The total price of all items printed will also be printed at the end.
     */
    public static void printOrders(Scanner sc){
        int sum = 0; 
        System.out.println("--- Order ---");
        while (sc.hasNext()){
            int num = sc.nextInt();
            if (foods.containsKey(num)) {
                Food tempFood = foods.get(num);
                System.out.println(tempFood);
                sum += tempFood.getPrice();
            } else {
                Combo tempCombo = combos.get(num);
                tempCombo.internalPrint();
                sum += tempCombo.getPrice();
            }
        }
        System.out.println("Total: " + sum);
    }

}

Food class

public class Food {
    private String name;
    private int price;
    private int orderNum;

    public Food(String name, int price, int orderNum){
        this.name = name;
        this.orderNum = orderNum;
        this.price= price;
    }

    public String getName(){
        return this.name;
    }

    public int getPrice(){
        return this.price;
    }

    public int getOrderNum(){
        return this.orderNum;
    }
}

Burger class

/** 
 * This is a subclass of the Food class,
 * which contains a identity field of "Burger",
 * to differentiate between other Food objects.
 * This class also overrides the toString method to print out the burger.
 */
public class Burger extends Food {
    private String name;
    private int price;
    private int orderNum;
    private String type = "Burger";

    public Burger(String name, int price, int orderNum){
        super(name,price,orderNum);
    }

    public String toString() {   
        return "#"+this.getOrderNum()+' '+this.type+": "+this.getName() + " ("+this.getPrice()+')';

    }

}

Drink class

public class Drink extends Food {
    private String name;
    private int price;
    private int orderNum;
    private String type = "Drink";

    public Drink(String name, int price, int orderNum){
        super(name,price,orderNum);
    }

    public String toString() {   
        return "#"+this.getOrderNum()+' '+this.type+": "+this.getName() + " ("+this.getPrice()+')';

    }

}

Snack class

public class Snack extends Food {
    private String name;
    private int price;
    private int orderNum;
    private String type = "Snack";

    public Snack(String name, int price, int orderNum){
        super(name,price,orderNum);
    }

    public String toString() {   
        return "#"+this.getOrderNum()+' '+this.type+": "+this.getName() + " ("+this.getPrice()+')';

    }

}

Combo class

import java.util.ArrayList;

public class Combo {
    ArrayList<Food> foodParts = new ArrayList<>();
    private int orderNum;
    private String type = "Combo";

    public Combo(ArrayList<Food> foodParts, int orderNum){
        this.foodParts = foodParts;
        this.orderNum = orderNum;
    }

    public void internalPrint(){
        System.out.println("#"+this.orderNum+' '+this.type + " ("+this.getPrice()+')');
        for (Food tempFood : this.foodParts) {
            System.out.println("   "+tempFood);
        }
    }



    public int getPrice(){
        int sum = 0;
        for (Food tempFood : this.foodParts) {
            sum += tempFood.getPrice();
        }
        return sum-50;
    }

    public int getOrderNum(){
        return this.orderNum;
    }
}

Test input

add Burger Hamburger 399
add Snack Fries 189
add Combo 0 1 2
add Drink SoftDrink 149
add Combo 0 2 1
add Combo 0 1 2
add Snack Drumlets 169
add Burger CheeseBurger 200
add Drink OrangeJuice 209
end
2 1 4 3 0

Expected output to be fulfilled

Error: Invalid combo input 0 1 2
Error: Invalid combo input 0 2 1
#0 Burger: Hamburger (399)
#5 Burger: CheeseBurger (200)
#1 Snack: Fries (189)
#4 Snack: Drumlets (169)
#2 Drink: SoftDrink (149)
#6 Drink: OrangeJuice (209)
#3 Combo (687)
    #0 Burger: Hamburger (399)
    #1 Snack: Fries (189)
    #2 Drink: SoftDrink (149)
 --- Order ---
 #2 Drink: SoftDrink (149)
 #1 Snack: Fries (189)
 #4 Snack: Drumlets (169)
 #3 Combo (687)
    #0 Burger: Hamburger (399)
    #1 Snack: Fries (189)
    #2 Drink: SoftDrink (149)
 #0 Burger: Hamburger (399)
 Total: 1593 

While writing this code, even though it's simple, I am in a dilemma whether combo should be distinguished from a Food object. In the first place Food doesn't offer much extensibility benefits, but having a subclass Food object that contains other Food objects didn't make any sense to me, so I didn't do it, but many of the functions seems duplicated. Can anyone help me with this? Any other comments on my code are also accepted. Thank you.

\$\endgroup\$
  • \$\begingroup\$ The current question title, which states your concerns about the code, is too general to be useful here. Please edit to the site standard, which is for the title to simply state the task accomplished by the code. Please see How to get the best value out of Code Review: Asking Questions for guidance on writing good question titles. \$\endgroup\$ – Toby Speight Nov 5 '18 at 10:31
1
\$\begingroup\$

Consider renaming Food to OrderableItem or something similar. Among other things, that will remove the cognitive dissonance of making Combo and Drink each a Food.

In most situations, you would prefer OrderableItem to be an interface rather than a class. The interface allows you to treat multiple classes as the same type of thing while still getting different behaviors. Part of the confusion here is that the different things don't actually have different behaviors except for combo. Burgers, snacks, and drinks are all things that you buy and consume. A combo is a group of things that you buy and consume. Only toString differs in behavior.

You're right in a way. With the type in Food or OrderableItem, you don't really need to subclass it for this problem. To make it worth subclassing, you need things that have different behavior. For example, if you wanted to maintain ingredient inventory, you might subclass them. Because inventory is handled differently. Combo should delegate its handling to the items in it. But burger should update the bun and patty inventory while drink updates the cup and syrup inventory. Of course, different burgers and drinks may themselves have different inventory behavior, and this treats those as the same.

If your assignment requires a class, consider making it abstract. Because you don't want to instantiate OrderableItem (i.e. you never want to say new OrderableItem). You only want to instantiate its children. That's essentially the description of an abstract class, a class from which you can inherit but can't instantiate.

Given the way that you use it, consider moving toString to OrderableItem. Then you don't have it all over the place.

    ArrayList<Food> foodParts = new ArrayList<>();

Consider

    List<OrderableItem> items = new ArrayList<>();

These aren't parts of the food. They're different items of food (or drink or combo).

Also, you generally don't make implementations of interfaces as types. Instead, make the interface (List in this case) the type. Among other things that makes it easier to change implementations later.

Same feedback for HashMap and Map.

    private String type = "Drink";

This could be

    private static final String TYPE = "Drink";

It's going to be the same for every instance of a given class.

It is common to make final variables ALL_CAPS but not required.

See Stack Overflow for how to make it a field on OrderableItem and still final.

If you move toString to OrderableItem

    public void internalPrint(){
        System.out.println("#"+this.orderNum+' '+this.type + " ("+this.getPrice()+')');
        for (Food tempFood : this.foodParts) {
            System.out.println("   "+tempFood);
        }
    }

This could be

    @Override
    public String toString() {
        StringBuilder builder = new StringBuilder(super.toString());
        for (OrderableItem item : items) {
            builder.append("\n   ").append(item);
        }

        return builder.toString();
    }

This calls the OrderableItem toString to form the initial string.

This uses StringBuilder to form a string of indefinite length.

I don't see any point in saying that the loop variable is temporary, as that should be obvious.

It is not necessary to use this to access object fields unless there is some ambiguity. E.g. a local variable with the same name as the field. You can use it if you find it more readable that way. It doesn't hurt anything, but it is not necessary in Java.

\$\endgroup\$
  • \$\begingroup\$ Hi, thanks for the feedback, some questions I still have is what is the point of an interface in this question vs a concrete class? I know a reasoning is just a body of signatures, so you mainly only use it as a variable reference and polymorphism, but then for this question there are mainly only getters, which are common among all subclasses of OrderableItem, so why bother making OrderableItem an interface and not a concrete class if I am going to duplicate the code 4 times by extension? \$\endgroup\$ – Prashin Jeevaganth Oct 3 '18 at 6:22
  • \$\begingroup\$ I'm actually relatively new to Java(I think it's obvious from the way I code) and the way I said assignments. For the feedback on setting the type declaration of fields as the “parent” interface/class, you would have to set it to the one which gives access to the methods that you need right? I learnt about ArrayList without going into details of what interfaces it actually implemented, same for map. \$\endgroup\$ – Prashin Jeevaganth Oct 3 '18 at 6:58
  • \$\begingroup\$ Noted on the use of final. However I have this following concern about the use of type in OrderableItem. I believe one of the uses of inheritance, other than using a general parent reference and letting the child instance exist under it, is to remove fields like type, which suggests identifying with fields rather than a class type, which to my experience is what nobody wants? If I were to give TYPE to OrderableItem, then what’s the use of inheritance? I’m going back to square one just by differentiating all OrderableItem using String TYPE. \$\endgroup\$ – Prashin Jeevaganth Oct 3 '18 at 6:58
  • \$\begingroup\$ Nice use of StringBuilder, well I didn’t know that library existed, thanks for informing me about it. Do you have tips on how I can learn more about useful commonly used libraries like these as a Java beginner? I come from Python background so I usually try to find similarity between the 2 languages. I need to know about Exceptions too as they are affecting my debugging. \$\endgroup\$ – Prashin Jeevaganth Oct 3 '18 at 6:59
  • \$\begingroup\$ As for naming of the loop variable, it’s kinda a habit as I don’t know what to name, naming it singularly might not be obvious to me in debugging when compared to the plural form. I use this in my code as a habit too… previous assignment require me to extend classes continuously, putting this.method() would remind me to change them and I might not need to change it since the child classes later on are indeed calling the inherited method instead of a field which is undefined or have private access. \$\endgroup\$ – Prashin Jeevaganth Oct 3 '18 at 7:00
1
\$\begingroup\$

I agree that the object-oriented design could be improved. I'll provide an alternative design, and discuss the reasons for the changes below.

Restaurant.java

Your Main class, and particularly your main() function, has a lot of code. I'd prefer to see an elegant skeleton, with most of the logic delegated to other classes:

import java.util.Scanner;

public class Restaurant {
    public static void main(String[] args) {
        try (Scanner in = new Scanner(System.in)) {
            Menu menu = Menu.parse(in);
            System.out.println(menu);

            while (in.hasNextLine()) {
                String line = in.nextLine();
                try {
                    Order order = Order.parse(menu, line);
                    System.out.println(order);
                } catch (IllegalArgumentException badOrder) {
                    System.out.println("Invalid order input: " + line);
                }
            }
        }
    }
}

What the code above does is build a Menu, then take orders. (And once we've built the menu, why shouldn't we be able to take orders from multiple customers, one per line?)

Note that I've introduced a Menu class and an Order class — objects that I think are missing in your object-oriented modeling. (Your .printOrders() prints while it parses — too much work for one function.) Also observe that each class knows how to parse itself, and supports a human-friendly .toString() representation — a pattern that I shall repeat with each item on the menu. (In this respect, your printOrders() was inconsistent: it calls System.out.println(tempFood), but tempCombo.internalPrint().)

Menu.java

You store the menu using six separate lists and maps: burgers, snacks, drinks, foods, combos, and comboSeq, mostly to facilitate the output ordering implemented in printSequence(). Having so many data structures and classes leads to duplicated code, both in parsing (if (type.equals(…)) … etc.) and in printing (for (Burger tempBurger: burgers) … etc.).

It would be better to store all of the data in one list, and treat the sorting problem as a sorting problem. I've used an enum to help with the sorting: the declaration order of the ItemType enums is used by the Comparator in .toString().

Storing all of the menu items in the same list would, of course, require the single items and the combos to have a common type. I don't recommend making Combo a subclass of Food, especially since combos lack a name and a pre-determined price, and are constructed quite differently. Rather, they should each inherit from a common interface or abstract class (Item).

Error messages should probably be printed to System.err rather than System.out.

import java.util.ArrayList;
import java.util.Comparator;
import java.util.List;
import java.util.Scanner;
import java.util.function.BiFunction;
import java.util.stream.Collectors;

public class Menu {
    public enum ItemType {
        Burger(SingleItem::parse),
        Snack(SingleItem::parse),
        Drink(SingleItem::parse),
        Combo(ComboItem::parse);

        private final BiFunction<Menu, String, Item> parser;

        ItemType(BiFunction<Menu, String, Item> parser) {
            this.parser = parser;
        }

        public static ItemType byName(String name) {
            return Enum.valueOf(ItemType.class, name);
        }
    }

    public static abstract class Item {
        private final int itemNumber;
        private ItemType type;

        protected Item(int itemNumber, ItemType type) {
            this.type = type;
            this.itemNumber = itemNumber;
        }

        public int getItemNumber() {
            return this.itemNumber;
        }

        public ItemType getItemType() {
            return this.type;
        }

        public abstract int getPrice();
    }

    private List<Item> items = new ArrayList<>();

    private Menu() {}

    public static Menu parse(Scanner in) {
        for (Menu menu = new Menu(); ; ) {
            switch (in.next()) {
              case "add":
                String typeName = in.next();
                ItemType type = ItemType.byName(typeName);
                try {
                    Item item = type.parser.apply(menu, typeName + in.nextLine());
                    menu.items.add(item);
                } catch (IllegalArgumentException badCombo) {
                    System.err.println("Error: " + badCombo.getMessage());
                }
                break;
              case "end":
                in.nextLine();      // Consume the newline after "end"
                return menu;
              default:
                System.err.println("Error: unknown command");
            }
        }
    }

    public int getItemCount() {
        return this.items.size();
    }

    public Item getItem(int i) throws IllegalArgumentException {
        try {
            return this.items.get(i);
        } catch (IndexOutOfBoundsException e) {
            throw new IllegalArgumentException();
        }
    }

    public String toString() {
        List<Item> items = new ArrayList<>(this.items);
        items.sort(Comparator.comparing(Item::getItemType));
        return items.stream()
                    .map(Item::toString)
                    .collect(Collectors.joining(System.lineSeparator()));
    }
}

SingleItem.java

Your Burger, Drink, and Snack classes are nearly identical. One class for all of them suffices.

import java.util.Scanner;

public class SingleItem extends Menu.Item {
    public static Menu.Item parse(Menu menu, String line) {
        Scanner s = new Scanner(line);
        return new SingleItem(menu.getItemCount(), Menu.ItemType.byName(s.next()), s.next(), s.nextInt());
    }

    private final String name;
    private final int price;

    public SingleItem(int itemNumber, Menu.ItemType type, String name, int price) {
        super(itemNumber, type);
        this.name = name;
        this.price = price;
    }

    public String getName() {
        return this.name;
    }

    @Override
    public int getPrice() {
        return this.price;
    }

    public String toString() {
        return String.format(
            "#%d %s: %s (%d)", this.getItemNumber(), this.getItemType(), this.getName(), this.getPrice()
        );
    }
}

ComboItem.java

As mentioned above, combos need to have a base class in common with Food. Otherwise, you can't reuse code to handle both, even if you have similarly named methods (such as Food.getOrderNum() and Combo.getOrderNum()).

I'll mention here that the parsing code in your main():

ArrayList<Food> trf = new ArrayList<>();
trf.add(foods.get(c1));
trf.add(foods.get(c2));
trf.add(foods.get(c3));

Combo tempCombo = new Combo(trf, counter);

… would be more succinctly written as Combo combo = new Combo(Arrays.asList(c1, c2, c3), counter). Of course, that means that your Combo(ArrayList<Food>, int) constructor needs to be Combo(List<Food>, int) instead — it's better practice anyway, not to insist on a particular implementation of List. I also recommend dropping the meaningless temp… prefixes in variable names throughout your code.

System.out.printf() would be more readable than System.out.println(… + … + … + … + …).

import java.util.ArrayList;
import java.util.List;
import java.util.Scanner;
import java.util.stream.Collectors;

public class ComboItem extends Menu.Item {
    public static Menu.Item parse(Menu menu, String line) {
        Scanner s = new Scanner(line = line.replaceFirst("^Combo +", ""));
        try {
            ComboItem combo = new ComboItem(menu.getItemCount());
            // Items must be listed in ascending order
            for (int i = -1, j; s.hasNextInt(); i = j) {
                j = s.nextInt();
                if (j <= i) throw new IllegalArgumentException();
                combo.include(menu, j);
            }
            return combo;
        } catch (IllegalArgumentException e) {
            throw new IllegalArgumentException("Invalid combo input: " + line);
        }
    }

    private final List<Menu.Item> subItems = new ArrayList<>();

    public ComboItem(int itemNumber) {
        super(itemNumber, Menu.ItemType.Combo);
    }

    public void include(Menu menu, int itemNumber) throws IllegalArgumentException {
        this.subItems.add(menu.getItem(itemNumber));
    }

    @Override
    public int getPrice() {
        return -50 + this.subItems.stream().mapToInt(Menu.Item::getPrice).sum();
    }

    public String toString() {
        return String.format(
            "#%d %s: (%d)%n", this.getItemNumber(), this.getItemType(), this.getPrice()
        ) +
        this.subItems.stream()
            .map(item -> "    " + item.toString())
            .collect(Collectors.joining(System.lineSeparator()));
    }
}

Order.java

This class is pretty straightforward, and follows naturally from the principles stated above.

import java.util.ArrayList;
import java.util.List;
import java.util.Scanner;
import java.util.stream.Collectors;

public class Order {
    public static Order parse(Menu menu, String line) {
        Order order = new Order();
        for (Scanner in = new Scanner(line); in.hasNextInt(); ) {
            order.add(menu.getItem(in.nextInt()));
        }
        return order;
    }

    private List<Menu.Item> items = new ArrayList<>();

    public void add(Menu.Item item) {
        this.items.add(item);
    }

    public int getPrice() {
        return this.items.stream().mapToInt(Menu.Item::getPrice).sum();
    }

    public String toString() {
        return String.format(
            "--- Order ---%n%s%nTotal: %d",
            this.items.stream.map(item).collect(Collectors.joining(System.lineSeparator())),
            this.getPrice()
        );
    }
}
\$\endgroup\$

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

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