Hacker News Reader

Comment by tptacek | Hacker News Reader

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.

View on HN

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.

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.)

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.

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.

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!").

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.

Reply on news.ycombinator.com