Am I Doing it Wrong? | Hacker News Reader

Am I Doing it Wrong?

gist.github.com

38 pointsdenysonique40 comments

As someone who grew up on imperative and functional programming, I am loathe to add a class where a method will do. I hate having to open a dozen files to read logic that would be 30 lines if it were consolidated into one method.

I suspect the 'senior developer' has similar feelings about OOP to my own; he just sounds a lot more forceful about them. I don't think either position is necessarily 'right', but he's probably not a good person for you to work with.

Even as a functional programmer, I'd be OK with separating out comment functionality into a separate file, class or not. This code seems fine to me.

Rails is not my bread and butter however in iOS I find it'a almost always better to split things out into helper classes. Otherwise when you find that you end up needing that method in other parts of the app and then you either have to break it out into a class anyway or have two of the same method in two separate controllers I.E. NOT what you want.

I think the OPs code looks clean and follows OOP.

Surprised that nobody has mentioned yet: this code is stateful for no reason. There is no need for instance variables, a constructor, an object, or a class at all. It's just a function. Put it in a mixin, as a module function, in a helper, as a class method; there are lots of ways to do this that don't involve extra code, nounifying verbs, and state.

I'm about 5-6 years into my Ruby programming career (and 5-6 years into my OO programming career) and this is something that I've been wondering about lately. When I read Design Patterns for Ruby Russ Olsen seemed to generally write classes that require instantiation, but when I look at the work of other Rubyists that I admire they're often writing classes that are nothing but class methods.

I'm starting to get a pretty good feel for it, but what are the criteria that you use to determine if a class should be stateless or not?

The rule of thumb I use in most cases: Can it be written statelessly without contortions or large inefficiencies? If so, then I make it stateless. Depending what is idiomatic in the language and project, I will usually put it into a function instead of a class.

I find stateless code easier to reason about.

Classes exist to bundle state; if there's no need for state, there often shouldn't be a class in the first place. I tend to use module_fun for that, or just:

    module Foo
      def self.some_stateless_method
        ...
      end
    end

    Foo.some_stateless_method

    class MyController
      include Foo

      def index
        some_stateless_method
      end
    end

and so forth.

People think of modules as being mixins, but they're actually just function namespaces--which you can happen to bind into class contexts via include/extend.

I tend to use class methods where:

a.) The class exists, because you want state, and

b.) The function operates principally on, or is only useful in, the context of that class, but

c.) The function isn't bound to the object's state.

That last requirement is fuzzy, since the call semantics are different: self.class.foo is awkward, so I'll often write instance methods which don't actually depend on the state, just taking advantage of the local namespace.

I think it depends on the situation, and it's difficult to for me to describe. I try to think about it like this:

I am writing a class to represent a car. I need to know if the cars doors are opened or closed. The `open` or `closed` attribute is the state of the car, so it makes sense that the Car object would have that state.

Now compare that to the use of `LanguageDetector` from the example:

    LanguageDetector.new(@comment).set_language

Presumably this object is keeping a reference to the comment for it's internal state. When I'm dealing with an object, I like to say in my head "language detector, can you tell me the language for some text?". Nothing in my question gave the text a 1:1 relationship with the language detector. Since my question did not identify a relationship, I would not make the comment a part of the LanguageDetector object. I'd rather write something like this:

    detector = LanguageDetector.new
    language = detector.detect_language(@comment)
    @comment.language = language

What might the LanguageDetector actually want to have as it's state? I think maybe things like encodings, or particular languages it supports. The question I ask is "language detector, what encodings or languages do you support?", and the relationship becomes clear. For example:

    detector = LanguageDetector.new(:encodings => [...], :languages => [...])
    # etc

I hope that makes sense, and that I don't sound too crazy (talking to myself and whatnot).

As a note, I think this particular variant of SRP is adopted from Java, where there is no concept of a function not bound to an object. You might be tempted to use static methods for plain-old namespaced functions, but there's a second quandary: it is really difficult to manipulate anything but instances in Java. Since there are no first-class functions, you can't very well pass these functions around to configure other objects, etc. So you wind up with layers of OOP scar tissue around the problem: DI frameworks, Handlers, Managers, Strategies, Factories, etc.

