4

Reading Head First Design Patterns. I came to the chapter 3, the Decorator Pattern,

Decorator Pattern is very clear for me. but one of the "sharpen your pencil"

Our friends at Starbuzz have introduced sizes to their menu. You can now order a coffee in tall, grande, and venti sizes (translation: small, medium, and large). Starbuzz saw this as an intrinsic part of the coffee class, so they’ve added two methods to the Beverage class: setSize() and getSize(). They’d also like for the condiments to be charged according to size, so for instance, Soy costs 10¢, 15¢ and 20¢ respectively for tall, grande, and venti coffees.How would you alter the decorator classes to handle this change in requirements?

at the end of the chapter , they added code snippet to solve this part this code snippet was added to every CondimentDecorator concrete class

public double cost() {
    double cost = beverage.cost();
    if (getSize() == Beverage.TALL) {
        cost += .10;
    } else if (getSize() == Beverage.GRANDE) {
        cost += .15;
    } else if (getSize() == Beverage.VENTI) {
        cost += .20;
    }
    return cost;
}

first add Size enum to the Beverage class, then do if..else to check different sizes

I don't think this code snippet will be extensible in the future, what if starbuzz asked to add more sizes, it will lead to change every single CondimentDecorator concrete class, and it will be maintenance nightmare

I have been thinking to refactor this part to be also a design pattern, but i am still new into this topic.

What is the most proper pattern we can apply to solve this size issue ?

The full class hierarchy can be found in https://github.com/bethrobson/Head-First-Design-Patterns/tree/master/src/headfirst/designpatterns/decorator/starbuzzWithSizes

Robert Harvey
  • 198,589
  • 55
  • 464
  • 673
  • can you post the class heirarchy, I haven't gone through Heads First book so I am unable to make out the CondimentDecorator flow here. – Sikorski Aug 20 '18 at 10:23
  • Possible duplicate of [How do I edit a chain of if-else if statements to adhere to Uncle Bob's Clean Code principles?](https://softwareengineering.stackexchange.com/questions/363874/how-do-i-edit-a-chain-of-if-else-if-statements-to-adhere-to-uncle-bobs-clean-co) – gnat Aug 20 '18 at 10:23
  • @Sikorski, I edited the question and added the source code of starbuzz – Melad Basilius Aug 20 '18 at 11:10
  • 1
    if this was to be production code then the getCost() method would actually be hitting a database for fetching the actual cost. When it comes to this adding more size or types is just a matter of more rows in database. Classes would not change that much. I wonder if adding subclasses for all types of coffee is even valid or maintainable. – Sikorski Aug 20 '18 at 12:23
  • @Sikorski, Very good point, but in this context the author is trying to explain the Decorator pattern, for me i try to improve my design skills, so that question was in my head for long time – Melad Basilius Aug 20 '18 at 12:26
  • 1
    If you like, read my updated response. Since this was a great question, it invites a more general one, to tease out what to do when the action at each decision point of the if-else/switch block is more complicated than just returning a value. – bcperth Aug 22 '18 at 00:52

6 Answers6

5

Instead of going for a full pattern with a repo connected to the database or config files, I will suggest a little fix for similar situations, because I don't think your functionality have grown complex enough to invest more efforts into it. Furthermore a more complex pattern might in fact not be the right answer anymore once you get more things added , so keep it simple.

For that I'll go for a possibility that Java offer :

1 : enum can have attributes.

public enum Beuverage{
    TALL(0.10), GRANDE(0.15), VENITI(0.20);
    public final int cost;//If i remember well it is mandatory private
    private Beuverage(int cost){...}
}

This suppose that same supplementary price for all drinks.

2 : Price differents for each type of drinks for each size : replace if/else by a Map<Size, int> internally in each component decorator

This is pretty much sefl explanitory.

Walfrat
  • 3,456
  • 13
  • 26
  • +1 for an elegant solution that uses language features instead of forcing the 500kg gorilla implementation (a.k.a. the classic implementation example from the Gang-of-Four book). It shows nicely what patterns are: abstract concepts that can be implemented in a multitude of ways. – besc Aug 20 '18 at 16:16
  • @Walfrat How does this solve the problem for other condiments with different cost additions, like caramel is 12¢, 17¢ and 22¢? You seem to have made the assumption that TALL(0.10), GRANDE(0.15), VENITI(0.20) is the only case and that is not what is asked... or am I missing something? – bcperth Aug 20 '18 at 22:46
  • @bcperth I didn't look the whole code, but from what I saw when I rechecked, only the Soy have that if statements.Milk doesn't have it. So if we want something a bit more elaborate, I add the simple possiblity to replace the if/else in each block by an internal static map. Ultimately the end solution would of course be a persistence mechanism with an IHM to be able to manage all of that. – Walfrat Aug 21 '18 at 08:33
  • @Walfrat Ah yes thank you! I am not a Java person, so I read up on Java maps after your reply, and in this use case looks quite a lot like a PHP associative array (the equivalent the solution in PHP!). I upped both the question and your reply..... – bcperth Aug 22 '18 at 00:02
1

In this example, we have to store a table of additional costs for each different condiment at the various sizes. In your example Soy costs 10¢, 15¢ and 20¢ respectively for tall, grande, and venti coffees. Maybe the corresponding costs for caramel is 12¢, 17¢ and 22¢ and different extra costs again for all other condiments. This information has to be stored somewhere, and the logical place is in each of the concrete decorator classes.

Presumably the point of the decorator classes is to be able to calculate the total cost of the final coffee, by first visiting the basic coffee and then work outwards with each condiment adding its extra. There is no getting away from the fact that each decorator needs to know the size to be able to do its job.

So the solution suggested in the book is not too bad for the situation, despite the maintenance problems you have correctly identified.

EDIT: After reading Walfrat's optimal reply for Java, I removed some unhelpful parts of my original reply. Without being language specific, there are several methods to remove if-else/switch statement blocks, such as polymorphism (see Rob), Composite (see Sikorski), Strategy (Rob again), but also including Visitor not mentioned. There are also several language specific options like Maps and Enums in Java, associative arrays in PHP and arrays of functions in C, C++ depending on how complicated the action at each decision point. Maybe there is scope for a really good general question here!

But don't forget you example is coming from a textbook whose purpose is to illustrate Decorator Pattern in action. This is quite a good example which shows how powerful the pattern is, but also how its starts to stress under certain situations.

In practice, nobody in their right mind would go to the trouble of using Decorator to add up costs like this, and there are much easier approaches. Decorator is better used in more complex situations, like processing documents or building complex user interfaces, by layering functionality.

bcperth
  • 275
  • 1
  • 5
  • I upvoted your answer, but unfortunately it will not solve the problem, because if we will add new size in the future, we still have to change the cost() method in all of the CondimentDecorator concrete classes – Melad Basilius Aug 20 '18 at 12:53
  • @Melad Ezzat Yes there is no alternative to updating the cost() method in each concrete decorator, because each concrete decorator needs to know the size and the extra costs that are unique to itself. The "problem" is theoretical because you would no accumulate costs like this using decorator. You can only solve the problem by not using decorator, or by adding a pile of new classes ( and I know which I would choose). Its interesting and fun though I think you agree :-) – bcperth Aug 20 '18 at 12:59
