

Am I Doing it Wrong? - denysonique
https://gist.github.com/2838490

======
gwillen
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.

~~~
kenrikm
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.

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

~~~
omgsean
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?

~~~
aphyr
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.

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

~~~
mjs
"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.

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

~~~
damoncali
_"Who" is acceptable at all times._

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

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

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

Any good counterexamples?

~~~
hcarvalhoalves
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

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

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

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

~~~
mindcrime
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.

~~~
icebraining
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.

~~~
omgsean
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"?

~~~
bluesnowmonkey
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.

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

~~~
hcarvalhoalves
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.

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

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

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

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

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

