The Secret To Rails OO Design | Hacker News Reader

The Secret To Rails OO Design

blog.steveklabnik.com

87 pointsddagradi33 comments

I don't know, Steve, it looks like you're creating classes with long scary names just to "encapsulate" Ruby one-liners; your whole DictionaryPresenter class (for instance) boils down to:

  Post.all.inject(Hash.new {|h, k| h[k] = []}) 
    {|hash, post| hash[post.name[0]] = post; hash }

You might be right that the forest of classes you're creating will be useful in the long run. But this is exactly the slippery slope that the Gang of Four disciples fell down in Java.

I think using the phrase "Plain Old $X Object" might be a warning sign.

Any time you write a post talking about learning OO, and your example class hierarchy includes classes that do nothing but wrap things that are already idiomatic in the language, expect your friends and those who care to jump on you to find and squash the radioactive object bees that are trying to burrow their way into your spinal column.

This. A class whose name started out as an extremely abstract verb (present) having just one method (which might as well have been named :call) is a code smell, a holdover from languages that required this ceremony just to pass around and use higher-order functions. I think the furthest I would have gone is a :display_category getter in each model, and let the caller decide which collection type to end up with.

I had similar thoughts but from the other direction - yes, this is what Ruby (and/or Rails particularly) seemed to want to avoid. "We don't need all of these classes, look how simple this is! It's not like Java with all of these patterns, etc."

But actually I don't think there's much wrong with this. It is OO. It's not domain object oriented, it's just OO. you're creating an object which has a single responsibility - to calculate a sort order, or to take something and present it.

While GoF disciples may well have made many mistakes, this doesn't mean that this kind of OO design which emphasizes separation of concerns is wrong. It may look more complex, and perhaps it's a premature refactoring - we don't know out of context. But these classes are doing more than "just wrapping" something. They're promoting the concept of "displaying" or "sorting" or "calculating a rate for something" to a first class concept in your system, and in the process making it easier to change the implementation and to test in isolation. They're not from the problem domain. But they make sense in your program domain. And that's fine (if not abused).

Edit: (as sibling reminded me of a point to make). Some of these classes could be functions if your language supports that. But they don't have to be, if making them classes makes them easier to work with, test and understand.

In this case, he's made a full class when all he needed was a module to provide a namespace. In the grand scheme of things that's not a huge mis-step, but it communicates the wrong idea to the reader, in my mind.

Later he makes the same mistake, and then proceeds to refactor the function into private helper methods with a public interface.

(I'm assuming/hoping he's finding the feedback productive.)

It's true: There is nothing less fun than chasing injected dependencies around from place to place in the code, only to discover that they all boil down to one line that you could have just... read.

Skimming through this essay, it looked as if the second half might be more convincing than the first, because there the logic being refactored was significantly hairier than one line.

The second half of this post made me think about _Style: Toward Clarity And Grace_, the (geekiest ever) writing book Richard Gabriel recommends (and it is great). The first fifth of the book does little else but rail against "nominalization", which is the bad writing habit of turning verbs into nouns, which abets flabby passive voice writing.

Isn't that exactly what Steve's doing here?

Combining the flexibility of Ruby with one of my favorite patterns from "Working Effectively with Legacy Code," we can take complex computations and turn them into objects.

AAAGH! BEES!

Look at Norvig code. I'm struck by how he just gets to the point. "Here's how I'm modeling my data. Here's a few functions that work with the model: whack, whack, whack! Here's 'main'. Boom, I solved Soduku."

"TurnaroundCalculator" doesn't even need state. Why is it a class with a bunch of private methods? Sure, this code doesn't belong in an AR model. So stick it in a function!

Yeah, I'm convinced. The passive-voice metaphor alone is difficult to resist.

The second example turns 18 lines of pretty simple code to 48 lines of function soup. At least to this reader's eyes.

(Added): Granted, many of the 48 is whitespace, but it still moves logically related code further away from eachother.

Your general point about a one-line may be well made but your one-liner doesn't do what Steve's code does. Idiomatic or not, your dictionary will only have letters that correspond to posts' first letters.

The OP has the right idea but the wrong examples.

Did you read this:

  DictionaryPresenter.new(PostCategorizationPolicy,
                          Post.all).as_dictionary

