38
\$\begingroup\$

I was recently rejected from what looked like a really promising string of interviews. I did very well in a questionnaire style review, and then they handed me this assignment (more or less):

Design a coffee machine which makes different beverages based on set ingredients. The initialization of the recipes for each drink should be hard-coded, although it should be relatively easy to add new drinks. The machine should display the ingredient stock (+cost) and menu upon startup, and after every piece of valid user input. Drink cost is determined by the combination of ingredients. For example, Coffee is 3 units of coffee (75 cents per), 1 unit of sugar (25 cents per), 1 unit of cream (25 cents per). Ingredients and Menu items should be printed in alphabetical order. If the drink is out of stock, it should print accordingly. If the drink is in stock, it should print "Dispensing: ". To select a drink, the user should input a relevant number. If they submit "r" or "R" the ingredients should restock, and "q" or "Q" should quit. Blank lines should be ignored, and invalid input should print an invalid input message.

They supplied the default ingredients (&stock @10) and drinks/recipes.

They told me to "not try and impress", don't overly document (specifically commenting is not required). The code should be scaleable, readable, and just not overly complicated.

Anyway, it's more likely that I made some major error in data structure choice or handling drink selections, than me missing some detail. Still, I can privately send the full assignment description if anyone is curious...I just don't want to put it out there since it's a big company currently hiring. I changed the name of some stuff in my code to make the exact assignment less searchable. I actually have a lot of opinions on things I would change in this code, but I have a hard time believing that they are things that would get me rejected. It must be something bigger...

(I have since been informed that the team was hoping for "A more OO design"...felt like I took it in that direction, personally.)

DrinkMachine

import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStreamReader;
import java.util.Collections;
import java.util.List;
import java.util.ArrayList;
import java.util.Map;

public class DrinkMachine{

    private static List<Drink> drinkList = new ArrayList<Drink>();
    private static List<Ingredient> ingredientList = new ArrayList<Ingredient>();

    public static void main(String[] args) {
        addAllIngredients();
        addAllDrinks();
        updateCosts();
        updateMakeable();
        display();
        startIO();
    }

    public static void startIO(){
        BufferedReader reader = new BufferedReader(new InputStreamReader(System.in));
        String input = "";
        //running loop
        while(true){            
            try {
                /*I considered using a switch here for more readable code. Elected to have a dynamic drink menu so drinks can be added
                 * to addAllDrinks() and this loop would not have to be change. The decision to have slightly less readable code
                 * in favor of a dynamic menu was made upon my belief that easily modifiable code is incredibly important.
                 */
                input = reader.readLine().toLowerCase();

                if(input.equals("")){
                    continue;
                }else if (input.equals("q")){ 
                    System.exit(0); 
                }else if(input.equals("r")){ 
                    restockIngredients();
                    updateMakeable();
                }else if(Integer.parseInt(input) > 0 && Integer.parseInt(input) <= drinkList.size()){ //dynamic drink menu selection
                    makeDrink(drinkList.get(Integer.parseInt(input)-1));
                }else{
                    throw new IOException();//legal, but invalid input
                }
            } catch (Exception e) {
                System.out.println("Invalid selection: " + input + "\n");//illegal input
            }
        }//running loop     
    }

    public static void display(){
        //The sample IO in the assignment appears to add a blank line after output, so I included that.
        System.out.println("Inventory:\n");
        for (Ingredient i : ingredientList){
            System.out.println(i.getName() + "," + i.getStock() + "\n");
        }

        System.out.println("Menu:\n");
        int count = 1;
        for (Drink d : drinkList){
            System.out.printf("%d,%s,$%.2f," + d.getMakeable() + "\n\n", count, d.getName(), d.getCost());
            count++;
        }
    }

    public static void updateMakeable(){
        for (Drink d : drinkList){
            Map<String, Integer> currRecipe = d.getRecipe();
            for (Ingredient i : ingredientList){
                if (currRecipe.containsKey(i.getName()) && i.getStock() < currRecipe.get(i.getName())){
                    d.setMakeable(false);
                    break;//check next drink
                }
                d.setMakeable(true);
            }//Ingredient loop
        }//Drink loop
    }

