19
\$\begingroup\$

After reading Designing a coffee machine yesterday, I decided I could also give the same problem a try.

I have used the following problem statement (copied from the given question):

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.

I may have interpreted some points clearly and a personal remark on the assignment is that the action of restocking is quite abstract and having a single maximum for every ingredient is unrealistic. I still have implemented it that way because "They supplied the default ingredients (&stock @10) and drinks/recipes."

In my decision I have attempted to abstract everything as much as possible, a few points I am aware of:

  • My CoffeeMachineDisplay#processAndContinue method is not really clean, ideally it should be refactored, but I'm unsure how.
  • I have avoided using enums for the ingredients as that limits the applicability of this application in real software, you may want to dynamically load the ingredients and drinks from a database for example.
  • I have kept an eye on obvious performance improvements, though have not wasted too much time and/or code on premature optimization in my eyes.
  • I have used defensive programming everywhere, because users are evil.

public class Searcher<K, T> {
    private final Map<K, T> cache;

    public Searcher(final Function<? super T, ? extends K> keyExtractor, final Collection<? extends T> items) {
        this.cache = ((Collection<T>)items).stream()
            .collect(Collectors.toMap(keyExtractor, item -> item));
    }

    public final T search(final K key) {
        T item = cache.get(key);
        if (item == null) {
            throw new IllegalArgumentException("No item has been found for key " + key);
        }
        return item;
    }
}

public class IngredientSearcher extends Searcher<String, Ingredient> {
    public IngredientSearcher(final Collection<? extends Ingredient> ingredients) {
        super(Ingredient::getName, ingredients);
    }
}

public class DrinkSearcher extends Searcher<String, Drink> {
    public DrinkSearcher(final Collection<? extends Drink> drinks) {
        super(Drink::getName, drinks);
    }
}

public class Ingredient {
    private final String name;
    private final BigDecimal cost;

    public Ingredient(final String name, final BigDecimal cost) {
        this.name = Objects.requireNonNull(name, "name");
        this.cost = Objects.requireNonNull(cost, "cost");
    }

    public String getName() {
        return name;
    }

    public BigDecimal getCost() {
        return cost;
    }

    @Override
    public boolean equals(final Object obj) {
        if (this == obj) return true;
        if (obj == null || getClass() != obj.getClass()) return false;

        Ingredient that = (Ingredient)obj;

        if (cost.compareTo(that.cost) != 0) return false;
        if (!name.equals(that.name)) return false;

        return true;
    }

    @Override
    public int hashCode() {
        int result = name.hashCode();
        result = 31 * result + cost.hashCode();
        return result;
    }

    @Override
    public String toString() {
        return "(" + name + ", " + cost + ")";
    }

    public static IngredientSearcher createSearcher(final Collection<? extends Ingredient> ingredients) {
        return new IngredientSearcher(ingredients);
    }
}

public class Drink {
    private final String name;
    private final List<Ingredient> ingredients = new ArrayList<>();

    private final BigDecimal cost;

    public Drink(final String name, final Ingredient... ingredients) {
        this(name, Arrays.asList(ingredients));
    }

    public Drink(final String name, final Collection<? extends Ingredient> ingredients) {
        this.name = Objects.requireNonNull(name, "name");
        this.ingredients.addAll(ingredients);

        this.cost = this.ingredients.stream().map(Ingredient::getCost).reduce(BigDecimal.ZERO, BigDecimal::add);
    }

    public String getName() {
        return name;
    }

    public List<Ingredient> getIngredients() {
        return new ArrayList<>(ingredients);
    }

    public BigDecimal getCost() {
        return cost;
    }

    public Map<Ingredient, Long> getIngredientCount() {
        return ingredients.stream()
            .collect(Collectors.groupingBy(ingredient -> ingredient, Collectors.counting()));
    }

    @Override
    public boolean equals(final Object obj) {
        if (this == obj) return true;
        if (obj == null || getClass() != obj.getClass()) return false;

        Drink drink = (Drink)obj;

        if (!ingredients.equals(drink.ingredients)) return false;
        if (!name.equals(drink.name)) return false;

        return true;
    }