Yeah, so that's getting a bit long. It happens. :)

STEVE! LOOK OUT! BEES!

So Steve wants an index by letter for posts. Building an index by letter for a collection in Ruby is practically a core function of the language. Generalize my comment above to, "don't wrap core functions of the language up in classes; do them in idiom".

Sure and I agree with your point about his example - it's not a good example.

Did you read this:

    We've already added a new feature: any collection can now be displayed as a dictionary

I think this a very weak motivation for creating a new class!

When you start writing ThreeCamelcaseWords for your class names, it's probably time start questioning the design.

GoFites do this because they're Scared Of Code. Don't be afraid of code. Sure, don't poop it all over your files, cargo culting it from place to place. But also don't file it all away into a necronomicon to be conjured by name like a demon ("I Ankh-f-n-khonsu::Base, SET OVER THOSE THAT WOULD RENDER DICTIONARIES ORDERED BY FIRST LETTERS!").

I think of these as policy objects, and they only make sense when you have choices. That is, the benefit is not from pushing the logic into a new class. The benefit is from removing a switch on a policy flag, and using the runtime dispatching that the language provides. That is, instead of writing the branches yourself and keeping the current policy as a flag, class names are your policy flag and virtual methods are your branches. It pushes low-level code (if state A, do this, if state B, do this...) into the language runtime.

Personally, I don't make policy objects until I have two options. If I have one option, I hardcode everything because I want to do the minimal amount of design necessary. I only add in the abstractions when they're necessary.

In his post, this benefit was mostly an aside - "and have them be different" while showing one other option. Pushing logic into classes is not desirable in itself. As you pointed out, it adds layers of abstraction onto what can be conceptually simple tasks. But when you're able to remove some of your own machinery and rely on existing machinery, it's a win.

Although I do most of my work in C++, and I often have to create function objects - C++ has free functions, but it's often easier to pass around function objects. In his examples, I think PostCategorizationPolicy and UserCategorizationPolicy would be better served as functions.

When policies are themselves complex, and when they can be generalized across domains, it sometimes makes sense to factor them out into libraries. And sometimes, the cleanest interface you can come up with to a library is a class hierarchy.

But oftentimes, this approach of creating a policy class hierarchy as soon as you have more than one choice turns out to amount to nothing more than nominalizing the "if" statement.

Remember also that the rules of the game in Ruby are very different from C++. Things that make plenty of sense in C++ make zero sense in Ruby, where functions are effectively first class variables.

I really can't speak for what's best in Ruby, having never programmed in it. And my Python programs almost never have classes - just functions operating on lists and dictionaries.

As I said, I can easily see that functions would make more sense in his example. This is even true in C++, it's just that the "functions" often are objects with operator() defined. Once local state gets involved, though, they have to become objects.

I have nothing against if statements, but I know when I'm writing algorithmic code, and when I'm writing machinery code. Type fields is machinery code, and I try to avoid it.

Which then, if I'm not mistaken, boils down to:

  Post.all.group_by {|post| post.name[0] }

He wants to enumerate over the empty lists for first letters not used by posts too, apparently.

what i found odd about the original, that's clarified here, is that it's not really separating a concern. when, for moderate amounts of data, for nontrivially complex applications, you want to improve the performance of a call like this, you'll be digging into arel and lazy enumeration, both of which depend on how you access the datastore via activerecord.

What's not mentioned here is how you decide not to extract something.

