Designing a coffee machine

Question

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;
        }

    }

Answer 1

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.

Answer 2

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).

Answer 3

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);
        }
    }

}

Answer 4

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.

Answer 5

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!