    @Override
    public String toString() {
        return "(" + name + ", " + ingredients + ")";
    }

    @Override
    public int hashCode() {
        int result = name.hashCode();
        result = 31 * result + ingredients.hashCode();
        return result;
    }

    public static DrinkSearcher createSearcher(final Collection<? extends Drink> drinks) {
        return new DrinkSearcher(drinks);
    }
}

public final class Default {
    private Default() {
        throw new UnsupportedOperationException();
    }

    private static final List<Ingredient> DEFAULT_INGREDIENTS =
        Arrays.asList(
            new Ingredient("Coffee", new BigDecimal("0.75")),
            new Ingredient("Decaf Coffee", new BigDecimal("0.75")),
            new Ingredient("Sugar", new BigDecimal("0.25")),
            new Ingredient("Cream", new BigDecimal("0.25")),
            new Ingredient("Steamed Milk", new BigDecimal("0.35")),
            new Ingredient("Foamed Milk", new BigDecimal("0.35")),
            new Ingredient("Espresso", new BigDecimal("1.10")),
            new Ingredient("Cocoa", new BigDecimal("0.90")),
            new Ingredient("Whipped Cream", new BigDecimal("1.00"))
        );

    public static List<Ingredient> getDefaultIngredients() {
        return new ArrayList<>(DEFAULT_INGREDIENTS);
    }

    private static final List<Drink> DEFAULT_DRINKS = new ArrayList<>();
    static {
        IngredientSearcher ingredientSearcher = new IngredientSearcher(getDefaultIngredients());
        DEFAULT_DRINKS.add(new Drink("Coffee", Arrays.asList(
            ingredientSearcher.search("Coffee"),
            ingredientSearcher.search("Coffee"),
            ingredientSearcher.search("Coffee"),
            ingredientSearcher.search("Sugar"),
            ingredientSearcher.search("Cream")
        )));
        DEFAULT_DRINKS.add(new Drink("Decaf Coffee", Arrays.asList(
            ingredientSearcher.search("Decaf Coffee"),
            ingredientSearcher.search("Decaf Coffee"),
            ingredientSearcher.search("Decaf Coffee"),
            ingredientSearcher.search("Sugar"),
            ingredientSearcher.search("Cream")
        )));
        DEFAULT_DRINKS.add(new Drink("Caffe Latte", Arrays.asList(
            ingredientSearcher.search("Espresso"),
            ingredientSearcher.search("Espresso"),
            ingredientSearcher.search("Steamed Milk")
        )));
        DEFAULT_DRINKS.add(new Drink("Caffe Americano", Arrays.asList(
            ingredientSearcher.search("Espresso"),
            ingredientSearcher.search("Espresso"),
            ingredientSearcher.search("Espresso")
        )));
        DEFAULT_DRINKS.add(new Drink("Caffe Mocha", Arrays.asList(
            ingredientSearcher.search("Espresso"),
            ingredientSearcher.search("Cocoa"),
            ingredientSearcher.search("Steamed Milk"),
            ingredientSearcher.search("Whipped Cream")
        )));
        DEFAULT_DRINKS.add(new Drink("Cappuccino", Arrays.asList(
            ingredientSearcher.search("Espresso"),
            ingredientSearcher.search("Espresso"),
            ingredientSearcher.search("Steamed Milk"),
            ingredientSearcher.search("Foamed Milk")
        )));
    }

    public static List<Drink> getDefaultDrinks() {
        return new ArrayList<>(DEFAULT_DRINKS);
    }
}

public class CoffeeMachine {
    private final List<Ingredient> ingredients = new ArrayList<>();
    private final Map<Ingredient, Integer> ingredientStock = new HashMap<>();
    private final List<Drink> drinks = new ArrayList<>();

    public CoffeeMachine(final Collection<? extends Ingredient> ingredients, final Map<? extends Ingredient, Integer> ingredientStock, final Collection<? extends Drink> drinks) {
        this.ingredients.addAll(ingredients);
        this.ingredientStock.putAll(ingredientStock);
        this.drinks.addAll(drinks);

        this.ingredients.forEach(ingredient -> this.ingredientStock.putIfAbsent(ingredient, 0));
    }