    public static void updateCosts(){
        for (Drink d : drinkList){
            double currCost = 0;
            Map<String, Integer> currRecipe = d.getRecipe();
            for (Ingredient i : ingredientList){
                if (currRecipe.containsKey(i.getName())){
                    currCost += i.getCost()*currRecipe.get(i.getName());
                }
            }//inner
            d.setCost(currCost);
        }//outer
    }

    public static void makeDrink(Drink drink){
        if(drink.getMakeable()){
            System.out.println("Dispensing: " + drink.getName() + "\n");
            for (Ingredient i : ingredientList){
                if(drink.getRecipe().containsKey(i.getName())){
                    i.setStock(i.getStock()-drink.getRecipe().get(i.getName()));
                }
            }
        }else{
            System.out.println("Out of stock: " + drink.getName() + "\n");
        }
        updateMakeable();
        display();
    }

    public static void restockIngredients(){
        for(Ingredient i : ingredientList){
            i.setStock(10);
        }
        updateMakeable();
        display();
    }

    //Add ingredients through addAllIngredients
    public static void addIngredient(Ingredient ingredient){
        ingredientList.add(ingredient);
    }
    //Add drinks through addAllDrinks
    public static void addDrink(String name, String[] recipe){
        drinkList.add(new Drink(name, recipe));
    }

    public static void addAllIngredients(){
        addIngredient(new Ingredient("Coffee", 0.75));
        addIngredient(new Ingredient("Decaf Coffee", 0.75));
        addIngredient(new Ingredient("Sugar", 0.25));
        addIngredient(new Ingredient("Cream", 0.25));
        addIngredient(new Ingredient("Steamed Milk", 0.35));
        addIngredient(new Ingredient("Foamed Milk", 0.35));
        addIngredient(new Ingredient("Espresso", 1.10));
        addIngredient(new Ingredient("Cocoa", 0.90));
        addIngredient(new Ingredient("Whipped Cream", 1.00));   

        Collections.sort(ingredientList);
    }

    public static void addAllDrinks(){  
        addDrink("Coffee", new String[]{"Coffee", "Coffee", "Coffee", "Sugar", "Cream"});
        addDrink("Decaf Coffee", new String[]{"Decaf Coffee", "Decaf Coffee", "Decaf Coffee", "Sugar", "Cream"});
        addDrink("Caffe Latte", new String[]{"Espresso", "Espresso", "Steamed Milk"});
        addDrink("Caffe Americano", new String[]{"Espresso", "Espresso", "Espresso"});
        addDrink("Caffe Mocha", new String[]{"Espresso", "Cocoa", "Steamed Milk", "Whipped Cream"});
        addDrink("Cappuccino", new String[]{"Espresso", "Espresso", "Steamed Milk", "Foamed Milk"});

        Collections.sort(drinkList);
    }

}

Drink

import java.util.HashMap;
import java.util.Map;

public class Drink implements Comparable<Drink>{
    private Map<String, Integer> recipe = new HashMap<String, Integer>();//map ingredients to units per
    private String name;
    private double totalCost = 0;
    private boolean makeable = false;

    public Drink(String name, String[] recipe){
        this.name = name;
        setRecipe(recipe);
    }

    public int compareTo(Drink drink){
        return name.compareTo(drink.getName());
    }

    public void setRecipe(String[] recipe){
        for(String s : recipe){
            if(this.recipe.containsKey(s)){
                this.recipe.put(s, this.recipe.get(s)+1);//increment if multiple units
            }else{
                this.recipe.put(s, 1);//insert first occurrence of ingredient
            }
        }
    }

    public void setName(String name){
        this.name = name;
    }

    public void setCost(double totalCost){
        this.totalCost = totalCost;
    }

    public void setMakeable(boolean makeable){
        this.makeable = makeable;
    }

    public Map<String, Integer> getRecipe(){
        return recipe;
    }

    public double getCost(){
        return totalCost;
    }