0

You could create size based implementations of CondimentDecorator. E.G. LargeCondimentDecorator, SmallCondimentDecorator etc each cost method returning the appropiate cost for that size.

Then make sure the appropiate one is initiated for the size instead of using the type enum.

Rob
  • 167
  • 3
  • yes that would work, but only by triplicating the Decorator structure and now spreading your cost data over three times as many classes, an even worse problem than before - but good thinking!. – bcperth Aug 20 '18 at 13:03
  • I upvoted your answer, but it will not just triple the CondimentDecorator concrete classes, it will multiply all the CondimentDecorator concrete classes by the number of sizes we will support in the future, it is another issue – Melad Basilius Aug 20 '18 at 13:12
  • 1
    Thinking about it more I dont really like my answer. It would work as you could compose with other decorators.e.g. new Large(new Sprinkles(new Latte))) but Size is not a condiment and therfore its horrible. I dont like the idea of creating subclasses of a size e.g. LargeSprinkles either due the class explosion. – Rob Aug 20 '18 at 13:27
  • @Rob, That is the goal of this forum, is to discuss everybody idea, by that everybody will be a winner, it is not about right or wrong it is about knowledge and experience transfer – Melad Basilius Aug 20 '18 at 13:49
0

One thing I'd do for sure would be refactor that into its own method. We are adding cost based on its size, aren't we? Then, it should have its method:

public double cost() { 
    return beverage.cost() + additionalCostBasedOnSize(); 
} 

private double additionalCostBasedOnSize() {
    switch(getSize()){
        case Beverage.TALL: return .10;
        case Beverage.GRANDE: return .15;
        case Beverage.VENTI: return .20; 
        default: return 0;
    }
}

From this starting point you may find some common pattern between different condiments to group them together

A Bravo Dev
  • 254
  • 1
  • 4
0

You could use the strategy pattern. Pseudo code:

inteface SizePricingPolicy
getCost(Size)

class DefaultSizePricing:SizePricingPolicy{
int getCost(Size){
 case Beverage.TALL: return .10;
        case Beverage.GRANDE: return .15;
        case Beverage.VENTI: return .20; 
        default: return 0;
}

}

Each decorator would be passed an instance of the pricing policy via constructor injection. The decorators cost() method would call the policies getCost() method passing in the size as an argument as part of determining the cost.

The benefit being: The size pricing policy can be swapped out instead of edited and because the decorators only hold references to the SizePricingPolicy interface they are isolated from changes to the size pricing.

Rob
  • 167
  • 3
0

In this case I have gone for some psuedo Composite pattern and the cost logic is altogether in a different class. Class diagram is as below :

Class Diagram