    public List<Ingredient> getIngredients() {
        return new ArrayList<>(ingredients);
    }

    public List<Drink> getDrinks() {
        return new ArrayList<>(drinks);
    }

    public int getCurrentStock(final Ingredient ingredient) {
        checkIsValidIngredient(ingredient);
        return ingredientStock.get(ingredient);
    }

    public boolean isOutOfStock(final Drink drink) {
        checkIsValidDrink(drink);
        return drink.getIngredientCount().entrySet().stream()
            .anyMatch(entry -> {
                Ingredient ingredient = entry.getKey();
                long count = entry.getValue();
                return (ingredientStock.get(ingredient) < count);
            });
    }

    public void restock(final Map<? extends Ingredient, Integer> newIngredientStock) {
        this.ingredientStock.putAll(newIngredientStock);
    }

    public void makeDrink(final Drink drink) {
        checkIsValidDrink(drink);
        if (isOutOfStock(drink)) {
            throw new IllegalArgumentException("Drink " + drink + " is not in stock");
        }
        drink.getIngredients().forEach(this::decrementStock);
    }

    private void decrementStock(final Ingredient ingredient) {
        ingredientStock.compute(ingredient, (innerIngredient, stock) -> stock - 1);
    }

    private void checkIsValidIngredient(final Ingredient ingredient) {
        if (!ingredients.contains(ingredient)) {
            throw new IllegalArgumentException("Unknown ingredient: " + ingredient);
        }
    }

    private void checkIsValidDrink(final Drink drink) {
        if (!drinks.contains(drink)) {
            throw new IllegalArgumentException("Unknown drink: " + drink);
        }
    }
}

public class CoffeeMachineDisplay {
    private final CoffeeMachine coffeeMachine;
    private final Scanner scanner = new Scanner(System.in);

    private final List<Ingredient> indexedIngredients;
    private final List<Drink> indexedDrinks;

    public CoffeeMachineDisplay(final CoffeeMachine coffeeMachine) {
        this.coffeeMachine = Objects.requireNonNull(coffeeMachine, "coffeeMachine");

        this.indexedIngredients = coffeeMachine.getIngredients().stream()
            .sorted(Comparator.comparing(Ingredient::getName))
            .collect(Collectors.toList());
        this.indexedDrinks = coffeeMachine.getDrinks().stream()
            .sorted(Comparator.comparing(Drink::getName))
            .collect(Collectors.toList());
    }

    public void displayCurrentStock() {
        for (Ingredient ingredient : indexedIngredients) {
            int currentStock = coffeeMachine.getCurrentStock(ingredient);
            System.out.println("Ingredient: " + ingredient.getName() + " / Stock: " + currentStock + " / Cost: " + ingredient.getCost());
        }
        System.out.println();
    }

    public void displayMenu() {
        for (int i = 0; i < indexedDrinks.size(); i++) {
            Drink drink = indexedDrinks.get(i);
            System.out.print((i + 1) + ". Drink: " + drink.getName() + " / Cost: " + drink.getCost());
            if (coffeeMachine.isOutOfStock(drink)) {
                System.out.print(" [Out of stock]");
            }
            System.out.println();
        }
        System.out.println();
    }

    private static final Pattern IS_DIGIT_PATTERN = Pattern.compile("^\\d+$");
    private static final Pattern IS_R_PATTERN = Pattern.compile("^[rR]$");
    private static final Pattern IS_Q_PATTERN = Pattern.compile("^[qQ]$");

