Decorator as a Singleton Pattern

[mbde]

Attached is a classic Decorator pattern. My question is how would you modify the below code so that you can wrap zero or one of each topping on to the Pizza

Right now I can have a Pepporini -> Sausage --> Pepporini --> Pizza class driving the total cost up to $10, charging twice for Pepporini.

I don't think I want to use the Chain of Responsibility pattern as order does not matter and not all toppings are used?

Thank you



namespace PizzaDecorator
{
public interface IPizza
{
double CalculateCost();
}

public class Pizza: IPizza
{
public Pizza()
{
}

public double CalculateCost()
{
return 8.00;
}

}

public abstract class Topping : IPizza
{
protected IPizza _pizzaItem;

public Topping(IPizza pizzaItem)
{
this._pizzaItem = pizzaItem;
}

public abstract double CalculateCost();

}

public class Pepporini : Topping
{
public Pepporini(IPizza pizzaItem)
: base(pizzaItem)
{
}

public override double CalculateCost()
{
return this._pizzaItem.CalculateCost() + 0.50;
}


}

public class Sausage : Topping
{
public Sausage(IPizza pizzaItem)
: base(pizzaItem)
{
}


public override double CalculateCost()
{
return this._pizzaItem.CalculateCost() + 1.00;
}
}

public class Onions : Topping
{
public Onions(IPizza pizzaItem)
: base(pizzaItem)
{
}

public override double CalculateCost()
{
return this._pizzaItem.CalculateCost() + .25;
}
}
}

 

[jmeckley]

in a pure decorator implementation the decorators, do not know about each other. However you could solve this by adding another member to IPizza which checks for topping types. this is off the top of my head.

interface IPizza
{
  bool HasTopping(Type topping);
}

abstract class Topping : IPizza
{
   public virtual bool HasTopping(Type topping)
   {
      return GetType() == topping
   }
}

class Pepporini : Topping
{
   public override bool HasTopping(Type topping)
   {
      if(base.HasTopping(topping) return true;
      return topping == typeof(Pepporini);
   }
}

class Sausage : Topping
{
   public override bool HasTopping(Type topping)
   {
      if(base.HasTopping(topping) return true;
      return topping == typeof(Sausage);
   }
}

it still feels somewhat clunking as checking for toppings is not related to the topping itself. however you can always refactor. maybe have Topping be it's own object independent of pizza and have a topping/pizza adapter which is aware of the toppings.

if this is for learning the problem is interesting. If this is for an actual product, I would make toppings there own entity and calculate the total from pizza. add toppings to the pizza instead of decorating the pizza and have the pizza manage the toppings

interface Topping
{
   double Cost {get;}
}

class Pizza : IPizza
{
   private List<Topping> toppings = new List<Topping>();

   public IPizza AddTopping(Topping topping)
   {
      if(!toppings.Contains(topping))
          toppings.Add(topping);

      return this;
   }

   public double CalculateCost()
   {
      double total = Cost;
      foreach(Topping topping in toppings)
         total+=topping.Cost;
      return total;
   }