    public String getName(){
        return name;
    }

    public boolean getMakeable(){
        return makeable;
    }

}

Ingredient

    public class Ingredient implements Comparable<Ingredient>{
        private String name = "";
        private double cost = 0.00;
        private int stock = 0;

        public Ingredient(String name, double cost){
            this.name = name;
            this.cost = cost;
            this.stock = 10;
        }

        public int compareTo(Ingredient ingredient) {
            return name.compareTo(ingredient.getName());
        }

        public void setName(String name){
            this.name = name;
        }

        public void setCost(double cost){
            this.cost = cost;
        }

        public void setStock(int stock){
            this.stock = stock;
        }

        public String getName(){
            return name;
        }

        public double getCost(){
            return cost;
        }

        public int getStock(){
            return stock;
        }

    }
\$\endgroup\$
  • \$\begingroup\$ Without even looking at the implementation if I was reviewing this code for an interview the all the static members and static void methods would really scare me. Most static states (mostly mutable ones) make testing hard and hurts extensibility. \$\endgroup\$ – pllee Mar 4 '15 at 20:04
  • 2
    \$\begingroup\$ Also, why are the addAll* methods public? They're only used once at initialization. As a general advice: OOP dictates that you need to know as little as possible about the implementation of a class. There should not be a public update* method (if at all), since one should expect an object of a class to behave properly and figure out by itself when it needs updating. It seems that you're seeing OOP as 'let's split my program into useful subroutines' instead of 'I want to use my classes without knowing their exact implementation'. \$\endgroup\$ – Sanchises Mar 4 '15 at 22:49
31
\$\begingroup\$

I have since been informed that the team was hoping for "A more OO design"...felt like I took it in that direction, personally.

All the methods in your DrinkMachine class are static. I believe the ability to have more than one DrinkMachine is a very important feature. DrinkMachines should be able to have different inventories, different products, etc. Now, you might think "But I was told to keep it simple!". This is not about "trying to impress" or anything, this is about reasonable functionality.

I am afraid that this staticness of the DrinkMachine cost you a lot of "interview-points".


Do you really need to be able to change the name of an Ingredient after it has been created?

I think you should strive on making some of your fields final. This also helps with tim's point about you don't need to initialize fields if you assign them in a constructor, that's just confusing. - final fields can only be initialized once.

\$\endgroup\$
  • 3
    \$\begingroup\$ I think you're right about how the ability to have more than one DrinkMachine is an important feature, even if it is not a specified requirement. Regarding making some fields final...I suppose I need to think harder about whether or not fields should change. I think I often say "Why not" or "Doesn't hurt" regarding that...in other words, I decided not to make them final because I couldn't think of a reason why they shouldn't be able to change. Instead, I suppose it's better to think "Is there a reason why it ever should change?" and declare it final if I can't think of one. \$\endgroup\$ – spacecadet Mar 4 '15 at 8:57
  • 3
    \$\begingroup\$ @spacecadet : this subject is covered by keyword like mutable/immutable. As too many programmers, you have been teached to build automatic mutable objects, which is sad. Try to learn about Immutable advantages : programmers.stackexchange.com/questions/151733/… \$\endgroup\$ – Cyril Gandon Mar 5 '15 at 13:24
  • \$\begingroup\$ @CyrilGandon This is tricky because I know I overdid it by making everything changeable, but when it comes to mutability in general, a lot of my feelings are shared w/ this: programmers.stackexchange.com/a/151786/170331 I ask "Why shouldn't it change?" because mutability is default. If the question should instead be "Why should it change?" then we would want things to be final and immutable by default. Anyway I'm not the one to decide if this school of thought is right/wrong, but it's why I (and apparently others) think mutability until there is an explicit reason for it not to be. \$\endgroup\$ – spacecadet Mar 5 '15 at 23:46
8
\$\begingroup\$

Sorry to hear that you didn't get the job. I'll keep my fingers crossed for you for your next attempts.