    public boolean processInputAndContinue() {
        System.out.print("Dispensing: ");

        String input = scanner.next();
        System.out.println();
        if (IS_DIGIT_PATTERN.matcher(input).matches()) {
            int selectedDrink = Integer.parseInt(input);
            if (selectedDrink < 1 || selectedDrink > indexedDrinks.size()) {
                System.out.println("Invalid drink number");
                System.out.println();
                return true;
            }
            Drink drink = indexedDrinks.get(selectedDrink - 1);
            if (coffeeMachine.isOutOfStock(drink)) {
                System.out.println("Drink " + drink + " is not in stock");
                System.out.println();
                return true;
            }
            coffeeMachine.makeDrink(drink);
            return true;
        }
        if (IS_R_PATTERN.matcher(input).matches()) {
            Map<Ingredient, Integer> newStock = coffeeMachine.getIngredients().stream()
                .collect(Collectors.toMap(ingredient -> ingredient, ingredient -> 10));
            coffeeMachine.restock(newStock);
            System.out.println("Restocked");
            System.out.println();
            return true;
        }
        if (IS_Q_PATTERN.matcher(input).matches()) {
            System.out.println("Quit");
            return false;
        }
        System.out.println("Invalid input");
        System.out.println();
        return true;
    }
}

public class CoffeeMachineApplication {
    public static void main(String[] args) {
        List<Ingredient> ingredients = Default.getDefaultIngredients();
        Map<Ingredient, Integer> ingredientStock = ingredients.stream().
            collect(Collectors.toMap(ingredient -> ingredient, ingredient -> 10));
        List<Drink> drinks = Default.getDefaultDrinks();

        CoffeeMachine coffeeMachine = new CoffeeMachine(ingredients, ingredientStock, drinks);
        CoffeeMachineDisplay coffeeMachineDisplay = new CoffeeMachineDisplay(coffeeMachine);

        do {
            coffeeMachineDisplay.displayCurrentStock();
            coffeeMachineDisplay.displayMenu();
        } while (coffeeMachineDisplay.processInputAndContinue());
    }
}
\$\endgroup\$
15
\$\begingroup\$

Overall

I like how you made a lot of your classes immutable. I also like the OOP-ness of your code. I'd say you'd definitely "pass the test".

A class just to have a class

public class IngredientSearcher extends Searcher<String, Ingredient> {
    public IngredientSearcher(final Collection<? extends Ingredient> ingredients) {
        super(Ingredient::getName, ingredients);
    }
}

I am usually against creating a class just for the sake of creating a class. This can just as well be a static method:

public static Searcher<String, Ingredient> ingredientSearcher(final Collection<? extends Ingredient> ingredients) {
    return new Searcher<>(Ingredient::getName, ingredients);
}

The same goes for your DrinkSearcher.

Naming things

DEFAULT_DRINKS is technically not a constant, because it is mutable. I would either name this defaultDrinks, or when creating it, wrap it inside a Collections.unmodifiableList() call.


 Pattern IS_DIGIT_PATTERN

Hungarian notation, anyone?


IS_R_PATTERN could be named with what the R means, to make your logic flow more self-documenting. How about IS_REFILL ?

Coffee, Coffee, Coffee, Sugar, Cream

This code is very tedious to write and I bet you used Ctrl + C and Ctrl + V when writing it!

DEFAULT_DRINKS.add(new Drink("Coffee", Arrays.asList(
    ingredientSearcher.search("Coffee"),
    ingredientSearcher.search("Coffee"),
    ingredientSearcher.search("Coffee"),
    ingredientSearcher.search("Sugar"),
    ingredientSearcher.search("Cream")
)));

I think it would be better to add a drink something like this:

.add(Drink.builder("Coffee", ingredientSearcher)
     .with("Coffee", 3)
     .with("Sugar")
     .with("Cream")
     .build()
);
\$\endgroup\$
6
\$\begingroup\$

Your main concerns

  • My CoffeeMachineDisplay#processAndContinue method is not really clean, ideally it should be refactored, but I'm unsure how.

The common approach is to process and handle inputs in a loop:

  1. Display prompt, read input
  2. Parse and validate input, or else return to step 1
  3. Perform action (quit is one of them)
  4. Print the result of the action, start over from step 1

You could decompose and refactor your method to these steps. The main loop would be better in CoffeeMachineDisplay than in CoffeeMachineApplication.main.

  • I have avoided using enums for the ingredients as that limits the applicability of this application in real software, you may want to dynamically load the ingredients and drinks from a database for example.