So every thing that goes in Coffee is basically a CoffeeComponent hence the interface. It has three main methods getSize(), getName() and contributionToCost(). Note that contributionToCost() method takes a parameter PricingService, this is because the cost logic is with Pricing Service. AbstractCoffeeComponent is implementing the CoffeeComponent interface to provide common methods' implementation. From AbstractCoffeeComponent two classes are subclass : Type and Ingredient. So espress, mocha will refer to Type class and sugar, milk will be Ingredient instances. Idea is that PricingService has a lookup method which can give the cost on the basis of CoffeeComponent's name and size. Rest of the hierarchy is just to showcase how coffee concept can be coded. Below is the code for PricingService, Client and Coffee.

PricingService.java

package coffee;

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

/**
 * Pricing ideally would be querying database for costs but here HashMap will do
 */
public class PricingService {

    private static final class CostComponent {
        private String componentName;
        private Size componentSize;

        public CostComponent(String name, Size size) {
            this.componentName = name;
            this.componentSize = size;
        }

        @Override
        public boolean equals(Object o) {
            if (this == o) return true;
            if (o == null || getClass() != o.getClass()) return false;
            CostComponent costComponent1 = (CostComponent) o;
            return componentName.equalsIgnoreCase(costComponent1.componentName) &&
                    componentSize == costComponent1.componentSize;
        }

        @Override
        public int hashCode() {
            return Objects.hash(componentName, componentSize);
        }
    }

    private final static Map<CostComponent, Double> costs = new HashMap<>();

    static {
        costs.put(new CostComponent("Espresso", Size.TALL), 10.0);
        costs.put(new CostComponent("Rum", Size.TALL), 40.0);
    }


    public double costOf(CoffeeComponent component){
        return costs.getOrDefault(new CostComponent(component.getName(), component.getSize()),0.0);
    }

}

Coffee.java

package coffee;

import java.util.HashSet;
import java.util.Set;

/**
 * Container class - because we want coffee
 */
public class Coffee {

    private final Type type;
    private Set<Ingredient> ingredients;

    public Coffee(Type type) {
        this.type = type;
        this.ingredients = new HashSet<>();
        this.ingredients.addAll(type.getDefaultIngredients());
    }

    public void addIngredient(Ingredient ingredient){
        this.ingredients.add(ingredient);
    }

    /**
     * Propagate size to all igredients and type
     */
    public void setSize(Size size){
        for(Ingredient ingredient: ingredients){
            ingredient.setSize(size);
        }
        type.setSize(size);
    }

    /**
     * Aggregate of all pieces for cost
     */
    public double cost(PricingService pricingService){
        double cost = 0.0;
        for(Ingredient ingredient: ingredients){
            cost+=ingredient.contributionToCost(pricingService);
        }
        cost+=type.contributionToCost(pricingService);
        return cost;
    }

}

Client.java

package coffee;

public class Client {


    public static void main(String[] args) {
        PricingService starbuzz = new PricingService();

        //Order an Espresso
        Coffee espresso = new Coffee(new Type("Espresso"));
        /**
         * Here new Type, new Ingredient is done, ideally this will also be some sort of lookup using Factory pattern
         */
        //add sugar
        espresso.addIngredient(new Ingredient("Sugar"));
        //add rum :)
        espresso.addIngredient(new Ingredient("Rum"));
        //make it large
        espresso.setSize(Size.TALL);

        double cost = espresso.cost(starbuzz);
        System.out.println("Cost : " + cost);
    }
}

CoffeeComponent.java

package coffee;

public interface CoffeeComponent {

    String getName();

    void setSize(Size size);

    /**
     * Default lookup, implementations can override if wanted
     */
    default double contributionToCost(PricingService pricingService){
        return pricingService.costOf(this);
    }

    /**
     * By default coffee is normal unless explicitly stated
     */
    default Size getSize(){
        return Size.NORMAL;
    }

}

Ingredient.java

package coffee;

/**
 * Sugar, milk, soy etc come here
 */
public class Ingredient extends AbstractCoffeeComponent {

    public Ingredient(String name) {
        super(name);
    }
}

Type.java

package coffee;

import java.util.Collections;
import java.util.Set;

/**
 * Espresso, Mocha etc types come here
 */
public class Type extends AbstractCoffeeComponent {

    public Type(String name) {
        super(name);
    }

    /**
     * Default ingredients that come with this type
     */
    public Set<Ingredient> getDefaultIngredients(){
        return Collections.emptySet();
    }
}

AbstractCoffeeComponent.java

package coffee;

public abstract class AbstractCoffeeComponent implements CoffeeComponent {

    protected final String name;
    protected Size size;

    public AbstractCoffeeComponent(String name) {
        this.name = name;
    }

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

    @Override
    public void setSize(Size size) {
        this.size = size;
    }

    @Override
    public Size getSize() {
        return size;
    }
}

As you can see if you need to add any more types, ingredients, size just add more rows in PricingService's Hashmap Hope this helps. Although I agree with @bcperth that the example was to show pros and cons of Decorator pattern.

Sikorski
  • 109
  • 5