It would probably be very hard to pin-point one exact detail that could have saved this code. It is very possible that their response that they were hoping for a "more OO design" was just some vague thing that they came up with to justify their decision. Since it is very hard to know how other candidates performed in the other parts of the interview, it is not even certain that this code was the main reason that they chose someone else.

As these coding exercises are a way for the company to gauge your familiarity and experience with a specific language or with coding in general, it was probably a whole collection of little things that made you seem less experienced than a competing applicant. @Tim already compiled a good list of these sorts of things. But if they were specifically looking for a Java developer, let me add some more observations (not all of which everyone would necessarily care about or even agree with):

  • You use ArrayLists for your drink and ingredient lists. Since each of these lists should not support two identical entries, the more sensible choice would be to use a Set instead of a List. Specifically, using a TreeSet will always keep its entries sorted, so you would not even need to use Collections.sort(...) any more. Someone might assume that you are not familiar with these "advanced" data-structure types if you are not using them.

  • Since you are using the String input only inside of the try {}-block, you can just define it right there (String input = reader...) rather than defining it outside and initializing it to empty String even though that initialized value is never used.

  • You throw an IOException for a wrong input, which is usually understood to relate to I/O errors when it comes to things like reading/writing files or network streams. A more sensible exception would be UnsupportedOperationException or IllegalArgumentException. In general, though, it is extremely expensive to throw and catch exceptions. So the much better approach would be to move your "Invalid selection"-println into the else-block instead of throwing an exception.

  • It is generally considered bad practice to catch generic Exceptions rather than specific ones, especially if you then don't output the exception! For example, if someone modifies your makeDrink(...)-code and makes a mistake that leads to a NullPointerException or something else being thrown in that code, you catch this exception and turn it into the "Invalid selection"-message, which will make it much harder to track down that bug.

  • If a for-statement or if-statement only has a single line following it, you can omit the curly braces (e.g. for(Ingredient i : ingredientList) i.setStock(10);).

  • Java support varargs which allow you to write public static void addDrink(String name, String... recipe) and have it behave exactly as String[] would inside the function, except that you can now call it as addDrink("Coffee", "Coffee", "Coffee", "Coffee", "Sugar", "Cream"). While this seems less awkward than having to add new String[]{}, it does make the code a little less clear as the first "Coffee" in the list gets special treatment but looks like part of the list in the call. So this one is definitely a judgement call.

  • ...

There are probably several other things that they (or others) might consider indications of mastery of a language (or lack thereof), like using the ternary operation and enums (as @200_success did in his answer), maybe even generics if you can find a way to have it make sense in this simple example, ... So, even though they specifically said to "not try and impress", there are definitely certain things that people who live and breathe Java to the point that they are dreaming about it at night would just instinctively do or use to write concise and clear code without the intention to "impress". What exactly those things are depends a lot on the individual person (you can see hints of this in the comments to answers here, although there are definitely some conventions), so it's hard to say what rubbed them the wrong way for your submission (or at least a better way for someone else's).