"OOP scar tissue" is my new favorite phrase.

To add some real value, by this I mean that I've run into this crap a lot. In Java, sometimes I want the ability to have a function which returns other functions based on an input. This is doable in a variety of languages other than Java.

In Java, I didn't call it such, but I ended up writing a FactoryFactory. (I am not proud.) I had one layer of abstraction to produce Foo objects with a certain configuration, and a layer above that to parameterize the creation of a Foo-creator. sigh. Scar tissue indeed.

Yup, I've seen this as well, way too often. Rather than a function, you get a class, rather than arguments you get constructor parameters, and then the get stuck at what to name the method that actually does anything.

I think what you are saying is very valid. Personally, I tend to pull things out into stateful classes (e.g. CommentPoster, OrderMailer, ElephantCleaner, etc) for two reasons:

1. Testability. Isolating these bits of application logic and testing them on their own has proven less prone to issues than including it all in, say, controller methods or helpers, which get messy over time.

2. Swappability. Okay, sure, you may never need a second kind of CommentPoster, but you never know. Keeping things like this stateful and properly abstracted out saves me plenty of time as the app's requirements get more complex.

It's not exactly premature optimization if I always follow this pattern and know exactly how it will work. (Sometimes it's just an unused optimization.) To me, there is very little added complexity, and if anything it makes it a bit easier in the long run.

1. Testing is easier without state. I'm trying to illustrate that state and context (and namespaces) are separable.

2. Swapping implementations is easier: would you rather pass a function, or an object with a single function? This ties back in to testing: it's easier to create a test variant of a function than it is to create a new class.

Don't get me wrong: state is great, and I use it all over the place. I generally avoid it, though, unless the state is needed, simpler to understand, or more concise.

Yes. This is "stupid OO". The author knows how to "modularize" code by segregating it into classes, and hasn't learned to view functional decomposition in the same way. Yes, justinko is doing it wrong.

http://cdn.memegenerator.net/instances/400x/22419205.jpg

15 years experience (mostly java) here... I agree with senior developer. You're needlessly breaking the logic across multiple files to handle potential use cases that may never come up. It's not horrible, but if you have 100 of these controllers, now you need 100 service objects (or maybe 50 if you can group things). All to keep the extra three lines of code out of the controller.

Keep things in one place to start and then if the need for sharing or abstracting comes up I the future, refactor the code.

This approach should be reversed if you're shipping a library for use by third parties (abstract and isolate everything you can).

"This approach should be reversed if you're shipping a library for use by third parties"

I'm coming to believe more and more strongly that there should be a difference in the way writing library code, and writing code that uses libraries, is approached. (This applies to the use of design patterns, abstractions, and various other "best practices.")

In many cases, there's a big difference in both programmer quality and the amount of time spent on each line of code between library code and application code. And worse, most of the time, when programmers come across good code, it's library code, not application code. This leads programmers to think that library code--with its multiple abstractions and design pattern complexity--is the right way to do things, and so then then apply this approach, quite inappropriately, to their application code.

Sorry, it's just horribly wrong: "The "senior developer", whom is the stake holder's right hand man, said this:"

"Who", not "whom". There's no reason to use "whom" at all any more unless you really know when you should. "Who" is acceptable at all times.

The code seems fine though... ;)

"Who" is acceptable at all times.

I was unaware. Is this just laziness slipping into common usage or has it always been the case?

It may have been laziness once but if anything slipped, it slipped a long time ago. I'd argue that a sentence like "Whom are you talking to?" would sound pompous and old-fashioned to most Anglophones (and not solely because it ends with a preposition).

Here's a heuristic: Verbs don't belong in class names. Maybe in module names. But even then.

Any good counterexamples?

command objects are usually named as imperative verbs.

Yes, I agree. Naming a class with a verb is a good signal that you're trying abstract the wrong thing. If you want to move certain responsibilities outside the object, you make a "Handler" class that is in charge of that behavior and listen to appropriate events.

How I would approached it: refactor the logic in FacebookHandler/TwitterHandler, make the Comment object send an event when one is added, make the handlers listen and do their magic. Implement a common interface for grabbing the Tweet/FB post text. Done. Now all your models can post to Twitter/FB with a common interface.

