1

I was studying the "Decorator pattern". Doing some tests with C# I do not understand why I do not get the expected result. This is the code:

public abstract class Drink
{
    public string description = "Generic drink";

    public string GetDescription()
    {
        return description;
    }
}

public abstract class DrinkDecorator : Drink
{
    public abstract string GetDescription();
}


public class SecretIngredient : DrinkDecorator
{
    Drink drink;

    public SecretIngredient (Drink drink)
    {
        this.drink = drink;
    }

    public override string GetDescription()
    {
        return this.drink.GetDescription() + ", SecretIngredient ";
    }
}

public class Espresso : Drink
{
    public Espresso()
    {
        description = "Espresso";
    }
}


[TestFixture]
class TestClass
{
    [Test]
    public void TestMethod()
    {
        Drink drink = new Espresso();
        System.Diagnostics.Debug.WriteLine(drink.GetDescription());

        drink = new SecretIngredient (drink);
        System.Diagnostics.Debug.WriteLine(drink.GetDescription());
    }
}

Performing the test I get:

Espresso

Generic drink

While I would have expected:

Espresso

Espresso, SecretIngredient

Why? Thanks in advance.

Community
  • 1
  • 1
Luca F.
  • 113
  • 1
  • 10
  • 2
    `SecretIngredient secret = new SecretIngredient(drink);` – LarsTech Oct 31 '18 at 16:37
  • 1
    You have two completely different `GetDescription` methods. One defined in `Drink` and one in `DrinkDecorator`. I think you should have only one in `Drink` and have it be a virtual method. – Yacoub Massad Oct 31 '18 at 16:41
  • 2
    When you wrote this program you should have gotten a warning telling you that something confusing could happen here because "GetDescriptor hides inherited member". As the warning suggests, indeed something confusing is happening with calls to GetDescriptor. **Pay attention to compiler warnings and understand them**. We designed those warnings to help you. If you had taken the time to understand why the compiler was giving you a warning, then you would have figured this question out. – Eric Lippert Oct 31 '18 at 17:03
  • Also, you say that you are studying the decorator pattern, but this is not how you do the decorator pattern in C#. What tutorial or reference did you use to learn this incorrect implementation of the pattern? I am curious to know how people come to have incorrect beliefs about programming; it helps me write better articles and tutorials that teach correct beliefs. – Eric Lippert Oct 31 '18 at 17:10
  • @EricLippert Apologies, but I don't believe this has to do with it hiding the base member. He's going to get this result whether he uses the new keyword or not and the fact is that since he is hiding it he's wondering why he's still using it. The question suggests that the GetDecriptor is using the base method ```GetDescriptor()``` although he has instantiated a ```SecreIngredient```. If he uses ```Console.WriteLine(((SecretIngredient)drink).GetDescription());``` then it works. – Michael Puckett II Oct 31 '18 at 17:22
  • @MichaelPuckettII: I disagree. On seeing the warning mentioned by Eric, I tried replacing the triggering line (`public abstract string GetDescription();` ) with `public abstract override string GetDescription();` (the warning said to use `new` if hiding was intended, but hiding was not intended). This of course generated an error, warning me that the inherited member was not marked `virtual`, `abstract`, or `override`. So, I marked the base member `virtual`, as that was the only possible choice. This eliminated all warnings and generated the output expected by OP. – Brian Oct 31 '18 at 17:29
  • @Brian You confused me because I copied and pasted it exactly and ran it. I then use the 'new' keyword and ran it. I then added the conversion as I did in my comment and ran it. I didn't have to make other changes. TBH; this questions and code is a complete mess but I think I get what the OP is trying to solve. I believe I nailed it in my answer below... Would you also take a look and let me know your thoughts? – Michael Puckett II Oct 31 '18 at 17:31
  • @Brian As soon as you reference the types as they are expected things work. This is the same issue you fall into when using multiple similar interfaces on types which I also tried to show in my answer. Types can have many same members and it's always the referenced type of which these members are invoked. This has nothing to do with hiding members I wouldn't think. Am I missing something? – Michael Puckett II Oct 31 '18 at 17:34
  • @MichaelPucketII: Given the output the "expected output" section of the OP's question, I think the OP is interested in leveraging inheritance, not method hiding. Further, this fits well with the OP's existing code. With [rare exceptions](https://stackoverflow.com/a/5709191/18192), I consider `new` to indicate poor/no communication between the designers of the base and inherited class. Lack of communication is unavoidable if the base class is developed by a different team than the derived class, but such an inheritance hierarchy should not created intentionally. – Brian Oct 31 '18 at 17:40
  • @Brian I agree... My point is that the ```new``` keyword doesn't solve the problem either. This isn't an issue of member hiding but of type referencing. Any type that is referenced as another type will fall back to the referenced type for it's members and invoke them. I have shown all this below in my answer and this is a problem with understanding type referencing (implicit vs explicit and more). See how converting solves the issue: ```System.Diagnostics.Debug.WriteLine(((SecretIngredient)drink).GetDescription());``` – Michael Puckett II Oct 31 '18 at 17:42
  • No offense taken, and the problem is exactly that hiding is used when it is not intended; the warning makes that clear. The right response to the warning is not to add `new` to make the warning go away. The right response is to **understand why the warning exists, think hard about it, and ensure that you are implementing the pattern correctly**. In this case the pattern is not implemented correctly, which is the crux of the problem. – Eric Lippert Oct 31 '18 at 17:44
  • @EricLippert That's my point exactly. When you add ```new``` it doesn't solve the problem. This is an issue with type referencing. – Michael Puckett II Oct 31 '18 at 17:45
  • Right, `new` does not solve the problem. But **the point of the decorator pattern is that you must be able to use the decoration object as though it was an instance of the underlying object**. You *must* be able to cast to `Drink` and have it work as expected. If the implementation does not have that property then it is not an example of the decorator pattern! – Eric Lippert Oct 31 '18 at 17:46
  • @EricLippert Right! I think we are saying the same thing. The reason the pattern isn't working is because one, it's not implemented correctly to start with but 2 because it's not referencing properly; which is what the pattern overcomes. However; I didn't feel that hiding was the issue. – Michael Puckett II Oct 31 '18 at 17:50

2 Answers2

2

The fundamental problem is that you have not implemented the decorator pattern correctly. A correct implementation of the decorator pattern would be as follows (and we'll fix a bunch more stuff while we're at it.)

public abstract class Drink
{
  // Fields should be private or protected, but the 
  // description field that was here is useless, and
  // even if it were here, it should be a constant, 
  // not a variable
  // Eliminate it.
  // public string description = "Generic drink";

  // Things that are logically properties should be
  // properties, not GetBlah methods.

  // In new versions of C# you can use compact syntax 
  // for properties.

  // In the decorator pattern the behaviour mutated by the
  // decorator should be virtual.

  public virtual string Description => "generic drink";
}

public abstract class DrinkDecorator : Drink
{
  // The decorator must override the underlying implementation.
  public abstract override string Description { get; }
}

public class SecretIngredient : DrinkDecorator
{
    Drink drink;
    public SecretIngredient (Drink drink)
    {
        this.drink = drink;
    }

    // Use interpolation.
    public override string Description =>
      $"{this.drink.Description}, SecretIngredient ";
}

public class Espresso : Drink
{
    public Espresso()
    {
       // This is just wrong. We have a mechanism for overriding
       // behaviour so **use it**.
       //   description = "Espresso";
    }
    public override string Description => "Espresso";
}


[TestFixture]
class TestClass
{
    [Test]
    public void TestMethod()
    {
        Drink drink = new Espresso();
        System.Diagnostics.Debug.WriteLine(drink.Description);
        drink = new SecretIngredient (drink);
        System.Diagnostics.Debug.WriteLine(drink.Description);
    }
}

And now the correct implementation has the expected output.

The reason your wrong implementation got the wrong output was because it was wrong. You had two entirely separate implementations of GetDescription that had nothing to do with each other, one implemented by Drink and the other by the decorator, so which one got called depended on the compile-time type of the receiver, which was Drink.

You should have gotten a warning saying that you had a possibly unintentional hiding of an old method by a new method. Pay attention to those warnings. If you get a warning saying "this method is probably wrong" and then you get a wrong result when you call that method, the warning was right.

Eric Lippert
  • 647,829
  • 179
  • 1,238
  • 2,067
  • Apologies man but you've implemented it wrong in your example. In your example there's no need for a decorator. I believe this was an accident... The ```DrinkDecorator``` should have the concrete ```Drink``` type and ideally it should be protected. The inheritors of ```DrinkDecorator``` shouldn't have a hold on the drink type themselves; only pass it to the base (```DrinkDecorator```). This works but only if we do this for every type; in which case, we don't need a decorator anymore. – Michael Puckett II Oct 31 '18 at 19:33
  • The decorator also shouldn't be abstract; it simply needs to override the ```drink``` it instantiated. I'll paste the code in the comment and you can take a look; like I said I think this was accidental. – Michael Puckett II Oct 31 '18 at 19:37
  • @MichaelPuckettII: Sure, the logic can reasonably go in the decorator base class. The point of the exercise is that *there is only one description slot*, and not two as in the original code. – Eric Lippert Oct 31 '18 at 19:38
  • public abstract class DrinkDecorator : Drink { protected Drink drink; protected DrinkDecorator(Drink drink) => this.drink = drink; public override string Description => drink.Description; } public class SecretIngredient : DrinkDecorator { public SecretIngredient(Drink drink) : base(drink) { } public override string Description => $"{drink.Description}, {nameof(SecretIngredient)} "; } – Michael Puckett II Oct 31 '18 at 19:38
  • public class AnotherSecretIngredient : DrinkDecorator { public AnotherSecretIngredient(Drink drink) : base(drink) { } public override string Description => $"{drink.Description}, {nameof(AnotherSecretIngredient)} "; } – Michael Puckett II Oct 31 '18 at 19:38
  • Well, I wasn't trying to blow you up just thought you missed it on accident and figured you'd want to correct it. I think that's why you lost the votes on your answer. – Michael Puckett II Oct 31 '18 at 19:40
  • Your proposed implementation adds complexity without any demonstration of how that complexity provides a benefit. Again, the point of the exercise is not to debate the fine details of the decorator type hierarchy; there are lots of correct ways to implement that. The point is that (1) the relevant members must be virtual in the decorated class so that they can be overridden in the decorator, and (2) pay attention to compiler warnings that tell you that your code is wrong, because those warnings are right. I long ago stopped caring about downvotes, but I appreciate your concern. – Eric Lippert Oct 31 '18 at 19:45
  • Apologies again man but you did state `A correct implementation of the decorator pattern would be as follows` and in your implementation; you can remove the decorator and just reference the base Drink and it still works. The purpose of a decorator is to extend a type dynamically not force a type to implement extension. If you inherit from the decorator then you can extend but if you avoid the decorator then you should be concrete in those regards. You're right; it's not a debate but I still feel this is misleading. – Michael Puckett II Oct 31 '18 at 19:51
  • Also, in my code in the comments.. This removes complexity, it doesn't add it. Why would you say that? – Michael Puckett II Oct 31 '18 at 19:52
  • @MichaelPuckettII: Because in both our example implementations, the decorator base class adds zero value; it has no useful implementation to share, and it is never used polymorphically, so it has no value. Both of them are useless, but at least mine is short! *What is the compelling benefit of having a base class for all decorators?* Regardless though, the point of this exercise is not to consider the many possible ways that a decorator class hierarchy could be implemented, but rather to note that the virtual methods must be overridden correctly. – Eric Lippert Oct 31 '18 at 19:59
  • Ha, I was going to comment on that too. It should be an interface in our example but there is potential for base logic that isn't part of our descriptor. And if we use an interface then we are doing more in virtualization the type and the code changes even more. The point is; if you're going to make a decorator, then make a decorator lol. But if you're happy with that being your example then by all means let it stick. – Michael Puckett II Oct 31 '18 at 20:02
1

This is because you have Drink declared as a Drink type.

Before you read my explanation; if you do this to your code it will work and I try to explain why below:

System.Diagnostics.Debug.WriteLine(((SecretIngredient)drink).GetDescription());

When you assign a Type to the reference then this Type is the fallback for metadata. In other words, whatever fields, methods, properties, the Type has (or has inherited) is what is used; nothing above.

Here we have a simple Person that is also the base of an Employee. Look at the output and follow the type declarations.

using System;
namespace Question_Answer_Console_App
{
    class Program
    {
        static void Main(string[] args)
        {
            Person person = new Person() { Name = "Mathew" };
            Person employeePerson = new Employee() { Name = "Mark" };
            Person castedEmployee = new Employee() { Name = "Luke" };
            Employee employee = new Employee() { Name = "John" };
            //Compile error -> Employee personEmployee = new Person() { Name = "Acts" };    

            Console.WriteLine(person.Name);
            Console.WriteLine(employeePerson.Name); //Referenced Employee but got Person
            Console.WriteLine(((Employee)castedEmployee).Name); //Notice we cast here
            Console.WriteLine(employee.Name);

            Console.ReadKey();
        }
    }

    public class Person
    {
        public string Name { get; set; } = "No Name";
    }

    public class Employee : Person
    {
        new public string Name { get; set; }
        public string Address { get; set; }
    }
    //Output
    //Mathew
    //No Name
    //Luke
    //John
}

Ok, so if you were able to follow that and make sense of how Type metadata is used then now you need to look at this with interface. It's the same thing but we can have an awkward twist.

With in interface it is possible for two interfaces to have the same properties or methods or even property and method names but different logic. When a Type uses more than one interface and they share any of these but the logic required is different we then need to explicitly declare that interface members. However; when we do this then we ONLY get to use those members when the Type is references as such. Look at this similar example:

First notice that now 'Luke' (previously this was Mark but same logic) prints... Why is that when we are referencing a Person but it's instantiated as Employee. Before this didn't work. Also notice there's a hole in the output although the member was defined; however in this case our reference is to IEmployee when that happens. Play with all of this code for a while until it sinks in because it can be a big problem later.

using System;
namespace Question_Answer_Console_App
{
    class Program
    {
        static void Main(string[] args)
        {
            IPerson iPerson = new Person() { Name = "Mathew" };
            Person person = new Person() { Name = "Mark" };
            Person employeePerson = new Employee() { Name = "Luke" }; //pay attention to this!!
            IPerson iEmployeePerson = new Employee() { Name = "John" };
            IEmployee iEmployee = new Employee() { Name = "Acts" }; //And pay attention to this!!            
            Employee employee = new Employee() { Name = "Romans" };

            Console.WriteLine(iPerson.Name);
            Console.WriteLine(person.Name);
            Console.WriteLine(employeePerson.Name);
            Console.WriteLine(iEmployeePerson.Name);
            Console.WriteLine(iEmployee.Name);
            iEmployee.Name = "Corinthians"; //And pay attention to this!!
            Console.WriteLine(iEmployee.Name);
            Console.WriteLine(employee.Name);

            Console.ReadKey();
        }
    }

    public interface IPerson
    {
        string Name { get; set; }
    }

    public interface IEmployee
    {
        string Name { get; set; }
    }

    public class Person : IPerson
    {
        public string Name { get; set; } = "No Name";
    }

    public class Employee : Person, IEmployee
    {
        public string Address { get; set; }
        string IEmployee.Name { get; set; } //And pay attention to this!! (Explicit interface declaration)
    }
    //Output
    //Mathew
    //Mark
    //Luke
    //John

    //Corinthians
    //Romans
}

Now; if you understand that so far then let's look at ways to get around it. Take the first example: If you add virtual to Name property of Person and then use override in the Name property of Employee you'll see that the types now work as expected. This is because we are not referencing two different methods. We are marking one with the ability to be re-referenced (virtual) and another to reference it (override). This changes the behavior greatly.

So all that said and understood then let's make a proper decorator.

First; we need to have a type:

public class Person
{
    public virtual string Name { get; set; } = "John Doe";
}

Now we need types that have extended functionality... (this will need to be altered later)

public class Employee : Person
{
    public override string Name => $"Employee, {base.Name}";
    public string Job { get; set; }
}

public class Customer : Person
{
    public override string Name => $"Customer, {base.Name}";
    public bool IsShopping { get; set; }
}

Now it is possible for an employee to also be a customer. Based on our current design we have a problem... We should have added interfaces but then what about computation? In this example there isn't any, except for Name, which isn't real world but it does it's job. So in order to allow Person to be dynamically updated we should add a PersonDecorator. When we add this decorator we need to inherit from it and use other types to instantiate.

Here's our decorator:

public abstract class PersonDecorator : Person
{
    protected Person Person { get; }
    public PersonDecorator(Person person) => Person = person;
    public override string Name => Person.Name;
}

Now we can extend Person dynamically where we couldn't before. Upgrading Employee and Customer shows how to do this:

public class Employee : PersonDecorator
{
    public Employee(Person person = null) : base(person ?? new Person()) { }
    public override string Name => $"Employee, {base.Name}";
    public string Job { get; set; }
}

public class Customer : PersonDecorator
{
    public Customer(Person person) : base(person ?? new Person()) { }
    public override string Name => $"Customer, {base.Name}";
    public bool IsShopping { get; set; }
}

Now we have upgraded our types to utilize a decorator (and notice it has fallback to types that may not). Let's use it in a small example:

static void Main(string[] args)
{
    Person person = new Person() { Name = "Mathew" };
    Console.WriteLine(person.Name);

    person = new Employee(person) { Job = "Stocker" };
    Console.WriteLine(person.Name);

    person = new Customer(person) { IsShopping = true };
    Console.WriteLine(person.Name); 

    Console.ReadKey();
}
//OUTPUTS
//Mathew
//Employee, Mathew
//Customer, Employee, Mathew

Notice how we have now dynamically extended Person.

We could also make person dynamic like so:

static void Main(string[] args)
{
    Person person = new Customer(new Employee(new Person(){ Name = "Mathew" }){ Job = "Stocker" }){ IsShopping = true };
    Console.WriteLine(person.Name);
    Console.ReadKey();
}
//OUTPUTS
//Customer, Employee, Mathew

Look how it works without the base implemented first; it remains dynamic and true to itself.

static void Main(string[] args)
{
    //Person person = new Person() { Name = "Mathew" };
    //Console.WriteLine(person.Name);

    Person person = new Employee() { Job = "Stocker" };
    Console.WriteLine(person.Name);

    person = new Customer(person) { IsShopping = true }
    Console.WriteLine(person.Name); 

    Console.ReadKey();
}
//OUTPUTS
//Employee, John Doe
//Customer, Employee, John Doe

Here's the entire code for reference Decorator Pattern

using System;
namespace Question_Answer_Console_App
{
    class Program
    {
        static void Main(string[] args)
        {
            Person person = new Person() { Name = "Mathew" };
            Console.WriteLine(person.Name);

            person = new Employee(person) { Job = "Stocker" };
            Console.WriteLine(person.Name);

            person = new Customer(person) { IsShopping = true };            
            Console.WriteLine(person.Name);

            Console.ReadKey();
        }
        //OUTPUTS
        //Mathew
        //Employee, Mathew
        //Customer, Employee, Mathew
    }

    public class Person
    {
        public virtual string Name { get; set; } = "John Doe";
    }

    public abstract class PersonDecorator : Person
    {
        protected Person Person { get; }
        public PersonDecorator(Person person) => Person = person;
        public override string Name => Person.Name;
    }

    public class Employee : PersonDecorator
    {
        public Employee(Person person = null) : base(person ?? new Person()) { }
        public override string Name => $"Employee, {base.Name}";
        public string Job { get; set; }
    }

    public class Customer : PersonDecorator
    {
        public Customer(Person person) : base(person ?? new Person()) { }
        public override string Name => $"Customer, {base.Name}";
        public bool IsShopping { get; set; }
    }
}

Here's your code updated to the decorator pattern. Notice how you can dynamically update Drink which was an Expresso by adding it to decorators.

using System;
namespace Question_Answer_Console_App
{
    class Program
    {
        static void Main(string[] args)
        {
            Drink drink = new Espresso() { Description = "Expresso" };
            Console.WriteLine(drink.Description);

            drink = new SecretIngredient(drink);
            Console.WriteLine(drink.Description);

            drink = new Ice(drink);
            Console.WriteLine(drink.Description);

            Console.ReadKey();
        }
        //OUTPUTS
        //Expresso
        //Expresso with SecretIngredient
        //Expresso with SecretIngredient with Ice
    }

    public class Drink
    {
        public virtual string Description { get; set; }
    }

    public class Espresso : Drink { }

    public abstract class DrinkDecorator : Drink
    {
        protected Drink drink;
        protected DrinkDecorator(Drink drink) => this.drink = drink;
        public override string Description => drink.Description;
    }

    public class SecretIngredient : DrinkDecorator
    {
        public SecretIngredient(Drink drink) : base(drink) { }
        public override string Description => $"{drink.Description} with {nameof(SecretIngredient)} ";
    }

    public class Ice : DrinkDecorator
    {
        public Ice(Drink drink) : base(drink) { }
        public override string Description => $"{drink.Description} with {nameof(Ice)} ";
    }
}
Michael Puckett II
  • 6,586
  • 5
  • 26
  • 46