I think it's true that there is a class of novice programmers who will always just use whatever frameworks they are given, and write things out inline, never searching for a better way because they just don't have the natural curiosity (or they've never been shown the light) to seek out the better ways.

It's also true that to master a programming language you have to practice using all its abstractions. Ruby has a very pleasing and simple way to break down concerns, and clearly this is a tool you need in your box to write great Ruby code.

The true level of mastery is when you understand the cost-benefit of all this. For instance, breaking everything down into one-line methods is only of benefit if those methods are a non-leaky abstraction and make conceptual sense on their own. It's not inherently true that a 10-line method is harder to read—one ten-line method is much shorter than 10 one-line methods.

The effect is even more powerful with classes. Every class file you add is adding to the overhead of understanding the program. In order for that to be beneficial, you have a greater-than-linear improvement in program power and flexibility. Even if some extraction seems reusable, it might require tweaks every time you attempt to make a new use of it, or it might be a leaky abstraction that introduces bugs that cost you time down the line. What makes the true master are not principles about how long methods and classes should be, but rather how to factor the logic in such a way as to maximize re-use and conceptual integrity.

Finally, you must always be cognizant of the YAGNI principle. In a Rails app, you are writing code for a discrete problem domain where reusability may be limited. Taking the time to factor things truly beautifully is more worth it at the Gem/Framework level than the application level because there is much greater opportunity for reuse.

To those who are attacking this article. I think, Steve unfortunately has chosen a somewhat bad example to illustrate a very good point.

I have seen countless examples of complex payment logic shoved down into User model (or CreditCard model), because developer did not had the foresight to see that Payment logic does not belong in User or in CreditCard model.

About functional or using higher order functions - yeah go for it. The great thing about Ruby I think is - you can still use best of OO patterns with minimum amount of heavy lifting and sprinkle blocks and lambdas - resulting in pretty neat design overall.

Also, functional code != good code sometimes (http://www.drmaciver.com/2008/08/functional-code-not-equal-g...)

Steve isn't talking about which model code belongs to. Nobody is attacking the idea of factoring code out of models. But I'm attacking the idea that the place for any refactored code to go is into its own class.

But I am not talking about which model code belongs to either. I sm saying, the Payment logic probably does not belong in either of the models (User or CreditCard) and should be in its own class and should have an well defined interface - independent of how User is stored or how CreditCard is stored. And I think, thats exactly what steve is talking about.

I think there may be object bees on you. Just because "logic" doesn't belong in a model class doesn't mean it needs its own class. In Rails, you already have three good places to move code like this to:

* The controller class, to generate a populated instance variable used by a view

* The helper class, to transform a general-purpose instance variable into the variable you need in your view

* As a free function or static method anywhere in lib/, particularly if you could conceivably re-use the code in another project.

The issue here is nominalizing all the logic in your code; besides creating unnecessary indirection (indirection is the coin of the realm; spend it wisely), it also creates One More Thing everyone has to remember to use your code: how to instantiate your object.

Just write functions. People have built and maintained things far more amazing than blog software in C using nothing but well-named functions. What Would Norvig Do?

Helpers.

Enjoyed the post, but Rails also provides helpers to 'help' you with view specific logic. Granted, this might not be as fun as a plain Ruby class, but I've used helpers extensively on various projects and whenever I join an existing project, it's one of the first places I look with dealing with the view layer.

One of the nice Rails conventions.

Perfect response. My thoughts exactly. The only thing I would add is if its reusuable, document the method! If this is part of the apps "framework", document it! I can't count how many times I've run into basic app helpers with undocumented option hashes. Want your code to be reused? Make using it easier than rewriting it.

This code he calls good:

    DictionaryPresenter.new(PostCategorizationPolicy, Post.all).as_dictionary

Well, it smells like typical java overengineering. Why stop at that, let's go full way to:

    DictionaryPresenterFactory.new(PostCategorizationPolicyStrategy, Post.collect(CollectionStrategy::All)).present

In this blog post, the author is reifying concepts and improving separation of concerns(1). This is basic software development. Which makes me wonder why the OP got the impression this kind of thing was "secret"?

(1)The usual warnings about premature optimization apply.

One thing that people seem to forget about object orientation: when you're not actually using an object to represent state, but you still want to inherit and/or separate functionality, there is a simple way to do it while avoiding the creation of a bunch of junk objects that do nothing besides waste heap space and get garbage collected on the next cycle:

STATIC/CLASS METHODS.

A minor tangent, but do db tables whose models use `include` instead of inheriting a base class count as POROs?

from an organizational pov, where do these poros go in the structure. Would you put them in the model folder ?

I always liked the term PORC (Plain Old Ruby Classes) for the Ruby analogue of a POJO

Doesn't it worry you that you're working in an environment that dedicates so much time to filing code away into objects that it needs a name for the idea of an object that doesn't derive from one of the framework classes?

An antidote to this mentality:

http://norvig.com/design-patterns/

Reply on news.ycombinator.com