Nikolay Sturm's Blog

Musings about Development and Operations

A Critical Review of Practical Object-Oriented Design in Ruby (Chapters 1 to 3)

| Comments

Sandi Metz’ Practical Object-Oriented Design in Ruby is praised as one of the best books on object-oriented design for ruby developers (15 5-star reviews on amazon as of this writing). When I started reading it, however, I was quickly irritated by some of her advice. I want to take the time and focus on these controversial topics, in the hopes of learning what I might have missed or why it all actually makes sense.

This is not a slating review, however. I enjoy reading the book and found some helpful information in the first chapters. It is certainly worth its money.

Chapter 2: Creating Classes that have a Single Responsibility

If the responsibilities are so coupled that you cannot use just the behaviour you need, you could duplicate the code of interest. This is a terrible idea.

Although this advice makes sense on first reading, I don’t agree with its tone. I believe in the importance of context for decision making. IMHO, it often makes sense to tolerate a certain amount of code duplication in order for better abstractions to emerge. Abstracting prematurely, might lead to the necessity of rewriting the abstraction later, which is explictly what good design set out to prevent (see chapter 1).

Hide Instance Variables

In this section, Sandi argues for hiding all instance variables behind getters, even for class internal use. This would simplify future code changes if you ever need a derivative of an instance variable instead of the variable itself.

I totally don’t get her argument. A class’ code is usually kept in a single file, so changing an instance variable to a method call is trivial, even for plain text editors. Wrapping each instance variable in a getter looks more like over-engineering to me.

Furthermore, an instance variable gives a name to a value. When I need a derivative of that value, I would expect for the name needing to change as well.

I prefer keeping my code as simple as possible, using instance variables for simple data and extracting emerging concepts (derivative values) into properly named helper methods through refactoring.

Overall, I perceive chapter 2 as contradictory. On the one hand, Sandi argues for deferring decisions (e.g. on page 32: Any decision you make in advance of an explicit requirement is just a guess. Don’t decide; preserve your ability to make a decision later.) but on the other hand I hear her arguing for writing code in anticipation of future requirements.

Chapter 3: Managing Dependencies

Isolate Vulnerable External Messages

In this section, Sandi tries to reduce coupling to an external interface, by extracting a method call to an external object into its own method.

While I do see some value in this technique, I don’t buy her argument. In a big class with many calls to the same external method with many arguments, it might make sense to extract that method call, but I consider having such code bad design that needs to be fixed. I aim for small classes with small interfaces. In case of interface changes, search and replace is simple enough.

To stay with her example, I even consider extracting wheel.diameter bad design, because it hides the information of which diameter we are dealing with. It reduces legibility.

Use Hashes for Initialization Arguments

In order to reduce coupling to method argument order, Sandi proposes the use of argument hashes.

Initializing, for instance, a value object with an attributes hash makes perfect sense to me. However, I cannot follow her in defaulting to argument hashes for all methods. I am with Robert Martin, who basically said less is better. If I have a method with more than 2 arguments, the method smells. The solution in this case is to refactor the interface, so we get by with one or two arguments, in which case the order isn’t much of a problem.

While Sandi describes most problems of her approach, which gives the alert reader a hint that using argument hashes might be more problematic than it looks like at first, she misses the important point of key validation. If you use argument hashes and mistype a key, no error will be thrown.

GearWrapper isolates all knowledge of the external interface in one place and, equally importantly, it provides an improved interface for your application.

I couldn’t agree more with the idea of defining an application specific interface on top of an external class to decouple code. I was irritated, however, when I read Sandi just wrapping object instantiation and justifying it with the use of an argument hash. Why would you stop there and not move on to implementing a complete interface? Why just wrap the initializer? That doesn’t make any sense to me and is another case of seeming self-contradiction.

That’s it for now. If I find more controversial recommendations in the upcomming chapters, I’ll collect them in another blog post. As I don’t consider myself an expert developer, I might have missed some important aspects that shine a different light on Sandi’s recommendations. Feel free to enlighten me in the comments! :)