That's a very good point. That approach is more suitable for this task than an enum.

  • I have kept an eye on obvious performance improvements, though have not wasted too much time and/or code on premature optimization in my eyes.

I don't see big performance concerns here.

  • I have used defensive programming everywhere, because users are evil.

Oh yes they are. I'm just a tad concerned if the coffee machine and its display can go "out of sync", as their vision of the stock can diverge this way. Maybe it's alright, I'd have to look deeper to see if there's a real concern there, this is just a gut feeling for now.

What is a valid ingredient?

I looked at this method with suspicion:

private void checkIsValidIngredient(final Ingredient ingredient) {
    if (!ingredients.contains(ingredient)) {
        throw new IllegalArgumentException("Unknown ingredient: " + ingredient);
    }
}

I don't see why this method should throw. On closer look, I don't see why it exists at all. An ingredient should be valid if it can be constructed. It should be the constructor's job to refuse to construct something invalid. But I guess this behavior comes from your semantic rules, which I will address later below.

If an ingredient is not in stock, that shouldn't be illegal. It's something that can normally happen in the life time of any coffee machine.

This method is only called by getCurrentStock. When there is no stock, I'd expect a 0 returned, not an IllegalArgumentException.

What is a valid drink?

checkIsValidDrink is an interesting restriction. This coffee machine leaves no room for improvisation :-) You can pass makeDrink your favorite mix, the code compiles, but then at runtime you get a IllegalArgumentException. This might be fine if your recipe consists of Coffee x 100 (and will likely kill you), but it doesn't make a fun barista.

Maybe the coffee machine shouldn't be the authority on what is a valid drink or not. It can just make coffee when it has the ingredients for it.

The semantic rules, and input handling

There are a couple of semantic rules that cannot be enforced at compile time, and when inadvertently violated, cause the program to crash at runtime:

  • Calling makeDrink with an unauthorized drink compiles but crashes at runtime: users are assumed to know to only call makeDrink with an authorized drink
  • Creating a coffee machine with an empty stock compiles but crashes at runtime when you try to make any drink: users are assumed to know to create a coffee machine with consistent parameter set
  • Running out of stock crashes the program: users are assumed to know to call isOutOfStock first before calling makeDrink (this also make the program non-thread-safe, but probably that doesn't matter in this exercise)

Although it's not possible to convert these rules to cause compiler error instead of runtime error, you can change the program to handle these situations better:

  • If you want to allow only authorized drinks, then make makeDrink throw an UnauthorizedDrink checked exception and let users handle the situation gracefully
  • When makeDrink cannot make a Drink because it's out of stock, I think it will be better to throw an OutOfIngredients checked exception and let users handle the situation gracefully
  • Checking the stock will fail with an NPE for a Drink that's not authorized. That's a bug to fix.

Usability

It's too hard to create a CoffeeMachine. Basically I need the boilerplate code from your main class, and the Default class. Otherwise it's too much work to create test cases.

For one thing, it seems the Collection<Ingredient> parameter is unnecessary. The constructor could figure this out from the list of Drinks.

What will really help is some builders like @Simon suggested, to make it easier to create drinks and ingredients and write some tests to push the limits of the coffee machine.

\$\endgroup\$
  • \$\begingroup\$ On the Ingredient/Drink validity: Assume this isn't a CoffeeMachine but a DrinkMachine, then you could make a CoffeeMachine and a SoupMachine instance... You specify the ingredients and drinks during construction; now if you put a soup ingredient in the coffee machine, I would not be offended if an exception was thrown. \$\endgroup\$ – skiwi Mar 5 '15 at 17:14
  • \$\begingroup\$ About the thread-safety: I have never yet seen a DrinkMachine IRL that two users are able to use at once ;-) \$\endgroup\$ – Simon Forsberg Mar 5 '15 at 17:18
  • \$\begingroup\$ I assumed that by Ingredient you really meant CoffeeDrinkIngredient. If a SoupIngredient can be passed to your CoffeeMachine, then that's another design problem. You just need to use the right interfaces. \$\endgroup\$ – janos Mar 5 '15 at 17:28

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.