EDIT: For clarity

I stated this in a different comment, but, to repeat myself, I would call this class by what it is ("CommentPoster") instead of what it does ("PostComment").

  CommentPoster.new(@comment).post

Looks slightly better to me.

I don't know about wrong, but I sure wouldn't do it that way. Adding a new class makes everything more complex for the next guy to figure out, and doesn't actually solve any current problems. It just moves the code somewhere else.

While this example is not an abomination, the desire to "clean up" code like this can be indicator of a compulsive over-builder - and that can extract a heavy price down the road. So the sr dev is right to be cautious.

Recent commenting activity was prompted by DHH's tweets today. https://twitter.com/dhh/status/216242159496609793

And here's his response to the gist: https://gist.github.com/2838490#gistcomment-356060

Meh, the "YAGNI" citation by @dhh is pretty weak IMO. YAGNI is a guideline, a heuristic, not an absolute inviolable law of nature. And that's what experience buys you as a developer: the wisdom and insight to anticipate things that are likely to be needed, and places where code should be structured a certain way now, to accommodate future changes.

What we need is a new acronym (well, maybe somebody has probably already coined it, but whatever) YPNGTNI: You're Probably Not Going To Need It.

Personally, I think DHH is following YAGNI as a guideline correctly in this case.

As long as the code is easy to refactor - and in this case, it's a piece of cake - I think you should err on the side of YAGNI.

Wouldn't this potentially muddy up his tests though? Now he's creating a lot of mocks for a simple create action. Not only that, but should the create action not be called "create_and_maybe_send_to_social_networks"?

The CRUD metaphor doesn't map well to an application's network API -- which is what controllers/actions are, after all. Like if there's some code that gets executed when you click the "create comment" button, that's how you name it. Doesn't matter that the code creates a comment and maybe posts to social networks and some other things.

big picture tangent: if you've gained enough experience/wisdom to accurately predict needed features, you should encapsulate them in a product (or library/component/framework/plugin etc) to be sold/open sourced. In this way, you should never be able to anticipate things likely needed, unless you are writing such a product. :-) Not entirely facetious, as this process does actually happen as features get pushed down into standard libraries, languages, operating systems, hardware (e.g. video decoders). It's a similar idea to something attributed to Turing http://www.turing.org.uk/book/extracts/ext6-326.html:

  There need be no real danger of it ever becoming a drudge, for any processes that
  are quite mechanical may be turned over to the machine itself.

He also brought up http://www.naildrivin5.com/blog/2012/06/10/single-responsibi... - and I had a hard figuring out if it was meant as satire or not.

"class PostComment" that already sounds wrong if you think about it.

He was correct in that posting to Twitter/Facebook should be refactored, but wrong in the way he actually handled it.

I would have approached it by sending a "new comment" signal when a new comment is inserted in the database, and have the specific logic for posting to Twitter and Facebook elsewhere. I don't know if Rails has support for that, and maybe that's the disease because most Rails codebase I've come across seem to just stick to whatever is built-in instead of thinking outside of the box a little for the sake of better design.

yeah I'd write my own class/service to handle facebook/twitter/linkedin/etc.

I'm feeling a little guilt for being amused that someone got fired from a Ruby job for writing 20th century Java.

Maybe I'm stupid, but why do I need to create a new object so that I can do four things in a sequence?

And these four things have different failure and performance characteristics - so maybe we should not try hard to unify them.

I would call this class by what it is ("CommentPoster") instead of what it does ("PostComment").

  CommentPoster.new(@comment).post

In this case, it might be a premature optimization, but I find that classes like this are a lot more testable than controller logic. One thing, though. I would pull lines like this out of CommentPoster and put them back into the controller:

  LanguageDetector.new(@comment).set_language
  SpamChecker.new(@comment).check_spam
  CommentMailer.new(@comment).send_mail

This keeps the important steps a bit more mutable.

If you cannot follow the code standards of a project (adding a totally new way of structuring the code) and cannot restrain from throwing in new depencencies/frameworks like Ember instead of using jQuery as specified, then don't be a contractor. You'll waste your clients money and be frustrated.

Reply on news.ycombinator.com