• Post Reply Bookmark Topic Watch Topic
  • New Topic
programming forums Java Mobile Certification Databases Caching Books Engineering Micro Controllers OS Languages Paradigms IDEs Build Tools Frameworks Application Servers Open Source This Site Careers Other all forums
this forum made possible by our volunteer staff, including ...
Marshals:
  • Campbell Ritchie
  • Bear Bibeault
  • Ron McLeod
  • Jeanne Boyarsky
  • Paul Clapham
Sheriffs:
  • Tim Cooke
  • Liutauras Vilda
  • Junilu Lacar
Saloon Keepers:
  • Tim Moores
  • Stephan van Hulst
  • Tim Holloway
  • fred rosenberger
  • salvin francis
Bartenders:
  • Piet Souris
  • Frits Walraven
  • Carey Brown

What is wrong with this solution?

 
Ranch Hand
Posts: 153
1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Please observe the following video on the Decorator Pattern:


I saw someone challenge the Decorator Pattern in this video and providing the following arguments:

But why would you do that instead of just making an abstract class topping and defining many different toppings and having an ArrayList of those inside your Pizza. If you want to know the price of the Pizza instance now, you just have to loop through the ArrayList inside your getCost() method and add the price of a plain Pizza to it. Same goes for the description. Just call getName on all the toppings. Besides now you coul have all the toppings subscribe to a sales department or something like that and have it manage their prices.



What is wrong with his approach?
What design principles are being violated there?

My arguments to this are:
- Looping through an ArrayList is an incredibly inefficient way to implement this. For every topping, you would have to traverse the ArrayList.
Additionally, ArrayLists are terrible at sorting and searching, inserting an deleting because the underlying algorithms are of O(n) complexity.

- Not only is the loop inefficient, but you are asking the JVM to traverse a complex object graph each time, which adds to the inefficiency.

In the end, this is bad for performance.

If someone could provide better insight into how his solution is a bad one, I would love to hear it.
Thanks.


 
Sheriff
Posts: 15801
264
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Ryan McClain wrote:
My arguments to this are:
- Looping through an ArrayList is an incredibly inefficient way to implement this. For every topping, you would have to traverse the ArrayList.
Additionally, ArrayLists are terrible at sorting and searching, inserting an deleting because the underlying algorithms are of O(n) complexity.

- Not only is the loop inefficient, but you are asking the JVM to traverse a complex object graph each time, which adds to the inefficiency.

In the end, this is bad for performance.


How do you know it's really bad for performance? Did you perform benchmark tests? Can you back that up with quantitative data? Or are you optimizing based on what many programmers have been proven over the years to be very bad at: gut feeling and intuition? If you had 15 million different toppings, and customers could add 13 million toppings to an order, then you *might* be justified to worry about performance. But a typical pizza shop has what, 20 different toppings, maybe 40 tops (pun intended)? Is running through a list of 20-40 items really going to tax your computer to the limit of its capacity? I think not.  It all smells like premature optimization to me (Donald Knuth calls it the root of all evil).

Now, going back to the video, there were a few things I didn't like about how he implemented his ToppingDecorator and other classes. First, in ToppingDecorator, he called his field "tempPizza". That's lazy. Why "temp"?  Bad name. Next, he made it protected and he didn't make it final. That means his abstract class is already susceptible to having its encapsulation broken by subclasses. He then proceeds to break ToppingDecorator's encapsulation in Mozarella by reaching up into the superclass (ToppingDecorator) and accessing the protected tempPizza field directly from the subclass methods instead of calling super.getDescription() and super.getCost() -- that's what copy/paste programming will get you.  Bad coder.  He could have kept the pizza field in ToppingDecorator as private and he could've made it final. This at least will force subclasses to access the field's attributes via the inherited getters.

As for the argument that you quoted, the one that suggested having a Pizza keep a List<Toppings> that you put on it, I don't think that's necessarily a bad idea. Again, you shouldn't worry about performance at this point. Worry about getting behaviors, relationships, encapsulation, functional decomposition, clean dependencies, clean code, etc. before even thinking about performance. Clean code and designs are easier to make performant. Bad code just makes it harder to do anything with it, including making it performant.

Conceptually, I don't like the Pizza and Toppings example for Decorator. I always imagine a Decorator as being the same kind of thing as what it is decorating, like in the canonical example of a GUI Window being decorated (wrapped) by a ScrollableWindow which can be wrapped by perhaps a ResizableWindow, etc.  I've never liked the Pizza/Topping example, nor the other common Coffee/Additions example.  Pizza and the Toppings that you can put on it don't have that "same kind of thing" relationship in my mind.  Jalapeno, Cheese, Mushroom, Sausage are not special kinds of Pizza. They are just things I will add on top of a pizza.  Same thing for Coffee to cream, sugar, nutmeg, etc.

If you look for other examples of Decorator, I like the Email and its decorators example better.  You'd have a PlainEmail object that can get decorated by HTMLEmailDecorator that adds HTML formatting on top, which can be decorated with an ExternalEmailDecorator which adds a standard company email footer, which can be decorated by a ConfidentialEmailDecorator that adds a confidentiality marking, that can be wrapped in an EncryptedEmailDecorator that will handle encryption before it is finally sent out.  The MailSender will just see this object with its multiple Russian Nested Doll wrapping as a EMail object. This fits better with the idea of a Decorator being the same kind of thing as the object that it is decorating.

 
Ryan McClain
Ranch Hand
Posts: 153
1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Junilu Lacar wrote:

Ryan McClain wrote:
My arguments to this are:
- Looping through an ArrayList is an incredibly inefficient way to implement this. For every topping, you would have to traverse the ArrayList.
Additionally, ArrayLists are terrible at sorting and searching, inserting an deleting because the underlying algorithms are of O(n) complexity.

- Not only is the loop inefficient, but you are asking the JVM to traverse a complex object graph each time, which adds to the inefficiency.

In the end, this is bad for performance.


How do you know it's really bad for performance? Did you perform benchmark tests? Can you back that up with quantitative data? Or are you optimizing based on what many programmers have been proven over the years to be very bad at: gut feeling and intuition? If you had 15 million different toppings, and customers could add 13 million toppings to an order, then you *might* be justified to worry about performance. But a typical pizza shop has what, 20 different toppings, maybe 40 tops (pun intended)? Is running through a list of 20-40 items really going to tax your computer to the limit of its capacity? I think not.  It all smells like premature optimization to me (Donald Knuth calls it the root of all evil).


I don't need to benchmark something that has been proven to be bad for performance. We are scientists, so of course we don't act on intuition. Thus, we use algorithmic complexity theory to know what language constructs are bad for performance and which ones are not (CRUD on an ArrayList is O(n) complexity. I have no doubt you know about this). Since we know that O(n) grows exponentially, things can get out of hand really fast. We don't need to benchmark this to know it will be bad. I could reach into my Algorithms syllabus and show you how fast O(n) grows, but I don't need to because we both understand it.
I think it is important to think for yourself. It's not like Donald Knuth is a god of all truth.

Now, going back to the video, there were a few things I didn't like about how he implemented his ToppingDecorator and other classes. First, in ToppingDecorator, he called his field "tempPizza". That's lazy. Why "temp"?  Bad name.


As you know, programmers are lazy. A lot of programmers say being lazy makes you a good programmer, but I don't believe that.


Conceptually, I don't like the Pizza and Toppings example for Decorator. I always imagine a Decorator as being the same kind of thing as what it is decorating, like in the canonical example of a GUI Window being decorated (wrapped) by a ScrollableWindow which can be wrapped by perhaps a ResizableWindow, etc.  I've never liked the Pizza/Topping example, nor the other common Coffee/Additions example.  Pizza and the Toppings that you can put on it don't have that "same kind of thing" relationship in my mind.  Jalapeno, Cheese, Mushroom, Sausage are not special kinds of Pizza. They are just things I will add on top of a pizza.  Same thing for Coffee to cream, sugar, nutmeg, etc.


Yeah, but this video is made for laymen and the common layman doesn't understand GUI Windows. I believe this is why he used that example. Then, he argues in his other videos that people should trust him because he's been programming for 30 years (which I don't think is a valid argument). Now that you mention it, it is indeed a bad example; toppings are not a type of pizza. He also admitted in the comments that he wrote all of the code from the top of his head (which I think is bad practice). Well, anyway.


Thank you for your insight. It was helpful.
 
Junilu Lacar
Sheriff
Posts: 15801
264
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Ryan McClain wrote:
I don't need to benchmark something that has been proven to be bad for performance. We are scientists, so of course we don't act on intuition. Thus, we use algorithmic complexity theory to know what language constructs are bad for performance and which ones are not (CRUD on an ArrayList is O(n) complexity. I have no doubt you know about this). Since we know that O(n) grows exponentially, things can get out of hand really fast. We don't need to benchmark this to know it will be bad. I could reach into my Algorithms syllabus and show you how fast O(n) grows, but I don't need to because we both understand it. I think it is important to think for yourself. It's not like Donald Knuth is a god of all truth.


I still think that worrying about performance in this case is just a lot of hand-wringing for nothing.

Indeed, DK may not be the god of all truth but he is a well-respected authority in computer science and in particular, in the subject of algorithms. He was even awarded the A.M. Turing award in 1974 so I think it's safe to say that he probably knows a few things about O(n) and all that. Also, from what I've read recently, Knuth was actually quoting C.A.R Hoare on that root of all evil thing. Hoare, as you may recall, was the 1980 Turing Award winner. No small potatoes there either. Hoare definitely knows a few things about performance and computing. I don't know if a Turing Award pushes you closer to having deity status but I don't think either of them would worry too much about a few dozen pizza toppings possibly maxing out a JVM. Listening to people who know their stuff doesn't mean you're not thinking for yourself. My experience aligns with what he/they said about premature optimization, so I'm a believer.

As you know, programmers are lazy. A lot of programmers say being lazy makes you a good programmer, but I don't believe that.


Lazy comes in many colors. I prefer being lazy in the sense that I don't want to do things the hard way all the time. So I'll work hard not to do it the hard way. I'll spend five minutes trying to find a better name for one method just so I won't have to deal with the aggravation of coming back to that code later and not understanding it again because the name is misleading or just plain unclear. When it comes to trying to understand bad code, I'm lazy. When it comes to refactoring bad code, I'm eager as a beaver.

Yeah, but this video is made for laymen and the common layman doesn't understand GUI Windows. I believe this is why he used that example.


Not a valid excuse, IMO. Rather than making it easier for the uninitiated to understand, he only creates confusion and spreads misinformation. I imagine that most people who are learning to program can at least relate to the idea behind an email and its various decorators. That would have been a better example.

Thank you for your insight. It was helpful.


No problem, glad you found it so.
 
Well THAT's new! Comfort me, reliable tiny ad:
Thread Boost feature
https://coderanch.com/t/674455/Thread-Boost-feature
    Bookmark Topic Watch Topic
  • New Topic