With that in mind, while it will be useful for you to see examples of how this code would have been written by others (@200_success's is a great one!), the only way to truly improve your chances for the same kind of interview question next time is to code, code, code! Or for general inspiration or if you are short on ideas for own projects, you could read over and try to understand code written by others for renowned open-source projects. Large parts of OpenOffice are written in Java, for example. Do realize, though, that not all code written for any of these projects would be considered a stellar example of how to do it right. But it should still give you a great way to find out about language constructs you maybe didn't know about (a ? b : c, for example) and other available data structures. So you can make yourself a lot more resourceful simply by trying to understand every last (content and syntactical) detail of what you read there...

Good luck!

\$\endgroup\$
  • 2
    \$\begingroup\$ I generally agree with your answer here, especially the TreeSet<T> suggestion (I haven't considered that in my own attempt codereview.stackexchange.com/questions/83195/…). I do not agree though with that it is better to omit curly braces, I would say: Never omit them, they can cause nasty bugs. \$\endgroup\$ – skiwi Mar 5 '15 at 9:54
  • \$\begingroup\$ @skiwi True. Leaving them out can make the code more bug-prone. The reasen I added it to the list is that to me, using them in such simple contexts instead of the one-liner I put looks like awkward CS-class-style stick-to-the-rules coding rather than years of in-field experience. But this is a highly subjective interpretation and as far as good coding style goes, you are certainly correct. To me, it's kinda like using "kinda" when writing English. You wouldn't do that if you'd only learned it in school because it's not "correct", but if you use English daily, eventually you might. \$\endgroup\$ – Markus A. Mar 5 '15 at 15:21
  • \$\begingroup\$ Thanks @MarkusA. , I've taken a lot of notes based on your suggestions. A few questions/remarks.. re: dropping curly braces. I find myself often wanting to, but electing to keep them for the sake of readability and, you guessed it, how my professors taught me to keep it. A set / treeset specifically is something that actually clicked once I had submitted it. I'm familiar with a lot of these data-structures...I just don't use them enough. Need more diverse projects! Is there any set context where varargs are considered best practice? Or just personal pref? How about break & continue? \$\endgroup\$ – spacecadet Mar 5 '15 at 23:16
  • \$\begingroup\$ There's nothing wrong with using break and continue. Using break inside the updateMakeable() loop was correct. Changing it to if-else, without a break, would be a logic bug. \$\endgroup\$ – 200_success Mar 7 '15 at 8:29
  • 1
    \$\begingroup\$ @spacecadet Actually, I made a mistake: I read your break as a continue (rookie-mistake, I guess ;). I removed that point from my answer. I guess what caught my attention in your updateMakeable()-method was the fact that you repeatedly call d.setMakeable(true) inside the inner for-loop and I mistakenly blamed it onto your use of break. I would move that statement before the ingredients-loop so it only gets called once. \$\endgroup\$ – Markus A. Mar 7 '15 at 16:04
10
\$\begingroup\$

I believe your interviewer was looking for the usage of the decorator pattern. Atleast, I've seen almost the exact same problem description in a design pattern book I read. It's a very good read and they deserve credit!

The decorator pattern allows you to create compositions dynamically, starting from a normal coffee, and adding decoration layers (milk, sugar, etc.) on top of it.

Calling the cost function on the highest layer, will then calculate the cost of the highest layer and calls the cost function of the layer underneath, adding both together.

A short sketch:

base = new Drink(); 
DrinkInterface topping1 = new Topping(base);
DrinkInterface topping2 = new Topping(topping1);

I believe it would be pointless to give you the full solution, retrying it with the pattern would be a great exercise!

For a more in depth explanation of the decorator pattern (coffeemaker scenario), take a look at this.

\$\endgroup\$
  • 3
    \$\begingroup\$ Personally, I think avoiding this pattern is what the interviewers meant with not try and impress. \$\endgroup\$ – Simon Forsberg Mar 4 '15 at 13:15
  • 4
    \$\begingroup\$ I just don't see the reason for it. Keeping a list of all ingredients seems simpler to me. Using the pattern that you suggest will end up with quite many classes in this case. \$\endgroup\$ – Simon Forsberg Mar 4 '15 at 15:20
  • 2
    \$\begingroup\$ Keeping logic separated? Since one should develop with the idea that additional logic is needed. If you want to add a new decorator, all you have to do is create a class that implements the DrinkInterface and it scales perfectly, without having to alter any other existing classes (except the client ofcourse). Also Imagine if they in the future want to add a make() method, which can be different for each topping (milk/sugar/mocca). I'm not saying the pattern is a necessity or golden bullet. I however do believe it's a good idea to make it future proof and it's easier for communication purposes. \$\endgroup\$ – DJanssens Mar 4 '15 at 15:35
  • 3
    \$\begingroup\$ One small reason I would shy away from this approach is that the decorator pattern implies an order: it doesn't matter if you add milk then sugar vs sugar then milk. It's also more cumbersome to interact with the drink directly, given that it's buried under layers of reference. \$\endgroup\$ – skeggse Mar 4 '15 at 22:55
  • 1
    \$\begingroup\$ Yes - As DJanssens suggested its the Decorator pattern that is the key to this problem...Funnily enough Wikipedia as an example "Coffee Making Scenario" that might have actually been the inspiration for the assignment :-) : See en.wikipedia.org/wiki/… \$\endgroup\$ – AdamW Mar 5 '15 at 12:36
18
\$\begingroup\$

I agree with their assessment that the OOP design could be improved. In particular, the way you initialize the data is cumbersome, and I object to the necessity of calling updateMakeable() throughout your DrinkMachine code.

Ingredient.java

What makes an ingredient comparable to another? Is the comparison by name, cost, or quantity?

Having the stock be a part of an Ingredient doesn't feel right, as it mixes in part of the DrinkMachine's state. In my opinion, an Ingredient should be stateless and immutable. setName() makes no sense. setCost() is probably over-engineering for this exercise.

Since the exercise suggests that hard-coding the ingredients is OK, I would just use an enum whose entries are in alphabetical order.

import java.math.BigDecimal;

public enum Ingredient {
    // An extensible design wouldn't use an enum, but the problem specification
    // allows such hard-coding.  These entries must be in alphabetical order.
    COCOA("0.90"),
    COFFEE("0.75"),
    CREAM("0.25"),
    DECAF_COFFEE("0.75"),
    ESPRESSO("1.10"),
    FOAMED_MILK("0.35"),
    STEAMED_MILK("0.35"),
    SUGAR("0.25"),
    WHIPPED_CREAM("1.00");

    private final String name;
    private final BigDecimal cost;

    Ingredient(String cost) {
        this.name = this.name().replace("_", " ").toLowerCase();
        this.cost = new BigDecimal(cost);
    }

    // Using fixed-point representation to prevent rounding problems
    public BigDecimal getCost() {
        return this.cost;
    }

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

Drink.java

Many of the same remarks for Ingredient apply to Drink as well. Drinks aren't inherently Comparable. You don't have to plan for product reformulation (setRecipe()), rebranding (setName()) or adjusting the profit margins (setCost()). Makability is part of the DrinkMachine's state. (If you were to have such a method, isMakable() would be a more appropriate name than getMakeable(), both because of the spelling and because of the Java naming convention for predicates.)

In getRecipe(), it would be a good idea to return an unmodifiable map.

Once again, I would go with an enum, which takes care of initialization and sorting.

import java.math.BigDecimal;
import java.util.Collections;
import java.util.HashMap;
import java.util.Map;

public enum Drink {
    // An extensible design wouldn't use an enum, but the problem specification
    // allows such hard-coding.  These entries must be in alphabetical order.
    CAFFE_AMERICANO("Caffè Americano", "3 ESPRESSO"),
    CAFFE_LATTE("Caffè Latte", "2 ESPRESSO", "STEAMED_MILK"),
    CAFFE_MOCHA("Caffè Mocha", "ESPRESSO", "COCOA", "STEAMED_MILK", "WHIPPED_CREAM"),
    CAPPUCCINO("Cappuccino", "2 ESPRESSO", "STEAMED_MILK", "FOAMED_MILK"),
    COFFEE("Coffee", "3 COFFEE", "SUGAR", "CREAM"),
    DECAF_COFFEE("Decaf Coffee", "3 DECAF_COFFEE", "SUGAR", "CREAM");

    private final String name;
    private final Map<Ingredient, Integer> ingredients;
    private final BigDecimal cost;

    Drink(String name, String... recipe) {
        Map<Ingredient, Integer> map = new HashMap<Ingredient, Integer>();
        BigDecimal cost = BigDecimal.ZERO;
        for (String spec : recipe) {
            String[] amountOfStuff = spec.split(" ", 2);
            int quantity = (amountOfStuff.length > 1) ? Integer.parseInt(amountOfStuff[0]) : 1;
            String stuff = amountOfStuff[amountOfStuff.length - 1];
            Ingredient ingredient = Enum.valueOf(Ingredient.class, stuff);

            map.put(ingredient, quantity);
            cost = cost.add(ingredient.getCost().multiply(new BigDecimal(quantity)));
        }
        this.name = name;
        this.ingredients = Collections.unmodifiableMap(map);
        this.cost = cost;
    }

    public Map<Ingredient, Integer> getRecipe() {
        return this.ingredients;
    }

    public BigDecimal getPrice() {
        return this.cost;
    }

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

DrinkMachine.java

The improvements above will pay off here:

  • addAllIngredients() and addAllDrinks() are taken care of by the enums.
  • updateCosts() is taken care of by the Drink constructor.
  • updateMakeable() is no longer needed, since makability is no longer a property of each drink. Rather, the machine can query itself to see whether each drink can be made. (See canMake().) Eliminating updateMakeable() is a good thing — needing to call it everywhere is error-prone. Note that in your handling of the "r" command, you call restockIngredients(); followed by updateMakeable(); but restockIngredients() itself already calls updateMakeable().

The DrinkMachine could benefit from having a toString() method. It can take over your display() method.

Scanner provides a slightly nicer interface than BufferedReader. In this case, since all you need is readLine(), Console also works nicely.

Now that initialization has already been taken care of, your event loop can just be in main(). In the loop, calling System.exit(0) is unjustified — just a break would suffice. Catching all Exceptions is also too strong. For example, end-of-file (Ctrl z on Windows, Ctrl d elsewhere) would fail to cause the program to exit the way I expect. I also find it odd that you classify legal but invalid input as an IOException rather than IllegalArgumentException.

import java.io.Console;
import java.util.*;

public class DrinkMachine {
    public static class OutOfStockException extends RuntimeException {
        OutOfStockException(Drink drink) {
            super(drink.toString());
        }
    }

    private Map<Ingredient, Integer> stock = new HashMap<Ingredient, Integer>();

    public int getStock(Ingredient ingredient) {
        return this.stock.containsKey(ingredient) ?
               this.stock.get(ingredient) : 0;
    }

    public void restock() {
        for (Ingredient ingredient : Ingredient.values()) {
            if (this.getStock(ingredient) < 10) {
                this.stock.put(ingredient, 10);
            }
        }
    }

    public Drink[] menu() {
        return Drink.values();
    }

    public boolean canMake(Drink drink) {
        for (Map.Entry<Ingredient, Integer> quantStuff : drink.getRecipe().entrySet()) {
            if (this.getStock(quantStuff.getKey()) < quantStuff.getValue()) {
                return false;
            }
        }
        return true;
    }

    public void make(Drink drink) throws OutOfStockException {
        if (!this.canMake(drink)) {
            throw new OutOfStockException(drink);
        }
        for (Map.Entry<Ingredient, Integer> quantStuff : drink.getRecipe().entrySet()) {
            int quant = quantStuff.getValue();
            Ingredient stuff = quantStuff.getKey();
            this.stock.put(stuff, this.getStock(stuff) - quant);
        }
    }

    public String toString() {
        StringBuilder out = new StringBuilder("Inventory:\n");
        for (Ingredient ingredient : Ingredient.values()) {
            out.append(ingredient).append(": ")
               .append(this.getStock(ingredient)).append('\n');
        }
        out.append("\nMenu:\n");
        int i = 0;
        for (Drink drink : Drink.values()) {
            out.append(++i).append(": ").append(drink);
            if (this.canMake(drink)) {
                out.append(", $").append(drink.getPrice());
            } else {
                out.append(" (out of stock)");
            }
            out.append('\n');
        }
        return out.toString();
    }

    private static String prompt(Console con, String prompt) {
        con.printf("%s", prompt);
        return con.readLine();
    }

    public static void main(String[] args) {
        DrinkMachine machine = new DrinkMachine();
        machine.restock();

        Console con = System.console();
        con.printf("%s\n", machine);

        String origCmd;
        while ((origCmd = prompt(con, "Select: ")) != null) {
            String cmd = origCmd.toLowerCase();

            if (cmd.isEmpty()) {
                continue;
            } else if ("q".equals(cmd)) {
                break;
            } else if ("r".equals(cmd)) {
                machine.restock();
            } else try {
                int selection = Integer.parseInt(cmd);
                Drink order = machine.menu()[selection - 1];
                machine.make(order);
                con.printf("Dispensing: %s\n", order);
            } catch (ArrayIndexOutOfBoundsException | NumberFormatException invalid) {
                con.printf("Invalid input: %s\n", origCmd);
                continue;
            } catch (OutOfStockException outOfStock) {
                con.printf("Out of stock: %s\n", outOfStock.getMessage());
                continue;
            }
            con.printf("%s\n", machine);
        }
    }

}
\$\endgroup\$
  • \$\begingroup\$ I really like your enum-approach and I find your code very readable overall (+1). Just one quick question: Is there a reason you are using a BigDecimal for the price rather than a simple double or float or even an int in cents if you're worried about rounding? Reason I'm asking is because @skiwi copied your idea codereview.stackexchange.com/questions/83195/… and it might be helpful to justify or question the use of a very involved object like a BigDecimal for a very simple purpose like this. \$\endgroup\$ – Markus A. Mar 4 '15 at 21:44
  • 1
    \$\begingroup\$ @MarkusA. This application is no performance-sensitive. BigDecimal results in simpler code than using an int number of cents. float and double are unable to represent hundredths exactly. BigDecimal is immune to rounding errors. They carry precision information, so that new BigDecimal("1.00") will naturally print as "1.00" rather than "1". Unlike strings, you can do arithmetic on them, which is necessary to arrive at the total cost. For all those reasons, I chose BigDecimal. \$\endgroup\$ – 200_success Mar 4 '15 at 23:03
  • \$\begingroup\$ I didn't know that they preserve their precision when printing. That's indeed helpful. The imprecision that float and double introduce into the numbers, though, are so extremely tiny that you'd have to order tens of thousands of drinks on the same check before you see it for floats. For doubles it's many trillions. Should be safe. :) \$\endgroup\$ – Markus A. Mar 5 '15 at 3:59
23
\$\begingroup\$

First of all, I'm sorry that you didn't get the job. Better luck next time :)

Overall, I don't think your code is bad. But I do have a couple of points:

OOP

  • My main complaint would be that you are missing a recipe class. Having it as a string array/map is not very flexible. Because of this, you also expose the inner workings of the drink class to the outside world (for example in updateMakeable).
  • setRecipe is a bit confusing. Generally, setters only set things, but this method does more. It also doesn't fit in with getRecipe which returns a different type than setRecipe sets.
  • Your DrinkMachine does too much. It displays, it reads input, it handles the state and logic of the machine, and it contains the test cases and the main loop. At least the first two and the last two case should be handled somewhere else.

Misc

  • you don't need to initialize fields if you assign them in a constructor, that's just confusing.
  • having the stock of ingredients hardcoded in the constructor doesn't seem very flexible.
  • add @Override if appropriate (may avoid bugs, and increases readability).
  • having comments which show where a loop ends is not very common in Java (if your code is so nested that it needs them, it should be refactored).
  • many of your comments are actually not needed, or should be better integrated into the code (eg legal, but invalid input shouldn't be a comment, but the message of the exception, //running loop isn't very helpful, etc).
  • short variable names lead to harder to read code (drink, ingredient, currentCost are all not too long, and a lot more readable than d, i, and currCost).
\$\endgroup\$
  • 1
    \$\begingroup\$ Thanks, lots of good stuff here. A recipe class seems like the big thing. I might have taken some of their tips too literally (simplicity, don't try and impress, etc). I can recall thinking about splitting the code up more, but telling myself that it would go against the simplicity to have too much code in too many different places....I guess that wound up having the opposite effect. Thanks again. \$\endgroup\$ – spacecadet Mar 3 '15 at 22:43
  • 3
    \$\begingroup\$ I don't see a need to introduce a Recipe class. After all, what is a Drink, but a name and an ingredient list? The problem is that Drink shouldn't carry coffee machine state such as makability, and Ingredient shouldn't carry coffee machine state such as inventory level. \$\endgroup\$ – 200_success Mar 4 '15 at 12:07

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.