7

In the code sample below the Rental object is using Movie's fields to do a switch statement. Martin says : this is a bad idea but he does not give any deeper explanation as to why ?

Of course, you can say that this means a lot of coupling between Movie and Rental, or that Movie is badly encapsulated but those are just (almost meaningless) words, again, they don't explain the why? They don't go to the root of the question, so to say. They don't provide much understanding.

What I would like to know if there is a deeper, more general design principle why having a switch on Rental is a bad idea ?

In other words, how does Martin knows that the switch statement is on the wrong object ? Where does he get this insight from ? What is his thought process ?

enter image description here

Improve this question
6
  • 2
    I would imagine that mister Fowler has explained this if you turn the page?
    – A Boschman
    Jan 26, 2014 at 4:40
  • 1
    On the next page he is already talking about the how's of the refactoring and not the why's anymore. "Bad idea" is all the explanation there is which surprises me as well.
    – jhegedus
    Jan 26, 2014 at 4:43
  • 1
    It actually wants you to avoid switch statements based on data types. Each of those types should be an object, and the task should be performed by the each class instead. The class for new releases should give the charge for new release, etc. Keep the knowledge specific about a class in the class. By doing so, you can easily add new classes without having to alter logic in the class's users.
    – ikegami
    Jan 26, 2014 at 4:46
  • In particular, this should be refactored to three derived classes, like RegularMovie, NewReleaseMovie, and ChildrensMovie, and the logic in each case should be performed in the derived class overload of getCharge.
    – John Saunders
    Jan 26, 2014 at 4:47
  • @ikegami, your comment reminds me of the liskov substitution principle, which might be violated by the switch statement because then the Rental's behaviour depends on the concrete type of the Movie object.
    – jhegedus
    Jan 26, 2014 at 4:48

5 Answers 5

Reset to default
20

What happens when Movie has a new type added to it like Movie.DISCOUNT?

Of course, you can say that this means a lot of coupling between Movie and Rental, or that Movie is badly encapsulated but those are just (almost meaningless) words, again,

They're deeply meaningful words. They mean that when you change Movie, you now get to change Rental too since they're tightly coupled. Worse yet, in a language like Java there's nothing to tell you that this switch statement needs to be modified (though most IDEs do, to be fair). So if you don't modify the switch statement, you now have a bug that's giving people free movie rentals.

The primary focus of program design (and by extension refactoring) is optimizing code around the question "what happens when this changes?". That's the thought process that will help you understand many of the motivations in the book.

Improve this answer
3

There are two issues:

  1. Since the priceCode is part of the Movie class, the switch should likely be located within the Movie class. That localizes the work in the switch to the class which define what a priceCode is. If the Movie class were to decide to return different codes, then it is in the best position to modify the switch.

  2. The switch is probably not the only thing in the Movie class which depends on the priceCode. This would suggest creating derived classes of the Movie class, one per priceCode. Each would have its own overridden version of getCharge. This overridden method would not only have the separate algorithms which are currently in case statements, it could also depend on data that is specific to the individual derived classes.

Improve this answer
1
  • 2
    So basically a more general way of saying your point 1) would be : put the logic into the class where the data is on which the logic mostly operates. This really sounds like a basic OO principle because where else would you want to put such logic after all ? Perhaps this is so basic and commonly used OO principle that Martin took it for granted that the reader knows it so he did not even mention it.
    – jhegedus
    Jan 26, 2014 at 5:04
2

I can only agree with that if PriceCode is another object. In this case, it would be more appropriate to make the switch like PriceCode.REGULAR and not Movie.REGULAR for example. So Martin would be talking about Movie having constants defined on PriceCode, which looks like bad design.

Or maybe he's idea is to think on classes with very low coupling, for example, you want Rental to work with another class compatible with Movie but that is something else (for example, you want to rent a CD instead of a Movie).

Probably he's talking about the second option.

Improve this answer
2

The core concept of OO is to encapsulate the operations close to the data. Fowler's idea is that movie should decide itself how much it is worth (may be through inheritance into multiple movies?).

I do not completely concur with Fowler's idea:

  1. There are other ways to deal with this. What if 'rental policy' is an object in itself? Movie is supposed to represent the purest data about the movie, and not how a rental company considers a movie to be a rentable object.
  2. This does not work in enterprise applications: data gets serialized, transformed, dissected, split, joined... ensuring Fowler's idea would be hard.
  3. What is the unit of encapsulation? Is it better to consider renting as a composite? This composite owns these data elements of movie and rental policies, and offers a service that would be the single point of decision making.

Many a times, Fowler's ideas are usually best suited for teaching how to build small applications. These principles should in turn help build clear thoughts of building larger applications that contain composites.

Improve this answer
1

Why is relying upon the properties (or fields) of a another class to make a calculation a bad idea? The answer is inherent in the title of the book: refactoring. If changes are made to that class, the scope and meaning of the data can change. If a new movie type/price code is created, this code also needs to be adjusted.

Which points out the more precise problem -- relying upon a enum/type in another class. If Rental used a Renter class that had an age, and offered discounts based upon age, that wouldn't be such a problem. Age is unlikely to change. If on the other hand, our hypothetical Renter class had a Renter.Type (corporate, family, individual, minor, senior citizen), that would be much more likely to get modified, thus breaking Rental.

Improve this answer

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.