   public double Cost { get { return 8D; } }
}

Jason Meckley
Programmer
Specialty Bakers, Inc.

 

[mbde]

Thanks, for the discussion this is what I think I will go with... Of course I am not selling pizza's but an object where the customer gets to choose multiple items (so I didn't go Strategy pattern) and the order of the items are irrelevent (so I didn't choose Chain pattern). The only other pattern I was considering was composite, but again it looks like decorator is going to work

using System;
using System.Collections.Generic;
using System.Text;

namespace PizzaDecorator
{
    public interface IPizza
    {
        double CalculateCost();
        bool HasTopping(Type topping);
    }

    public class Pizza: IPizza
    {
        public Pizza()
        {
        }

        public double CalculateCost()
        {
            return 8.00;
        }

        public bool HasTopping(Type topping)
        {
            return false;
        }

    }

    public abstract class Topping : IPizza
    {
        protected IPizza _pizzaItem;

        public Topping(IPizza pizzaItem)
        {
           
        }

        public abstract double CalculateCost();
        public  bool HasTopping(Type topping)
        {
            // Check the parent Items 
            if (this._pizzaItem.HasTopping(topping))
            {
                return true;
            }
            // Check this Item
            return this.GetType() == topping;
        }
       

    }

    public class Pepporini : Topping
    {
        public Pepporini(IPizza pizzaItem)
            : base(pizzaItem) 
        {   
            if (! pizzaItem.HasTopping(typeof(Pepporini)))
            {
                this._pizzaItem = pizzaItem;
            }
            else 
            {
                throw new ArgumentException("Pizza has topping");
            }
        }

        public override  double CalculateCost()
        {
            return this._pizzaItem.CalculateCost() + 0.50;
        }

      
        
    }

    public class Sausage : Topping
    {
        public Sausage(IPizza pizzaItem)
            : base(pizzaItem)
        {
            if (!pizzaItem.HasTopping(typeof(Sausage)))
            {
                this._pizzaItem = pizzaItem;
            }
            else
            {
                throw new ArgumentException("Pizza has topping");
            }
        }


        public override double CalculateCost()
        {
            return this._pizzaItem.CalculateCost() + 1.00;
        }

     
    }

    public class Onions : Topping
    {
        public Onions(IPizza pizzaItem)
            : base(pizzaItem)
        {
            if (!pizzaItem.HasTopping(typeof(Onions)))
            {
                this._pizzaItem = pizzaItem;
            }
            else
            {
                throw new ArgumentException("Pizza has topping");
            }
        }
       
        public override double CalculateCost()
        {
            return this._pizzaItem.CalculateCost() + .25;
        }

      
    }
}

 

[jmeckley]

this is good. here are a few tips.

1. since you're throwing an exception you don't need the else clause you can simply do

if(has topping)
  throw new Exception();
this.pizza = pizza;

if the exception throws the next line is never reached.

2. create a DuplicateToppingException which inherits from excpetion and throw this instead of a generic Exception.
3. supply a meaningful exception message. Something like

class DuplicateToppingException : Exception
{
   public DuplicateToppingException(Type topping)
      : base(string.Format("{0} was already added to the pizza.", topping.Name))
   {
   }
}

4. there is probably a way to eliminate the repeated type checks, but it's escaping me right now. you could use a topping validation visitor, which keeps a list of toppings. (independent from the decorated pizza object). move the validation from the toppings to the validator and pass the validator along with each decorator.

Jason Meckley
Programmer
Specialty Bakers, Inc.

 

[mbde]

Jason,

Point
1. Good point, will do.

2 & 3. Yes, I was going to do that but it was sort of irrelevant to the post.

4. I never heard of a validation visitor, I will look into that.

Thank you

 

[jmeckley]

I never heard of a validation visitor, I will look into that.

don't get hung up on the name of the pattern. it simply another way to solve the problem.

Jason Meckley
Programmer
Specialty Bakers, Inc.

 

[Craig0201]

I'll ask a more basic question.....

What happens to the guy who wants double or triple pepperoni? Is he banned because you can only decorate a pizza with pepperoni once?

In other words, why do you want to do this? Knowing why will likely influence the responses.

What I can say is that you appear hellbent on cramming a pattern here. Use patterns when the need arises, not because patterns are good. In other words, can this be fixed rather more easily using composition?

C

 

[mbde]

Craig thanks for the response.
You do bring up a good point however I am not selling pizzas. That was just an example to mask some really business code. The company product is like a insurance service where you have to start out with a basic plan then you can addon addition types of coverage. But you either have it or you don't. I like the decorator pattern because it cascaded values like total price and other coverage aspects. The order of selection, in this case, does not matter. The only alternative that I could think of is the one Jason suggested and that is an array. But again I lean toward decorator for its ability to take an object in whatever state, do what it needs to do and pass it along.

 

[jmeckley]

mbde, for future posts use the real example. when examples are contrived the response may be as well. if however the problem is real (especially with money, insurance, etc) the response is more direct.

for example, had I known you were not dealing with pizzas I would favor Craig's reaction of applying patterns where they may not be needed. That and pizza's is a very textbook example of case study, so it comes across as more of an academic problem, rather than a practical one.

Jason Meckley
Programmer
Specialty Bakers, Inc.

 

[Craig0201]

What I can say is that you appear hellbent on cramming a pattern here. Use patterns when the need arises, not because patterns are good. In other words, can this be fixed rather more easily using composition?"

What you describe doesn't appear to require decorator. It seems to be that you want to use a pattern because it looks nice. (This has come out harshly and isn't meant to be so).

From my thoughts, what's wrong with having all the policy and the add-ons adhere to an IInsuranceElement (or whatever you want to call it) interface and a collection? It's seems at first glance, a lot simpler than trying to cram a pattern into a place it doesn't quite fit.

C

 

[mbde]

Jason, what then would be your advice for posting code on a public board so that the intellectal property of a company are not violated. Granted if I did get strange answers to my question then I would have most likely clearified. But since your first answer was, and usually is, the correct one I did not see the harm in masking the problem. While exposing it can be harmful to my client.

 

[jmeckley]

exposing snippets of code isn't harmful, or a violation of copy-write infringement (unless your company has a very strict policy about it). your not revealing sensitive information like data, passwords or servers. this information should be omitted. But simply asking "how do I code this?" won't expose company secrets.





Jason Meckley
Programmer
Specialty Bakers, Inc.

 

[DavidKuhn0422]

"What I can say is that you appear hellbent on cramming a pattern here. Use patterns when the need arises, not because patterns are good. In other words, can this be fixed rather more easily using composition?"

What you describe doesn't appear to require decorator. It seems to be that you want to use a pattern because it looks nice. (This has come out harshly and isn't meant to be so).

I think that using the decorator pattern is a good idea...are there better ways, maybe, but we don't know the complete business scenario. Decorator is used to take an existing object and add to it.

With Design Patterns and design in general, people have different opinions on the best way to do things.

 

[stevexff]

Patterns are good in as much as they are the optimal result of numerous iterations of people trying to solve a particular class of problem. But if you are going to apply one, you need to be sure that your problem is the same as the one the pattern is designed to solve.

As Craig has noted, composition looks good for this - a pizza (policy) can have zero or a number of toppings (add-on benefits or restrictions). If the pizza class has a collection of toppings, these can be subclassed to provide a strategy for calculating each. Then all you need is some kind of guard condition to prevent more than one of each subtype from being added to the collection (I'm a little rusty but I'm not aware of a collection that will do this out of the box).

Decorator is more use when you want to add functionality to a class you don't own or aren't at liberty to change, which isn't the case in your example...

Steve

[small]"Every program can be reduced by one instruction, and every program has at least one bug. Therefore, any program can be reduced to one instruction which doesn't work." (Object:erlDesignPatterns)[/small]

 

[jmeckley]

Then all you need is some kind of guard condition to prevent more than one of each subtype from being added to the collection (I'm a little rusty but I'm not aware of a collection that will do this out of the box).

This is known as a set. It mentions the hashtable in .net implementing this concept. There is also a typed set implementation in the Ies.Collections library (ISet<T>). I know Nhibernate requires this, I'm not sure if they also maintain the library as well.

Jason Meckley
Programmer
Specialty Bakers, Inc.