Instantly share code, notes, and snippets.

Embed
What would you like to do?
Am I doing it wrong?

Dear Rubyists,

I just lost a contract because of my code in a Rails project.

The specific code in question is related to a "posting a comment" feature. Here are the details:

In this project, "posting a comment" does not simply entail inserting a row into the database. It involves a procedure to yes, insert a row, but also detect its language, check for spam, send emails, and "share" it to Twitter and Facebook. I believe this algorithm should be encapsulated. I do not believe it belongs in a controller or a model. I do not believe Active Record callbacks should be used.

The "senior developer", whom is the stake holder's right hand man, said this:

All that's being accomplished is moving a few statements from the controller to the a new class and adding even more code to support the new construct. We don't have the problems that this would solve like comments being posted from multiple controllers.

What do you guys think? Am I doing it wrong?

Please leave a comment, good or bad. My motivation to continue programming is at an all time low.

Thank you.

# app/use_cases/post_comment.rb
# Called from the "create" action in a controller
class PostComment
def initialize(user, entry, attributes)
@user = user
@entry = entry
@attributes = attributes
end
def post
@comment = @user.comments.new
@comment.assign_attributes(@attributes)
@comment.entry = @entry
@comment.save!
LanguageDetector.new(@comment).set_language
SpamChecker.new(@comment).check_spam
CommentMailer.new(@comment).send_mail
post_to_twitter if @comment.share_on_twitter?
post_to_facebook if @comment.share_on_facebook?
@comment
end
private
def post_to_twitter
PostToTwitter.new(@user, @comment).post
end
def post_to_facebook
PostToFacebook.new(@user, @comment).action(:comment)
end
end
@mguterl

This comment has been minimized.

Show comment
Hide comment
@mguterl

mguterl May 30, 2012

@justinko - I agree that this logic should be encapsulated and I also agree that it should not be in the model (especially callbacks) or the controller. Keep your head up and try not to let this bring you down.

mguterl commented May 30, 2012

@justinko - I agree that this logic should be encapsulated and I also agree that it should not be in the model (especially callbacks) or the controller. Keep your head up and try not to let this bring you down.

@avdi

This comment has been minimized.

Show comment
Hide comment
@avdi

avdi May 30, 2012

One class to represent one responsibility? Sounds right to me.

avdi commented May 30, 2012

One class to represent one responsibility? Sounds right to me.

@nickjones

This comment has been minimized.

Show comment
Hide comment
@nickjones

nickjones May 30, 2012

I also agree that it should be encapsulated. I would have also looked at using a background queuing mechanism to schedule the Twitter and Facebook posting to occur asynchronously from finishing this method.

nickjones commented May 30, 2012

I also agree that it should be encapsulated. I would have also looked at using a background queuing mechanism to schedule the Twitter and Facebook posting to occur asynchronously from finishing this method.

@mperham

This comment has been minimized.

Show comment
Hide comment
@mperham

mperham May 30, 2012

I think your logic is reasonable and the code looks great. I also try to avoid premature architecture until something is really necessary but I don't know if that may or may not be the case here. Remember though that when you are a contractor, you're a hired gun paid to work on their system. Take advice and learn but if they ultimately can't compromise, it's best to move to a contract where you can work with the team and your opinion is respected.

mperham commented May 30, 2012

I think your logic is reasonable and the code looks great. I also try to avoid premature architecture until something is really necessary but I don't know if that may or may not be the case here. Remember though that when you are a contractor, you're a hired gun paid to work on their system. Take advice and learn but if they ultimately can't compromise, it's best to move to a contract where you can work with the team and your opinion is respected.

@joelmccracken

This comment has been minimized.

Show comment
Hide comment
@joelmccracken

joelmccracken May 30, 2012

It looks right. However, this is one of those places where adding an extra file adds more complexity. It certainly is not "wrong", though.

My guess is that the senior developer doesn't really "get" object oriented programming.

joelmccracken commented May 30, 2012

It looks right. However, this is one of those places where adding an extra file adds more complexity. It certainly is not "wrong", though.

My guess is that the senior developer doesn't really "get" object oriented programming.

@jcoene

This comment has been minimized.

Show comment
Hide comment
@jcoene

jcoene May 30, 2012

Definitely the right approach, I too would perform the external service work asynchronously.

jcoene commented May 30, 2012

Definitely the right approach, I too would perform the external service work asynchronously.

@jcasimir

This comment has been minimized.

Show comment
Hide comment
@jcasimir

jcasimir May 30, 2012

I think it's good, but am not surprised that someone who doesn't understand how it fits into the paradigm ("OMG what is a use_cases folder?!?!") would freak out a bit. "Architects", especially, are often scared of new ideas.

jcasimir commented May 30, 2012

I think it's good, but am not surprised that someone who doesn't understand how it fits into the paradigm ("OMG what is a use_cases folder?!?!") would freak out a bit. "Architects", especially, are often scared of new ideas.

@rwilcox

This comment has been minimized.

Show comment
Hide comment
@rwilcox

rwilcox May 30, 2012

It will be interesting when DCI really takes hold in the Rails community, and then the "Rails has too many abstractions" people have yet another abstraction to complain about ;)

rwilcox commented May 30, 2012

It will be interesting when DCI really takes hold in the Rails community, and then the "Rails has too many abstractions" people have yet another abstraction to complain about ;)

@jc00ke

This comment has been minimized.

Show comment
Hide comment
@jc00ke

jc00ke May 30, 2012

I'm doing the same thing right this second, and I'm keeping my job. It's not you, it's him. He's just not that into you, and that's OK. Use this case as a litmus test for your next client. If they think it's overkill, they're probably a sucky client. If they're cool with it, then you've found a good client.

My hunch is that if it weren't this (idiotic) reason it would have been something else, not driven by anything you did/would do.

jc00ke commented May 30, 2012

I'm doing the same thing right this second, and I'm keeping my job. It's not you, it's him. He's just not that into you, and that's OK. Use this case as a litmus test for your next client. If they think it's overkill, they're probably a sucky client. If they're cool with it, then you've found a good client.

My hunch is that if it weren't this (idiotic) reason it would have been something else, not driven by anything you did/would do.

@alindeman

This comment has been minimized.

Show comment
Hide comment
@alindeman

alindeman May 30, 2012

I think you're doing it right. Service objects like the one you created are showing up more and more in my Rails projects: they don't necessarily have any reason to depend on Rails, and definitely don't belong in the controller. I'm sure this object was much easier (and faster) to test as well because it is better factored.

alindeman commented May 30, 2012

I think you're doing it right. Service objects like the one you created are showing up more and more in my Rails projects: they don't necessarily have any reason to depend on Rails, and definitely don't belong in the controller. I'm sure this object was much easier (and faster) to test as well because it is better factored.

@pythonandchips

This comment has been minimized.

Show comment
Hide comment
@pythonandchips

pythonandchips May 30, 2012

Looks perfect to me. Definitely no need for this to be in a controller

pythonandchips commented May 30, 2012

Looks perfect to me. Definitely no need for this to be in a controller

@apotonick

This comment has been minimized.

Show comment
Hide comment
@apotonick

apotonick May 30, 2012

Dude move it to ActiveRecord::Base just to make sure all your domain objects can acts_as_commentable. What a bullshit, I've heard this point a million times "You're abstracting things to somewhere it doesn't belong to"... be happy you don't have to work with him anymore.

apotonick commented May 30, 2012

Dude move it to ActiveRecord::Base just to make sure all your domain objects can acts_as_commentable. What a bullshit, I've heard this point a million times "You're abstracting things to somewhere it doesn't belong to"... be happy you don't have to work with him anymore.

@jwo

This comment has been minimized.

Show comment
Hide comment
@jwo

jwo May 30, 2012

This code is appropriate, and I'd much rather have someone on my team that thinks about this, rather than splatting everything into Controller and Callbacks.

If the company looked at this code and decided against you because of it, you dodged a bullet by not wasting your time there.

jwo commented May 30, 2012

This code is appropriate, and I'd much rather have someone on my team that thinks about this, rather than splatting everything into Controller and Callbacks.

If the company looked at this code and decided against you because of it, you dodged a bullet by not wasting your time there.

@justinko

This comment has been minimized.

Show comment
Hide comment
@justinko

justinko May 30, 2012

Just to be clear everyone, the code that talks to remote services (PostToTwitter, PostToFacebook, CommentMailer, etc.) uses Sidekiq :)

Owner

justinko commented May 30, 2012

Just to be clear everyone, the code that talks to remote services (PostToTwitter, PostToFacebook, CommentMailer, etc.) uses Sidekiq :)

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston May 30, 2012

As everyone else had said, I vastly prefer the approach you took on this.

We don't have the problems that this would solve like comments being posted from multiple controllers.

...yet. Who knows what future problems will be easily solved by this more flexible, decoupled approach?

myronmarston commented May 30, 2012

As everyone else had said, I vastly prefer the approach you took on this.

We don't have the problems that this would solve like comments being posted from multiple controllers.

...yet. Who knows what future problems will be easily solved by this more flexible, decoupled approach?

@trptcolin

This comment has been minimized.

Show comment
Hide comment
@trptcolin

trptcolin May 30, 2012

Looks perfectly reasonable to me. I'd love to see this kind of code in a project.

Small nitpick: the controller knows about success/fail based on the #save! call raising or not, cool - the only possible wrinkle I'd see would be that if anything fails in line 17-22, the controller may want to know how handle that.

trptcolin commented May 30, 2012

Looks perfectly reasonable to me. I'd love to see this kind of code in a project.

Small nitpick: the controller knows about success/fail based on the #save! call raising or not, cool - the only possible wrinkle I'd see would be that if anything fails in line 17-22, the controller may want to know how handle that.

@garybernhardt

This comment has been minimized.

Show comment
Hide comment
@garybernhardt

garybernhardt May 30, 2012

I think he did you a service by filtering himself out. If you function anything like I do in contracting situations, that one would've left you feeling unfulfilled at best.

garybernhardt commented May 30, 2012

I think he did you a service by filtering himself out. If you function anything like I do in contracting situations, that one would've left you feeling unfulfilled at best.

@databyte

This comment has been minimized.

Show comment
Hide comment
@databyte

databyte May 30, 2012

👍

The hardest part is communicating the need for the design/code/feature. It can be a friendly recap of SRP or SOLID, an explanation of the downward spiral that can be AR callbacks or some slick tests that easily deal with that chain of commands. Then there's always the response "if you say so" while you downgrade the client and start looking elsewhere.

databyte commented May 30, 2012

👍

The hardest part is communicating the need for the design/code/feature. It can be a friendly recap of SRP or SOLID, an explanation of the downward spiral that can be AR callbacks or some slick tests that easily deal with that chain of commands. Then there's always the response "if you say so" while you downgrade the client and start looking elsewhere.

@RicSwirrl

This comment has been minimized.

Show comment
Hide comment
@RicSwirrl

RicSwirrl May 30, 2012

I think your code looks fine. It'd be interesting to see how the other guy would've done it too, for comparison. :)

RicSwirrl commented May 30, 2012

I think your code looks fine. It'd be interesting to see how the other guy would've done it too, for comparison. :)

@jcoene

This comment has been minimized.

Show comment
Hide comment
@jcoene

jcoene May 30, 2012

Good man :)

jcoene commented May 30, 2012

Good man :)

@solnic

This comment has been minimized.

Show comment
Hide comment
@solnic

solnic May 30, 2012

You're doing it great, I would do it in a similar way. Nowadays I tend to encapsulate domain logic in PORO objects. I do that to avoid too fat AR models, to speed up my tests and to follow best OO practices, especially following SRP. I usually create service-like objects that correspond to the real domain of my app, they "speak" the domain language, those are small, single-purpose objects that are easy to test and thus easy to understand.

Oh and that comment you quote here is a clear indication that the senior developer lacks basic knowledge about OO programming. It's not even funny.

solnic commented May 30, 2012

You're doing it great, I would do it in a similar way. Nowadays I tend to encapsulate domain logic in PORO objects. I do that to avoid too fat AR models, to speed up my tests and to follow best OO practices, especially following SRP. I usually create service-like objects that correspond to the real domain of my app, they "speak" the domain language, those are small, single-purpose objects that are easy to test and thus easy to understand.

Oh and that comment you quote here is a clear indication that the senior developer lacks basic knowledge about OO programming. It's not even funny.

@abernardes

This comment has been minimized.

Show comment
Hide comment
@abernardes

abernardes May 30, 2012

Dude, IMO it really sums up to this: if their senior programmer doesn't know what SRP is and/or why you used it in this case, you're probably better off without this job. Otherwise, it would be a constant source of conflict and frustration.

abernardes commented May 30, 2012

Dude, IMO it really sums up to this: if their senior programmer doesn't know what SRP is and/or why you used it in this case, you're probably better off without this job. Otherwise, it would be a constant source of conflict and frustration.

@bai

This comment has been minimized.

Show comment
Hide comment
@bai

bai May 30, 2012

Looks fine and... symmetrical. Thumbs up!

bai commented May 30, 2012

Looks fine and... symmetrical. Thumbs up!

@saturnflyer

This comment has been minimized.

Show comment
Hide comment
@saturnflyer

saturnflyer May 30, 2012

What is the controller's responsibility? It's not to control business procedures.

This posting of a comment is a complex procedure. Why, oh why, would you dump that in a controller method?

Breaking up your app like this allows you to grow and respond to change. But the senior dev is insisting that the software will never change... is that right? And I assume then he prefers that your tests be run through a controller requiring request setup rather than just unit testing the damn thing.

saturnflyer commented May 30, 2012

What is the controller's responsibility? It's not to control business procedures.

This posting of a comment is a complex procedure. Why, oh why, would you dump that in a controller method?

Breaking up your app like this allows you to grow and respond to change. But the senior dev is insisting that the software will never change... is that right? And I assume then he prefers that your tests be run through a controller requiring request setup rather than just unit testing the damn thing.

@bai

This comment has been minimized.

Show comment
Hide comment
@bai

bai May 30, 2012

@RicSwirrl I've seen an app that makes AR::Base connections establishment in views, uses mysql driver directly in views, and has 3,000-line controllers. Wanted to shoot myself in the face, hehe.

bai commented May 30, 2012

@RicSwirrl I've seen an app that makes AR::Base connections establishment in views, uses mysql driver directly in views, and has 3,000-line controllers. Wanted to shoot myself in the face, hehe.

@joelmccracken

This comment has been minimized.

Show comment
Hide comment
@joelmccracken

joelmccracken May 30, 2012

@bai sounds like someone was writing PHP in Rails

joelmccracken commented May 30, 2012

@bai sounds like someone was writing PHP in Rails

@mboeh

This comment has been minimized.

Show comment
Hide comment
@mboeh

mboeh May 30, 2012

Well, everyone knows that there's a finite number of classes available in the world. You don't want to waste those classes!

Seriously, though, this is the kind of code I try to write and I try to get people I work with to write. And this approach is the right one even if you never post a comment from any other controller.

mboeh commented May 30, 2012

Well, everyone knows that there's a finite number of classes available in the world. You don't want to waste those classes!

Seriously, though, this is the kind of code I try to write and I try to get people I work with to write. And this approach is the right one even if you never post a comment from any other controller.

@hebweb

This comment has been minimized.

Show comment
Hide comment
@hebweb

hebweb May 30, 2012

Simple:
As for coding, you used a pure OOP approach so your code is "correct".
As for code, I can understand that senior developer. he might understand OOP but believe that keeping the code inside is the way to KISS the code.
It's what I call "pragmatic programming". and although sometimes it's out of the strict conventions, and not an orthodox practice, it's the right solution.

hebweb commented May 30, 2012

Simple:
As for coding, you used a pure OOP approach so your code is "correct".
As for code, I can understand that senior developer. he might understand OOP but believe that keeping the code inside is the way to KISS the code.
It's what I call "pragmatic programming". and although sometimes it's out of the strict conventions, and not an orthodox practice, it's the right solution.

@ahoward

This comment has been minimized.

Show comment
Hide comment
@ahoward

ahoward May 30, 2012

'senior developer' is a fool. of course this should

  • be encapsulated as a conductor
  • be run in the background (guess what happens when twitter is down or the api limit hit)

this really shouldn't even be a question, hope you land a new gig soon

ahoward commented May 30, 2012

'senior developer' is a fool. of course this should

  • be encapsulated as a conductor
  • be run in the background (guess what happens when twitter is down or the api limit hit)

this really shouldn't even be a question, hope you land a new gig soon

@ahoward

This comment has been minimized.

Show comment
Hide comment
@ahoward

ahoward May 30, 2012

there are so many reasons doing this in the controller is suck-o-rama

  • unit tests hit the damn network and eat api limits. wtf?
  • untestable
  • 60s network timeouts in a save to the db (from user's perspective)

argh.

ahoward commented May 30, 2012

there are so many reasons doing this in the controller is suck-o-rama

  • unit tests hit the damn network and eat api limits. wtf?
  • untestable
  • 60s network timeouts in a save to the db (from user's perspective)

argh.

@raggi

This comment has been minimized.

Show comment
Hide comment
@raggi

raggi May 30, 2012

My issue with this, is that this should actually be your target resource, and it seems like if you have specifically put all of your model layer logic into just "Post" and "Comment" then your schema could potentially be refactored. You'll find your design more resource oriented (what rails folks often mean when they say "restful") if you actually model the resources you're creating.

The fact that this says "called from the create action in a controller" in the description seems to reinforce this. I'm not suggesting that this makes your clients complaints valid, as it sounds like your client would "dump" into a non-resource oriented controller too - also breaking the meaning of "create a resource". Maybe your resource is a PostComment already, maybe I'm wrong, but, adding constructs for stuff that does actually fit inside of resource oriented rest patterns could be considered excessive.

The additional abstractions for non-resource oriented actions/processes are fine, and I don't disagree with the principle. Classes that are literally just static functions with extra syntax wrapped around them aren't always necessary though, but there is a (possible but rare) cost of change argument here.

Finally - I wouldn't fire a contractor for this abstraction, but I would raise an eyebrow if I could refactor it to be simpler and more conventional, without impacting cost of change.

Out of interest, have you tried factoring this code in "the most simple and rails way you can"? Might be worth the learning exercise, regardless of the outcome.

raggi commented May 30, 2012

My issue with this, is that this should actually be your target resource, and it seems like if you have specifically put all of your model layer logic into just "Post" and "Comment" then your schema could potentially be refactored. You'll find your design more resource oriented (what rails folks often mean when they say "restful") if you actually model the resources you're creating.

The fact that this says "called from the create action in a controller" in the description seems to reinforce this. I'm not suggesting that this makes your clients complaints valid, as it sounds like your client would "dump" into a non-resource oriented controller too - also breaking the meaning of "create a resource". Maybe your resource is a PostComment already, maybe I'm wrong, but, adding constructs for stuff that does actually fit inside of resource oriented rest patterns could be considered excessive.

The additional abstractions for non-resource oriented actions/processes are fine, and I don't disagree with the principle. Classes that are literally just static functions with extra syntax wrapped around them aren't always necessary though, but there is a (possible but rare) cost of change argument here.

Finally - I wouldn't fire a contractor for this abstraction, but I would raise an eyebrow if I could refactor it to be simpler and more conventional, without impacting cost of change.

Out of interest, have you tried factoring this code in "the most simple and rails way you can"? Might be worth the learning exercise, regardless of the outcome.

@raggi

This comment has been minimized.

Show comment
Hide comment
@raggi

raggi May 30, 2012

I guess the shortest form of the above is: I'd probably start with a PostComment ""model""...

raggi commented May 30, 2012

I guess the shortest form of the above is: I'd probably start with a PostComment ""model""...

@joevandyk

This comment has been minimized.

Show comment
Hide comment
@joevandyk

joevandyk May 30, 2012

I'd have probably done PostService.post(user, entry, attributes). I don't think the instance variables are needed. But yeah, reasonable to extract the logic into a service class.

joevandyk commented May 30, 2012

I'd have probably done PostService.post(user, entry, attributes). I don't think the instance variables are needed. But yeah, reasonable to extract the logic into a service class.

@KellyMahan

This comment has been minimized.

Show comment
Hide comment
@KellyMahan

KellyMahan May 30, 2012

Your code is not wrong, and having it in the controller is just awful you are totally correct in this case. However I would attach the language and spam check as a call back method included from a module for each, unless for some reason you need to reject after creation instead of before creation. Everything else should be it's own class which runs through a queue system. Adding the job to the que I would trigger with a callback as well.

Just curious but what is your aversion to using active record callbacks in this case? If there are times where you need to create a comment without the triggers then I can see the use in using a separate class like this. But other than that what am I missing?

KellyMahan commented May 30, 2012

Your code is not wrong, and having it in the controller is just awful you are totally correct in this case. However I would attach the language and spam check as a call back method included from a module for each, unless for some reason you need to reject after creation instead of before creation. Everything else should be it's own class which runs through a queue system. Adding the job to the que I would trigger with a callback as well.

Just curious but what is your aversion to using active record callbacks in this case? If there are times where you need to create a comment without the triggers then I can see the use in using a separate class like this. But other than that what am I missing?

@batasrki

This comment has been minimized.

Show comment
Hide comment
@batasrki

batasrki May 30, 2012

Just to pile on the positivity, for once, this code is beautiful. It fits the explanation and shows everything clearly. I'd question why save a comment if it's spam, but that's a business decision, not a coding one.

Anyone who says that this code should be in the model or, OMFG WAT, controller, has no idea what they're doing. That person is a cargo-cultist at best, a goddamn idiot at worst. You're better off, my friend.

batasrki commented May 30, 2012

Just to pile on the positivity, for once, this code is beautiful. It fits the explanation and shows everything clearly. I'd question why save a comment if it's spam, but that's a business decision, not a coding one.

Anyone who says that this code should be in the model or, OMFG WAT, controller, has no idea what they're doing. That person is a cargo-cultist at best, a goddamn idiot at worst. You're better off, my friend.

@batasrki

This comment has been minimized.

Show comment
Hide comment
@batasrki

batasrki May 30, 2012

@KellyMahan, callbacks make it hard to reason about the code, as well as make the model much harder to test. You have to handle or somehow bypass callbacks for testing those cases where they're irrelevant to the actions happening. Also, returning false from a callback, whatever the callback might be, will cause save failure in the model. I ran into an example where an automated e-mail was being sent from a callback and an exception in the callback prevented the model from saving, even though all validation passed and data should've been saved.

Because of stuff like that, callbacks are probably best avoided.

batasrki commented May 30, 2012

@KellyMahan, callbacks make it hard to reason about the code, as well as make the model much harder to test. You have to handle or somehow bypass callbacks for testing those cases where they're irrelevant to the actions happening. Also, returning false from a callback, whatever the callback might be, will cause save failure in the model. I ran into an example where an automated e-mail was being sent from a callback and an exception in the callback prevented the model from saving, even though all validation passed and data should've been saved.

Because of stuff like that, callbacks are probably best avoided.

@nateklaiber

This comment has been minimized.

Show comment
Hide comment
@nateklaiber

nateklaiber May 30, 2012

I like it. The act of posting a comment can live outside of the request/response workflow of a controller. Encapsulating it into this object like this helps to ensure the atomic nature of posting the comment.

The one thing that stuck out to me is that you want to ensure the contract is met on your end.

  1. What if user or entry aren't not the expected ActiveRecord models you need? This would affect calls like comments and save!. You want to ensure you get the proper details you need in the constructor.
  2. What if attributes are not what you expect? I know the call to save! might raise an exception (depending on your validation rules) - but it would be nice to ensure attributes is a hash, has valid keys, and has provided you with the required keys necessary.

Ensuring that contract is met means you would have less errors in the rest of the stack.

Just a thought.

nateklaiber commented May 30, 2012

I like it. The act of posting a comment can live outside of the request/response workflow of a controller. Encapsulating it into this object like this helps to ensure the atomic nature of posting the comment.

The one thing that stuck out to me is that you want to ensure the contract is met on your end.

  1. What if user or entry aren't not the expected ActiveRecord models you need? This would affect calls like comments and save!. You want to ensure you get the proper details you need in the constructor.
  2. What if attributes are not what you expect? I know the call to save! might raise an exception (depending on your validation rules) - but it would be nice to ensure attributes is a hash, has valid keys, and has provided you with the required keys necessary.

Ensuring that contract is met means you would have less errors in the rest of the stack.

Just a thought.

@mboeh

This comment has been minimized.

Show comment
Hide comment
@mboeh

mboeh May 30, 2012

@batasrki Callbacks are still useful for handling persistence details (i.e. "this field should be persisted in this format" or "this field should have this value if not set explicitly"), although overriding #save is an option there as well.

mboeh commented May 30, 2012

@batasrki Callbacks are still useful for handling persistence details (i.e. "this field should be persisted in this format" or "this field should have this value if not set explicitly"), although overriding #save is an option there as well.

@garybernhardt

This comment has been minimized.

Show comment
Hide comment
@garybernhardt

garybernhardt May 30, 2012

@nateklaiber, Ruby will raise an exception if the objects don't respond to the right methods, and the validations should ensure that the attributes are right.

garybernhardt commented May 30, 2012

@nateklaiber, Ruby will raise an exception if the objects don't respond to the right methods, and the validations should ensure that the attributes are right.

@saturnflyer

This comment has been minimized.

Show comment
Hide comment
@saturnflyer

saturnflyer May 30, 2012

Characterizations of the senior dev are neither helpful nor well-informed.

saturnflyer commented May 30, 2012

Characterizations of the senior dev are neither helpful nor well-informed.

@deepfryed

This comment has been minimized.

Show comment
Hide comment
@deepfryed

deepfryed May 30, 2012

🤘 don't fret, you're way ahead of your peers in proper OO design. carry on.

deepfryed commented May 30, 2012

🤘 don't fret, you're way ahead of your peers in proper OO design. carry on.

@nateklaiber

This comment has been minimized.

Show comment
Hide comment
@nateklaiber

nateklaiber May 30, 2012

@garybernhardt

Example: Something goes wrong in the caller code, they pass in nil for user instead of a user object. The first error you would see is

NoMethodError: undefined method 'comments' for nil:NilClass

Compare that to getting a more descriptive exception on initialize. It's an Exception because the rest of the code won't work without a proper @user (line 12). Would you rather see the nil error?

@entry could be caught by the validation in the call to save! (don't know what those rules are based on this example).

Again, if something goes wrong in the caller, and attributes is nil and passed into the call to assign_attributes, you would get

NoMethodError: undefined method 'stringify_keys' for nil:NilClass

Personally, I would rather see more descriptive error messages than letting them fall down into Ruby or ActiveRecord. Wrap these up with unit tests to ensure your contract is met when edge cases are provided.

nateklaiber commented May 30, 2012

@garybernhardt

Example: Something goes wrong in the caller code, they pass in nil for user instead of a user object. The first error you would see is

NoMethodError: undefined method 'comments' for nil:NilClass

Compare that to getting a more descriptive exception on initialize. It's an Exception because the rest of the code won't work without a proper @user (line 12). Would you rather see the nil error?

@entry could be caught by the validation in the call to save! (don't know what those rules are based on this example).

Again, if something goes wrong in the caller, and attributes is nil and passed into the call to assign_attributes, you would get

NoMethodError: undefined method 'stringify_keys' for nil:NilClass

Personally, I would rather see more descriptive error messages than letting them fall down into Ruby or ActiveRecord. Wrap these up with unit tests to ensure your contract is met when edge cases are provided.

@garybernhardt

This comment has been minimized.

Show comment
Hide comment
@garybernhardt

garybernhardt May 30, 2012

@nateklaiber, I think you're advocating defensive programming, which I like at boundaries, but I don't like it within a system. I'd rather build the system so that bad data can't be produced, instead of allowing it to produce bad data and catching it later. That effort would be better spent fortifying the higher level pieces of the system so they can't produce nil.

garybernhardt commented May 30, 2012

@nateklaiber, I think you're advocating defensive programming, which I like at boundaries, but I don't like it within a system. I'd rather build the system so that bad data can't be produced, instead of allowing it to produce bad data and catching it later. That effort would be better spent fortifying the higher level pieces of the system so they can't produce nil.

@campbell

This comment has been minimized.

Show comment
Hide comment
@campbell

campbell May 30, 2012

The few times that I have been "separated" from a project, in 6 months I have been grateful that I'm no longer there. The signs of trouble were present, but I was too focused on solving the problems at hand. You'll look back on this & be thankful that you're no longer responsible for their technical debt.

Learn from the experience and use it to evaluate your future positions (as in should I go or should I stay?). Judging from your code, I don't think you have anything to worry about.

campbell commented May 30, 2012

The few times that I have been "separated" from a project, in 6 months I have been grateful that I'm no longer there. The signs of trouble were present, but I was too focused on solving the problems at hand. You'll look back on this & be thankful that you're no longer responsible for their technical debt.

Learn from the experience and use it to evaluate your future positions (as in should I go or should I stay?). Judging from your code, I don't think you have anything to worry about.

@jc00ke

This comment has been minimized.

Show comment
Hide comment
@jc00ke

jc00ke May 30, 2012

@campbell you can't say it like that. It's "Should I stay or should I go?" 🎸

jc00ke commented May 30, 2012

@campbell you can't say it like that. It's "Should I stay or should I go?" 🎸

@KellyMahan

This comment has been minimized.

Show comment
Hide comment
@KellyMahan

KellyMahan May 30, 2012

@batasrki an exception is precisely why i would use callbacks. In the spam module i would raise exceptions according to why the message is being rejected either for the user or for logging. And depending on what the need for language detection is, make rejections as well. If it's something that should never reject then it should also never fail, write it in such a way to log errors while rescuing the exception so the error can be fixed in the future.

Each case is different though and sometimes may require not using callbacks. But by never using callbacks you are throwing out a lot of very useful rails code that can save time and make for cleaner code.

KellyMahan commented May 30, 2012

@batasrki an exception is precisely why i would use callbacks. In the spam module i would raise exceptions according to why the message is being rejected either for the user or for logging. And depending on what the need for language detection is, make rejections as well. If it's something that should never reject then it should also never fail, write it in such a way to log errors while rescuing the exception so the error can be fixed in the future.

Each case is different though and sometimes may require not using callbacks. But by never using callbacks you are throwing out a lot of very useful rails code that can save time and make for cleaner code.

@nateklaiber

This comment has been minimized.

Show comment
Hide comment
@nateklaiber

nateklaiber May 30, 2012

@garybernhardt - Understood. I don't disagree with setting these at boundaries. I like to do both.

I am thinking about this object being used outside of the controller only (request/response). You could take a step up and ensure that the data is validated at the controller request level (proper objects and attributes hash), ensuring that only valid data will be provided to the PostComment model. You would have to protect all boundaries where this object could be used (maybe a rake task, maybe a background job, maybe through the console, maybe a separate API controller that uses it, etc). This is a possibility.

I like being more defensive with this object and ensuring the contract is met, rather than assuming the caller handles it in all contexts. Smaller
objects like this make it easier to have smaller building blocks all the way through the system.

nateklaiber commented May 30, 2012

@garybernhardt - Understood. I don't disagree with setting these at boundaries. I like to do both.

I am thinking about this object being used outside of the controller only (request/response). You could take a step up and ensure that the data is validated at the controller request level (proper objects and attributes hash), ensuring that only valid data will be provided to the PostComment model. You would have to protect all boundaries where this object could be used (maybe a rake task, maybe a background job, maybe through the console, maybe a separate API controller that uses it, etc). This is a possibility.

I like being more defensive with this object and ensuring the contract is met, rather than assuming the caller handles it in all contexts. Smaller
objects like this make it easier to have smaller building blocks all the way through the system.

@ahoward

This comment has been minimized.

Show comment
Hide comment
@ahoward

ahoward May 30, 2012

@saturnflyer comments are about 'senior developer' are meant to help OP keep contract, not be discouraged, or land next contract. i'll stand by my comment that firing some one over wanting to do good engineering is not smart

ahoward commented May 30, 2012

@saturnflyer comments are about 'senior developer' are meant to help OP keep contract, not be discouraged, or land next contract. i'll stand by my comment that firing some one over wanting to do good engineering is not smart

@saturnflyer

This comment has been minimized.

Show comment
Hide comment
@saturnflyer

saturnflyer May 30, 2012

@ahoward I'm not defending decisions made. I'm pointing out that characterizations of a person, about whom we have extremely limited knowledge, are not helpful in the discussion about the code sample here or the apparent decision to let @justinko's contract expire.
Argue that the decision is foolish, fine, but don't extrapolate that to mean the the person making the decision is a fool. The best outcome of any discussion about this decision is the change in behavior of a person, not in their humiliation.

saturnflyer commented May 30, 2012

@ahoward I'm not defending decisions made. I'm pointing out that characterizations of a person, about whom we have extremely limited knowledge, are not helpful in the discussion about the code sample here or the apparent decision to let @justinko's contract expire.
Argue that the decision is foolish, fine, but don't extrapolate that to mean the the person making the decision is a fool. The best outcome of any discussion about this decision is the change in behavior of a person, not in their humiliation.

@campbell

This comment has been minimized.

Show comment
Hide comment
@campbell

campbell May 30, 2012

Oh noes! RubyDramas.com reset?

@mperham nailed it with respect to the work environment. They pay, so they get to make the rules, but you can decide to stay or not.

campbell commented May 30, 2012

Oh noes! RubyDramas.com reset?

@mperham nailed it with respect to the work environment. They pay, so they get to make the rules, but you can decide to stay or not.

@davisre

This comment has been minimized.

Show comment
Hide comment
@davisre

davisre May 30, 2012

I think it makes perfect sense to encapsulate this logic and to separate it from the controller. But I prefer to think of OOP as objects that do things. In my head, classes are nouns, and their methods are verbs. Maybe I'd have a CommentPoster but not a PostComment, a TwitterPoster but not a PostToTwitter. Your class names are predicates (in the grammatical sense, not in the Ruby boolean method sense), and talking about them out loud is confusing: I create a PostToFacebook for my Comment and tell it to post. I know it's the trend, but it sounds odd to my ear, the aural equivalent of a code smell. (We've all heard how sensitive programmers are. Ahem.)

To me, the operations you're performing belong in the model -- not necessarily in the ActiveRecord class, but in that body of code that represents the things in your world, the bits and pieces that can be told to do things, like update the database, check for spam, and ping external services. The current discussions about objects in Rails are fruitful because as our systems are becoming more complex and their surroundings are changing, we're all recognizing that modeling the world is more complex than simply making attributes persistent. It requires layers and strategies beyond (within? around?) ActiveRecord.

But I'd be sad if we came out of these discussions thinking that a shoebox of amorphous classes that wrap abstract sequences was really the goal. Naming is important because it reflects the team's mental model and shapes the discussions that people have about the project. I'd also be sad if we vilified whole strategies in favor of new flawed ones -- controllers are bad, callbacks are bad, fat is bad, carbohydrates, too. Steer clear of the evil ingredient and magical designs will fall out of your fingers, or maybe materialize while you sleep.

For what it's worth, I think, you're closer to the solution than your manager was, but I imagine we're missing a bunch of interpersonal context, too.

davisre commented May 30, 2012

I think it makes perfect sense to encapsulate this logic and to separate it from the controller. But I prefer to think of OOP as objects that do things. In my head, classes are nouns, and their methods are verbs. Maybe I'd have a CommentPoster but not a PostComment, a TwitterPoster but not a PostToTwitter. Your class names are predicates (in the grammatical sense, not in the Ruby boolean method sense), and talking about them out loud is confusing: I create a PostToFacebook for my Comment and tell it to post. I know it's the trend, but it sounds odd to my ear, the aural equivalent of a code smell. (We've all heard how sensitive programmers are. Ahem.)

To me, the operations you're performing belong in the model -- not necessarily in the ActiveRecord class, but in that body of code that represents the things in your world, the bits and pieces that can be told to do things, like update the database, check for spam, and ping external services. The current discussions about objects in Rails are fruitful because as our systems are becoming more complex and their surroundings are changing, we're all recognizing that modeling the world is more complex than simply making attributes persistent. It requires layers and strategies beyond (within? around?) ActiveRecord.

But I'd be sad if we came out of these discussions thinking that a shoebox of amorphous classes that wrap abstract sequences was really the goal. Naming is important because it reflects the team's mental model and shapes the discussions that people have about the project. I'd also be sad if we vilified whole strategies in favor of new flawed ones -- controllers are bad, callbacks are bad, fat is bad, carbohydrates, too. Steer clear of the evil ingredient and magical designs will fall out of your fingers, or maybe materialize while you sleep.

For what it's worth, I think, you're closer to the solution than your manager was, but I imagine we're missing a bunch of interpersonal context, too.

@dustintitus

This comment has been minimized.

Show comment
Hide comment
@dustintitus

dustintitus May 30, 2012

Here's the real issue: you can't justify encapsulating this under SRP, because it's not SRP. On Twitter, @dhh was correct in pointing out that language detector and spam filter would have to be in their own models, and called from the controller. Otherwise, trying to call this SRP is nothing but a sad joke.

The concept is good, the implementation is poor. If you're going to code according to principles, then follow the damned principles!

dustintitus commented May 30, 2012

Here's the real issue: you can't justify encapsulating this under SRP, because it's not SRP. On Twitter, @dhh was correct in pointing out that language detector and spam filter would have to be in their own models, and called from the controller. Otherwise, trying to call this SRP is nothing but a sad joke.

The concept is good, the implementation is poor. If you're going to code according to principles, then follow the damned principles!

@joakimk

This comment has been minimized.

Show comment
Hide comment
@joakimk

joakimk May 30, 2012

This is sure a lot of positive feedback in 2 hours. The ruby / rails community is truly a great thing. Can't do anything but agree here, this is good code. One of the hard parts of being a developer is communication. You can't always win, even if you are "right". People have to accept to ideas at their own pace. If you can't convince them, let them have the code as they like.

You can't always be right though, so always consider feedback carefully, you don't want to be that impossible developer that always has to have things their way =)

joakimk commented May 30, 2012

This is sure a lot of positive feedback in 2 hours. The ruby / rails community is truly a great thing. Can't do anything but agree here, this is good code. One of the hard parts of being a developer is communication. You can't always win, even if you are "right". People have to accept to ideas at their own pace. If you can't convince them, let them have the code as they like.

You can't always be right though, so always consider feedback carefully, you don't want to be that impossible developer that always has to have things their way =)

@mguterl

This comment has been minimized.

Show comment
Hide comment
@mguterl

mguterl May 30, 2012

@rGenta since you seem to have such a strong opinion on the issue I would really like to see how you would implement this code according to the damned principles.

mguterl commented May 30, 2012

@rGenta since you seem to have such a strong opinion on the issue I would really like to see how you would implement this code according to the damned principles.

@dustintitus

This comment has been minimized.

Show comment
Hide comment
@dustintitus

dustintitus May 30, 2012

To back up "davisre":

One of the most important concepts behind OO is modularity: the ability to use an object in other code. Maybe other places in this application, maybe other applications. This is an important (but often ignored) aspect of DRY.

Therefore davisre's concept of object as noun has a lot of validity. If you are just going to put a random collection of calls or methods in some code, and you feel it needs to be encapsulated, that's what modules are for, not grouped together in some nonsense class. That's rather a perversion of the whole concept of Class.

dustintitus commented May 30, 2012

To back up "davisre":

One of the most important concepts behind OO is modularity: the ability to use an object in other code. Maybe other places in this application, maybe other applications. This is an important (but often ignored) aspect of DRY.

Therefore davisre's concept of object as noun has a lot of validity. If you are just going to put a random collection of calls or methods in some code, and you feel it needs to be encapsulated, that's what modules are for, not grouped together in some nonsense class. That's rather a perversion of the whole concept of Class.

@dustintitus

This comment has been minimized.

Show comment
Hide comment
@dustintitus

dustintitus May 30, 2012

@ mguteri:

I already told you.

dustintitus commented May 30, 2012

@ mguteri:

I already told you.

@mboeh

This comment has been minimized.

Show comment
Hide comment
@mboeh

mboeh May 30, 2012

@rGenta That doesn't seem to be what DHH said -- he said he'd put the language detector/spam filter logic in the Entry class, and the Twitter logic would be in the controller. I'm not clear on how that's a better example of SRP than this.

mboeh commented May 30, 2012

@rGenta That doesn't seem to be what DHH said -- he said he'd put the language detector/spam filter logic in the Entry class, and the Twitter logic would be in the controller. I'm not clear on how that's a better example of SRP than this.

@dustintitus

This comment has been minimized.

Show comment
Hide comment
@dustintitus

dustintitus May 30, 2012

@mboeh:

Direct quote of @dhh from Twitter: "Would have put language detector, spam filter in model. Turn twitter poster into a AM-type class & call both from controller."

dustintitus commented May 30, 2012

@mboeh:

Direct quote of @dhh from Twitter: "Would have put language detector, spam filter in model. Turn twitter poster into a AM-type class & call both from controller."

@dustintitus

This comment has been minimized.

Show comment
Hide comment
@dustintitus

dustintitus May 30, 2012

@mboeh: I think we may have a misunderstanding here. The point is that the language detector and spam filter either belong IN that model, or they should be in separate models and called from the controller.

My main point was not with his code, but with the commenters saying that it is a good example of SRP. In fact, it is anything but.

dustintitus commented May 30, 2012

@mboeh: I think we may have a misunderstanding here. The point is that the language detector and spam filter either belong IN that model, or they should be in separate models and called from the controller.

My main point was not with his code, but with the commenters saying that it is a good example of SRP. In fact, it is anything but.

@xoebus

This comment has been minimized.

Show comment
Hide comment
@xoebus

xoebus May 30, 2012

Could you take this one step further and just let the PostComment class be a PORO and have another class called PostCommentPoster (for a severe lack of a better name) which could have the dependencies of the LanguageDetector, SpamChecker, CommentMailer and the social sharing service handlers injected into it?

Edit: Just realised that I forgot throw my support in for @justinko above. I think your solution is much better than the other one suggested.

xoebus commented May 30, 2012

Could you take this one step further and just let the PostComment class be a PORO and have another class called PostCommentPoster (for a severe lack of a better name) which could have the dependencies of the LanguageDetector, SpamChecker, CommentMailer and the social sharing service handlers injected into it?

Edit: Just realised that I forgot throw my support in for @justinko above. I think your solution is much better than the other one suggested.

@mboeh

This comment has been minimized.

Show comment
Hide comment
@mboeh

mboeh May 30, 2012

@rGenta I read "put ... in model" as "I would have put those things in the model at question". I erred in saying Entry, though -- I meant Comment. In this case, the language detection and spam filter are already in their own model classes, so I'm unclear on what your reading of DHH's approach would change here. I think this is a case of Twitter's character limit resulting in ambiguous phrasing. I'm not sure DHH's opinions on object oriented design merit a lot of effort in reading, though.

I agree that class names should generally be nouns, but I don't understand your remark about a "random collection of calls". It's not a random collection -- it's the set of operations necessary to fulfill a user's request to add a comment to an entry. I don't see how moving that logic to the controller (and thus making it impossible to use outside a request) is an improvement. The operation of a user posting a comment on an entry is part of this application's model, so the specifics of that operation shouldn't appear in the controller.

mboeh commented May 30, 2012

@rGenta I read "put ... in model" as "I would have put those things in the model at question". I erred in saying Entry, though -- I meant Comment. In this case, the language detection and spam filter are already in their own model classes, so I'm unclear on what your reading of DHH's approach would change here. I think this is a case of Twitter's character limit resulting in ambiguous phrasing. I'm not sure DHH's opinions on object oriented design merit a lot of effort in reading, though.

I agree that class names should generally be nouns, but I don't understand your remark about a "random collection of calls". It's not a random collection -- it's the set of operations necessary to fulfill a user's request to add a comment to an entry. I don't see how moving that logic to the controller (and thus making it impossible to use outside a request) is an improvement. The operation of a user posting a comment on an entry is part of this application's model, so the specifics of that operation shouldn't appear in the controller.

@mboeh

This comment has been minimized.

Show comment
Hide comment
@mboeh

mboeh May 30, 2012

@xoebus PostComment already is a PORO, unless there's a nuance to the term I'm missing. It's not "post comment" as in "comment associated to a post" but "post comment" as in "post a comment to the site." It appears that Entry is the name of the class being commented on.

And that right there is part of the problem with not making your class names nouns.

mboeh commented May 30, 2012

@xoebus PostComment already is a PORO, unless there's a nuance to the term I'm missing. It's not "post comment" as in "comment associated to a post" but "post comment" as in "post a comment to the site." It appears that Entry is the name of the class being commented on.

And that right there is part of the problem with not making your class names nouns.

@dustintitus

This comment has been minimized.

Show comment
Hide comment
@dustintitus

dustintitus May 30, 2012

@mboeh:

My entire point is that they ARE in separate models, so the SRP principle would preclude them from being called from THIS model. If you really want to do SRP, then they must be called separately from the controller.

If you consider that what those models do are actually part of a "single responsibility", then they belong in this model. But again: models calling methods on other models is kind of anti- SRP.

dustintitus commented May 30, 2012

@mboeh:

My entire point is that they ARE in separate models, so the SRP principle would preclude them from being called from THIS model. If you really want to do SRP, then they must be called separately from the controller.

If you consider that what those models do are actually part of a "single responsibility", then they belong in this model. But again: models calling methods on other models is kind of anti- SRP.

@xoebus

This comment has been minimized.

Show comment
Hide comment
@xoebus

xoebus May 30, 2012

@mboeh

Ah, my mistake - you're totally correct.

What I was trying to say is that instead of passing the comment into the constructor of this class we pass the services that we are going to be using to process the comment instead. We then pass the comment model into the post method either as individual @entry, @user, and @attributes variables or combining these three variables into a new simple class.

This would allow us to test the comment posting classes without sending of all sorts of emails and sharing them on networks by stubbing the services during testing. I'm imagining something like this:

# In an initialiser or something - not sure.

language_detector = LanguageDetector.new
spam_checker = SpamChecker.new
mailer = CommentMailer.new
twitter_poster = PostToTwitter.new
facebook_poster = PostToFacebook.new

comment_poster = PostComment.new(language_detector,
  spam_checker,
  mailer,
  twitter_poster,
  facebook_poster)

# In the Controller

@comment = params[:comment] # or however you're getting the data
comment_poster.post(@comment)

This does require that the services used have their API changed by not having the comment they are working on be passed into the constructor but instead it would change from SpamChecker.new(@comment).check_spam to spam_checker.check(comment).

xoebus commented May 30, 2012

@mboeh

Ah, my mistake - you're totally correct.

What I was trying to say is that instead of passing the comment into the constructor of this class we pass the services that we are going to be using to process the comment instead. We then pass the comment model into the post method either as individual @entry, @user, and @attributes variables or combining these three variables into a new simple class.

This would allow us to test the comment posting classes without sending of all sorts of emails and sharing them on networks by stubbing the services during testing. I'm imagining something like this:

# In an initialiser or something - not sure.

language_detector = LanguageDetector.new
spam_checker = SpamChecker.new
mailer = CommentMailer.new
twitter_poster = PostToTwitter.new
facebook_poster = PostToFacebook.new

comment_poster = PostComment.new(language_detector,
  spam_checker,
  mailer,
  twitter_poster,
  facebook_poster)

# In the Controller

@comment = params[:comment] # or however you're getting the data
comment_poster.post(@comment)

This does require that the services used have their API changed by not having the comment they are working on be passed into the constructor but instead it would change from SpamChecker.new(@comment).check_spam to spam_checker.check(comment).

@mguterl

This comment has been minimized.

Show comment
Hide comment
@mguterl

mguterl May 30, 2012

@xoebus - I really like this approach over initializing the services inside of the class. This allows for much easier stubbing and if FaceboookPoster requires some type of credentials, PostComment doesn't have to know about them.

mguterl commented May 30, 2012

@xoebus - I really like this approach over initializing the services inside of the class. This allows for much easier stubbing and if FaceboookPoster requires some type of credentials, PostComment doesn't have to know about them.

@ahoward

This comment has been minimized.

Show comment
Hide comment
@ahoward

ahoward May 31, 2012

@saturnflyer fair enough - i've made the same argument in the past and it's true.

on another note - my stomach for bad raills' code is at an all time low...

ahoward commented May 31, 2012

@saturnflyer fair enough - i've made the same argument in the past and it's true.

on another note - my stomach for bad raills' code is at an all time low...

@mboeh

This comment has been minimized.

Show comment
Hide comment
@mboeh

mboeh May 31, 2012

A relevant article at the Wiki:

http://c2.com/cgi/wiki?ResponsibilityDrivenDesignConflictsWithYagni

@rGenta I have never heard the Single Responsibility Principle stated that way before. It exists completely independent of any form of MVC, so I don't see how SRP says anything about whether code goes into the Model or into the Controller. Just because one object asks another one to perform a task (i.e. "post this comment to Twitter") doesn't mean the first object is adopting the responsibility for that task. Otherwise, the SRP would be impossible and absurd on its face.

mboeh commented May 31, 2012

A relevant article at the Wiki:

http://c2.com/cgi/wiki?ResponsibilityDrivenDesignConflictsWithYagni

@rGenta I have never heard the Single Responsibility Principle stated that way before. It exists completely independent of any form of MVC, so I don't see how SRP says anything about whether code goes into the Model or into the Controller. Just because one object asks another one to perform a task (i.e. "post this comment to Twitter") doesn't mean the first object is adopting the responsibility for that task. Otherwise, the SRP would be impossible and absurd on its face.

@nateklaiber

This comment has been minimized.

Show comment
Hide comment
@nateklaiber

nateklaiber May 31, 2012

@rGenta - The language detector and spam filter are in their own objects. Their responsibility lives in their own object and can be accompanied by their own unit tests. The single responsibility of this object is to "post a comment". It's atomic in nature, in that it has to do everything in that list (language, spam, twitter, facebook). That is it's responsibility.

One of the most important concepts behind OO is modularity: the ability to use an object in other code. Maybe other places in this application, maybe other applications.

On one hand you say it's not single responsibility because he's delegating to other objects, and now you say the point of OO is that you can use an object in other code.

According your your guidelines, ActiveRecord violates SRP, too, so how would using that be any better? What if he had called class methods on the other objects instead, would you see it differently?

nateklaiber commented May 31, 2012

@rGenta - The language detector and spam filter are in their own objects. Their responsibility lives in their own object and can be accompanied by their own unit tests. The single responsibility of this object is to "post a comment". It's atomic in nature, in that it has to do everything in that list (language, spam, twitter, facebook). That is it's responsibility.

One of the most important concepts behind OO is modularity: the ability to use an object in other code. Maybe other places in this application, maybe other applications.

On one hand you say it's not single responsibility because he's delegating to other objects, and now you say the point of OO is that you can use an object in other code.

According your your guidelines, ActiveRecord violates SRP, too, so how would using that be any better? What if he had called class methods on the other objects instead, would you see it differently?

@unclebob

This comment has been minimized.

Show comment
Hide comment
@unclebob

unclebob May 31, 2012

Wait. You lost a contract over this? Huh? There's got to be more to the story.

unclebob commented May 31, 2012

Wait. You lost a contract over this? Huh? There's got to be more to the story.

@solnic

This comment has been minimized.

Show comment
Hide comment
@solnic

solnic May 31, 2012

@mboeh callbacks for mutating attribute values are OK only in simple scenarios but you have to be careful with them. When things grow bigger I switch to mapper objects that are more explicit, so rather than having one AR object that has to deal with multiple scenarios I have a bunch of mapper objects for each specific scenario where attributes are being calculated/converted for the underlaying AR object. And again, testing these objects is much better because I'm dealing with PORO objects.

I think it would make sense to say that in general people should be striving for NOT using callbacks. I even think that ORM authors should discourage people from using callbacks even if their ORM supports them.

solnic commented May 31, 2012

@mboeh callbacks for mutating attribute values are OK only in simple scenarios but you have to be careful with them. When things grow bigger I switch to mapper objects that are more explicit, so rather than having one AR object that has to deal with multiple scenarios I have a bunch of mapper objects for each specific scenario where attributes are being calculated/converted for the underlaying AR object. And again, testing these objects is much better because I'm dealing with PORO objects.

I think it would make sense to say that in general people should be striving for NOT using callbacks. I even think that ORM authors should discourage people from using callbacks even if their ORM supports them.

@JonRowe

This comment has been minimized.

Show comment
Hide comment
@JonRowe

JonRowe May 31, 2012

I think you're better off without this contract if that code got you fired, it's absolutely along the right lines of building a nice clean testable application. Did you explain your motivation and try to reason with them? Otherwise it sounds to me like the Senior Developer has been practising a bit of Mortgage Driven Development!

JonRowe commented May 31, 2012

I think you're better off without this contract if that code got you fired, it's absolutely along the right lines of building a nice clean testable application. Did you explain your motivation and try to reason with them? Otherwise it sounds to me like the Senior Developer has been practising a bit of Mortgage Driven Development!

@txus

This comment has been minimized.

Show comment
Hide comment
@txus

txus May 31, 2012

I totally agree with @jc00ke on this; you've found a sucky client. Unfortunately there are plenty of them, and you're definitely better off away from him. Especially if he won't listen to your motivation/technical reasons for this. Sucky clients won't trust you or your skills, so why do they hire you in the first place? It's always been a mystery to me.

Don't let this bother you Justin, and keep writing great code! :)

txus commented May 31, 2012

I totally agree with @jc00ke on this; you've found a sucky client. Unfortunately there are plenty of them, and you're definitely better off away from him. Especially if he won't listen to your motivation/technical reasons for this. Sucky clients won't trust you or your skills, so why do they hire you in the first place? It's always been a mystery to me.

Don't let this bother you Justin, and keep writing great code! :)

@justinko

This comment has been minimized.

Show comment
Hide comment
@justinko

justinko Jun 1, 2012

Thank you for the comments everyone! Feels great to have so many people back me up on this. I love this community.

To address some of the points discussed, here are some concepts that should cover all of them:

Defensive programming is rubbish:
http://danielroop.com/blog/2009/10/15/why-defensive-programming-is-rubbish/

On "modeling the real world":
http://programmers.stackexchange.com/questions/137994/does-object-oriented-programming-really-model-the-real-world

On the abuse of Dependency Injection:
http://weblog.jamisbuck.org/2008/11/9/legos-play-doh-and-programming

On nouns vs. verbs:

This code was originally implemented with the DCI paradigm. DCI started to break down for me, so I switched to a more "generic" OOP approach. What I did take away from DCI is the concept of modeling your code around use cases. This is why the classes are verbs. To avoid confusion, I probably should have named the class PostAComment.

Owner

justinko commented Jun 1, 2012

Thank you for the comments everyone! Feels great to have so many people back me up on this. I love this community.

To address some of the points discussed, here are some concepts that should cover all of them:

Defensive programming is rubbish:
http://danielroop.com/blog/2009/10/15/why-defensive-programming-is-rubbish/

On "modeling the real world":
http://programmers.stackexchange.com/questions/137994/does-object-oriented-programming-really-model-the-real-world

On the abuse of Dependency Injection:
http://weblog.jamisbuck.org/2008/11/9/legos-play-doh-and-programming

On nouns vs. verbs:

This code was originally implemented with the DCI paradigm. DCI started to break down for me, so I switched to a more "generic" OOP approach. What I did take away from DCI is the concept of modeling your code around use cases. This is why the classes are verbs. To avoid confusion, I probably should have named the class PostAComment.

@justinko

This comment has been minimized.

Show comment
Hide comment
@justinko

justinko Jun 1, 2012

@j3

I think it's good, but am not surprised that someone who doesn't understand how it fits into the paradigm ("OMG what is a use_cases folder?!?!") would freak out a bit. "Architects", especially, are often scared of new ideas.

Yes. Realizing that Rails offers nothing in terms of building an app with good OOP design principles is the best step you can take to improving your Rails code.

@trptcolin

Small nitpick: the controller knows about success/fail based on the #save! call raising or not, cool - the only possible wrinkle I'd see would be that if anything fails in line 17-22, the controller may want to know how handle that.

I don't build for failure unless it happens or is a business requirement. Obviously, the raminfications of a potential failure should always be taken into consideration.

@mboeh

Well, everyone knows that there's a finite number of classes available in the world. You don't want to waste those classes!

LOL

@raggi

I guess the shortest form of the above is: I'd probably start with a PostComment ""model""...

"Post" in PostComment is a verb. I should have named it PostAComment.

@unclebob

Wait. You lost a contract over this? Huh? There's got to be more to the story.

This code was initially implemented with DCI (I did get the "go ahead" for this). The stakeholders had issues with DCI, as did I over time. I updated this code to use a more generic OOP approach, but kept the modeling-around-use-cases aspect. Still, they had issues with it. I got a call from the stakeholder saying "This is not working out. The philosophical differences between you and the new [senior dev] are just too great." This all happened with absolutely zero code reviews or "warnings".

@dhh

Avoiding AR callbacks is retarded. https://twitter.com/dhh/status/207963090766139392

No, coupling posting-to-twitter with the context-less creation of a Comment is "retarded". Stubbing callbacks in a test so you may isolate some logic is "retarded".

Owner

justinko commented Jun 1, 2012

@j3

I think it's good, but am not surprised that someone who doesn't understand how it fits into the paradigm ("OMG what is a use_cases folder?!?!") would freak out a bit. "Architects", especially, are often scared of new ideas.

Yes. Realizing that Rails offers nothing in terms of building an app with good OOP design principles is the best step you can take to improving your Rails code.

@trptcolin

Small nitpick: the controller knows about success/fail based on the #save! call raising or not, cool - the only possible wrinkle I'd see would be that if anything fails in line 17-22, the controller may want to know how handle that.

I don't build for failure unless it happens or is a business requirement. Obviously, the raminfications of a potential failure should always be taken into consideration.

@mboeh

Well, everyone knows that there's a finite number of classes available in the world. You don't want to waste those classes!

LOL

@raggi

I guess the shortest form of the above is: I'd probably start with a PostComment ""model""...

"Post" in PostComment is a verb. I should have named it PostAComment.

@unclebob

Wait. You lost a contract over this? Huh? There's got to be more to the story.

This code was initially implemented with DCI (I did get the "go ahead" for this). The stakeholders had issues with DCI, as did I over time. I updated this code to use a more generic OOP approach, but kept the modeling-around-use-cases aspect. Still, they had issues with it. I got a call from the stakeholder saying "This is not working out. The philosophical differences between you and the new [senior dev] are just too great." This all happened with absolutely zero code reviews or "warnings".

@dhh

Avoiding AR callbacks is retarded. https://twitter.com/dhh/status/207963090766139392

No, coupling posting-to-twitter with the context-less creation of a Comment is "retarded". Stubbing callbacks in a test so you may isolate some logic is "retarded".

@mboeh

This comment has been minimized.

Show comment
Hide comment
@mboeh

mboeh Jun 1, 2012

@justinko Well, all I'll say is that if I was given the chance to hire someone who writes code like this gist or someone who writes code like DHH, the choice would be obvious. DHH's commercial success and fancy car doesn't make him a good programmer.

Also, in my experience, people who refer to people who disagree with them as 'retarded' are not fun to work with.

mboeh commented Jun 1, 2012

@justinko Well, all I'll say is that if I was given the chance to hire someone who writes code like this gist or someone who writes code like DHH, the choice would be obvious. DHH's commercial success and fancy car doesn't make him a good programmer.

Also, in my experience, people who refer to people who disagree with them as 'retarded' are not fun to work with.

@nateklaiber

This comment has been minimized.

Show comment
Hide comment
@nateklaiber

nateklaiber Jun 1, 2012

@justinko

You posted a link to the Defensive programming is rubbish article. Did you read it in it's entirety? It's actually saying the same things several others in this thread have said. None of us were advocating nil checking (which is what that article is against)

I agree with @garybernhardt to check at the boundaries - I just believe the boundary could be the constructor of an object. Your class is a contract. In your object, there are several points for failure that would cause nil errors - errors not as descriptive as if you would have checked those at the constructor and raised meaningful exceptions.

In the article you mentioned, the author advocates the same practice: don't ignore it, and give a meaningful message:

If you hadn’t been so “defensive” you would probably have an exception written to the log because when User A request their friend list they get a null instead, thus causing a null pointer in the findFavoriteFriends method. This would not only give you a starting point for the bug, it would also trigger a notification of the problem before the User A complaint even reached the developers because your log monitor picked up the exception and notified the developers.

He goes on to say:

When I began writing the article I agreed with the title, but after reading up on what Defensive Programming really is, I realized. I am a defensive programmer. Just not in the buzz word cop out sense. If you don’t believe me maybe you will believe Ward Cunningham who has a wiki article on Offensive Programming, and points out Defensive Programming is the same as a FailFast approach which is closer to how I would write my code.

Fail Fast is what I (and others) were advocating. The code you provided doesn't fail fast.

Gary also had a short screencast about avoiding nil

nateklaiber commented Jun 1, 2012

@justinko

You posted a link to the Defensive programming is rubbish article. Did you read it in it's entirety? It's actually saying the same things several others in this thread have said. None of us were advocating nil checking (which is what that article is against)

I agree with @garybernhardt to check at the boundaries - I just believe the boundary could be the constructor of an object. Your class is a contract. In your object, there are several points for failure that would cause nil errors - errors not as descriptive as if you would have checked those at the constructor and raised meaningful exceptions.

In the article you mentioned, the author advocates the same practice: don't ignore it, and give a meaningful message:

If you hadn’t been so “defensive” you would probably have an exception written to the log because when User A request their friend list they get a null instead, thus causing a null pointer in the findFavoriteFriends method. This would not only give you a starting point for the bug, it would also trigger a notification of the problem before the User A complaint even reached the developers because your log monitor picked up the exception and notified the developers.

He goes on to say:

When I began writing the article I agreed with the title, but after reading up on what Defensive Programming really is, I realized. I am a defensive programmer. Just not in the buzz word cop out sense. If you don’t believe me maybe you will believe Ward Cunningham who has a wiki article on Offensive Programming, and points out Defensive Programming is the same as a FailFast approach which is closer to how I would write my code.

Fail Fast is what I (and others) were advocating. The code you provided doesn't fail fast.

Gary also had a short screencast about avoiding nil

@agraves

This comment has been minimized.

Show comment
Hide comment
@agraves

agraves Jun 22, 2012

I wouldn't approve this in a code review. Lots of complexity, overhead and glue for no measurable benefit.

The @comment stuff goes in the controller, spam and spell checks go in validations, email goes in an observer. Twitter and Facebook posts are fine in the controller.

Your fundamental mistake is fighting complexity with complexity. If you need that many public methods in the mix to post a comment you're exposing too much.

If I had someone who insisted on adding all this bloat I'd cut them loose, too.

agraves commented Jun 22, 2012

I wouldn't approve this in a code review. Lots of complexity, overhead and glue for no measurable benefit.

The @comment stuff goes in the controller, spam and spell checks go in validations, email goes in an observer. Twitter and Facebook posts are fine in the controller.

Your fundamental mistake is fighting complexity with complexity. If you need that many public methods in the mix to post a comment you're exposing too much.

If I had someone who insisted on adding all this bloat I'd cut them loose, too.

@mguterl

This comment has been minimized.

Show comment
Hide comment
@mguterl

mguterl Jun 22, 2012

@agraves I'm not sure that you know what meta-programming is.

mguterl commented Jun 22, 2012

@agraves I'm not sure that you know what meta-programming is.

@agraves

This comment has been minimized.

Show comment
Hide comment
@agraves

agraves Jun 22, 2012

@mguterl I sometimes forget that term isn't a slur and actually means something.

agraves commented Jun 22, 2012

@mguterl I sometimes forget that term isn't a slur and actually means something.

@tchype

This comment has been minimized.

Show comment
Hide comment
@tchype

tchype Jun 22, 2012

@justinko Three things and then a thought or two:

  1. Your code is very nice and readable; love it!
  2. While you CAN put this into a class, it really seems like the post method is your controller action, and post_to_twitter and post_to_facebook could simply be helper methods in your controller class code; at most, simple private helper methods could encapsulate some of the lines of code together, but is probably overkill; there's nothing special about your helpers since their implementations are one-liners, so you could probably just inline those as well.
  3. Factoring out the post to twitter and facebook into some other construct/class is awesome (although you might consider class methods -- Twitter.post(user, comment) and Facebook.comment(user, comment) since there's so little state being carried through.

Obviously, if this code needed to be shared or reused, that's what a Refactoring step is for -- AT THE TIME YOU NEED TO SHARE/REUSE THE CODE. Not before :-)

When working in someone else's project (private or Open Source), you definitely need to follow their style unless they are open to your preferences. If they are not, then yeah, may not be a fun place to work. But that can be a slippery slope and vastly narrow-down your options for potential employers too.

My guess is that this wasn't the first time they had talked to you about "class explosion" or something similar and that this was not the first time they didn't like this style...am I right? Just curious. If so, just make sure YOU are also open to feedback or changes your clients are looking for.

So sorry this happened to you. Good luck in the future.

tchype commented Jun 22, 2012

@justinko Three things and then a thought or two:

  1. Your code is very nice and readable; love it!
  2. While you CAN put this into a class, it really seems like the post method is your controller action, and post_to_twitter and post_to_facebook could simply be helper methods in your controller class code; at most, simple private helper methods could encapsulate some of the lines of code together, but is probably overkill; there's nothing special about your helpers since their implementations are one-liners, so you could probably just inline those as well.
  3. Factoring out the post to twitter and facebook into some other construct/class is awesome (although you might consider class methods -- Twitter.post(user, comment) and Facebook.comment(user, comment) since there's so little state being carried through.

Obviously, if this code needed to be shared or reused, that's what a Refactoring step is for -- AT THE TIME YOU NEED TO SHARE/REUSE THE CODE. Not before :-)

When working in someone else's project (private or Open Source), you definitely need to follow their style unless they are open to your preferences. If they are not, then yeah, may not be a fun place to work. But that can be a slippery slope and vastly narrow-down your options for potential employers too.

My guess is that this wasn't the first time they had talked to you about "class explosion" or something similar and that this was not the first time they didn't like this style...am I right? Just curious. If so, just make sure YOU are also open to feedback or changes your clients are looking for.

So sorry this happened to you. Good luck in the future.

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Jun 22, 2012

I'm a little late to the party, but unless you're actually reusing this PostComment logic in multiple places, I agree with your senior developer. Here's how I'd swing it in the regular controller:

class PostsController
  before_filter :set_entry
  before_filter :reject_spam

  def create
    @comment = @entry.comments.create!(params[:post].permit(:title, :body).merge(author: current_user))

    Notifications.new_comment(@comment).deliver

    TwitterPoster.new(current_user, @comment.body).post              if @comment.share_on_twitter?
    FacebookPoster.new(current_user, @comment.body).action(:comment) if @comment.share_on_facebook?    
  end

  private
    def set_entry
      @entry = current_account.entries.find(params[:id])
    end

    def reject_spam
      head :bad_request if SpamChecker.spammy?(params[:post][:body])
    end
end

I'm assuming that the language detection is something that needs to happen in the comment, so I would keep it in that model -- but not sure what it does.

dhh commented Jun 22, 2012

I'm a little late to the party, but unless you're actually reusing this PostComment logic in multiple places, I agree with your senior developer. Here's how I'd swing it in the regular controller:

class PostsController
  before_filter :set_entry
  before_filter :reject_spam

  def create
    @comment = @entry.comments.create!(params[:post].permit(:title, :body).merge(author: current_user))

    Notifications.new_comment(@comment).deliver

    TwitterPoster.new(current_user, @comment.body).post              if @comment.share_on_twitter?
    FacebookPoster.new(current_user, @comment.body).action(:comment) if @comment.share_on_facebook?    
  end

  private
    def set_entry
      @entry = current_account.entries.find(params[:id])
    end

    def reject_spam
      head :bad_request if SpamChecker.spammy?(params[:post][:body])
    end
end

I'm assuming that the language detection is something that needs to happen in the comment, so I would keep it in that model -- but not sure what it does.

@jszmajda

This comment has been minimized.

Show comment
Hide comment
@jszmajda

jszmajda Jun 22, 2012

This example specifically might be premature refactoring, but what about when you need to to 10 different optional things when creating a comment? What about when those things have internal dependencies? Complex systems require you to think about the code at a higher level, even when you're not sharing that code across multiple actions.

jszmajda commented Jun 22, 2012

This example specifically might be premature refactoring, but what about when you need to to 10 different optional things when creating a comment? What about when those things have internal dependencies? Complex systems require you to think about the code at a higher level, even when you're not sharing that code across multiple actions.

@arwagner

This comment has been minimized.

Show comment
Hide comment
@arwagner

arwagner Jun 22, 2012

@dhh this is interesting, and not at all what I thought you were suggesting. one difference is that the code the OP gave can be tested without loading all of rails.

arwagner commented Jun 22, 2012

@dhh this is interesting, and not at all what I thought you were suggesting. one difference is that the code the OP gave can be tested without loading all of rails.

@reinh

This comment has been minimized.

Show comment
Hide comment
@reinh

reinh Jun 22, 2012

The quality of code aside, I'm amazing at the short-sightedness that would get you fired over this. A single instance of bad code is not a fireable offense; it's a learning opportunity. The cost of firing and hiring is far, far greater than the cost of retraining, especially for someone like you that actually cares about code quality. I'm disappointed in your boss for making a terrible decision -- both for you and his company -- and frankly you're probably better off somewhere else.

reinh commented Jun 22, 2012

The quality of code aside, I'm amazing at the short-sightedness that would get you fired over this. A single instance of bad code is not a fireable offense; it's a learning opportunity. The cost of firing and hiring is far, far greater than the cost of retraining, especially for someone like you that actually cares about code quality. I'm disappointed in your boss for making a terrible decision -- both for you and his company -- and frankly you're probably better off somewhere else.

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Jun 22, 2012

joshsz, playing "what if" with code is exactly what YAGNI was coined to stop. The point is that the original poster prematurely abstracted logic.

Yes, there might well exist a future where you need more abstractions, but you wait for that day to arrive. You don't try to polish your crystal ball and imagine what that day will look like and then line up your abstractions in anticipation.

dhh commented Jun 22, 2012

joshsz, playing "what if" with code is exactly what YAGNI was coined to stop. The point is that the original poster prematurely abstracted logic.

Yes, there might well exist a future where you need more abstractions, but you wait for that day to arrive. You don't try to polish your crystal ball and imagine what that day will look like and then line up your abstractions in anticipation.

@jszmajda

This comment has been minimized.

Show comment
Hide comment
@jszmajda

jszmajda Jun 22, 2012

@dhh fair enough. It's hard to discuss concepts like these without specific cases. This case was probably overuse, others are not.

jszmajda commented Jun 22, 2012

@dhh fair enough. It's hard to discuss concepts like these without specific cases. This case was probably overuse, others are not.

@reinh

This comment has been minimized.

Show comment
Hide comment
@reinh

reinh Jun 22, 2012

@joshsz "Just in time, not just in case."

reinh commented Jun 22, 2012

@joshsz "Just in time, not just in case."

@scottburton11

This comment has been minimized.

Show comment
Hide comment
@scottburton11

scottburton11 Jun 22, 2012

I think the example @dhh provides would be harder to test than @justinko's. I think the natural response would be "well, don't test the corner cases". I think the natural result will be "Facebook posting is broken and nobody knows about it."

But what do I know.

scottburton11 commented Jun 22, 2012

I think the example @dhh provides would be harder to test than @justinko's. I think the natural response would be "well, don't test the corner cases". I think the natural result will be "Facebook posting is broken and nobody knows about it."

But what do I know.

@agraves

This comment has been minimized.

Show comment
Hide comment
@agraves

agraves Jun 22, 2012

I'd do something like this.

class CommentsController < ApplicationController
  def create
    @comment = current_user.comments.new(params[:comments])

    if @comment.save
      @comment.post_tweet!
      @comment.post_facebook!
      redirect_to '/something'
    else
      render :edit
    end
  end
end

#-----------------

class Comment < ActiveRecord::Base
  validate :not_spam

  private

  # implementation
end

#---------------

class CommentObserver < ActiveRecord::Observer
  before_save :detect_language
  after_save :send_email
end

agraves commented Jun 22, 2012

I'd do something like this.

class CommentsController < ApplicationController
  def create
    @comment = current_user.comments.new(params[:comments])

    if @comment.save
      @comment.post_tweet!
      @comment.post_facebook!
      redirect_to '/something'
    else
      render :edit
    end
  end
end

#-----------------

class Comment < ActiveRecord::Base
  validate :not_spam

  private

  # implementation
end

#---------------

class CommentObserver < ActiveRecord::Observer
  before_save :detect_language
  after_save :send_email
end
@jeremy6d

This comment has been minimized.

Show comment
Hide comment
@jeremy6d

jeremy6d Jun 22, 2012

I object to the idea that I can judge this code on anything but its readability. Other than that, I need more context. I think there's merit to design patterns and things like SRP but to elevate them into goods in and of themselves -- as being invited to judge this code encourages people to do -- is distracting, irrelevant, and counterproductive.

For similar reasons, I can't judge whether or not you should have been fired. It goes the other way, too: if that were a code sample and I was trying to figure out whether to hire you, I'd be making an arbitrary judgment call / slightly educated guess about your competency based on code representing a part of a solution to a problem I don't understand.

For what it's worth, I can't see myself writing what you wrote.

jeremy6d commented Jun 22, 2012

I object to the idea that I can judge this code on anything but its readability. Other than that, I need more context. I think there's merit to design patterns and things like SRP but to elevate them into goods in and of themselves -- as being invited to judge this code encourages people to do -- is distracting, irrelevant, and counterproductive.

For similar reasons, I can't judge whether or not you should have been fired. It goes the other way, too: if that were a code sample and I was trying to figure out whether to hire you, I'd be making an arbitrary judgment call / slightly educated guess about your competency based on code representing a part of a solution to a problem I don't understand.

For what it's worth, I can't see myself writing what you wrote.

@EricR

This comment has been minimized.

Show comment
Hide comment
@EricR

EricR Jun 22, 2012

I currently do something very similar to what agraves posted. What do you guys think about simply using observers to do this?

EricR commented Jun 22, 2012

I currently do something very similar to what agraves posted. What do you guys think about simply using observers to do this?

@agraves

This comment has been minimized.

Show comment
Hide comment
@agraves

agraves Jun 22, 2012

@jeremy6d Maintainability and ease of testing are just as important as readability.

agraves commented Jun 22, 2012

@jeremy6d Maintainability and ease of testing are just as important as readability.

@agraves

This comment has been minimized.

Show comment
Hide comment
@agraves

agraves Jun 22, 2012

@reinh I'm sure there's more to this story. If I had to guess, I'd suspect this was not the first time the poster was confronted over this "design pattern."

agraves commented Jun 22, 2012

@reinh I'm sure there's more to this story. If I had to guess, I'd suspect this was not the first time the poster was confronted over this "design pattern."

@joevandyk

This comment has been minimized.

Show comment
Hide comment
@joevandyk

joevandyk Jun 22, 2012

@dhh he didn't prematurely abstract logic. He put the responsibility for what happens when you post a comment into a single class.

Your example has the controller assuming complete knowledge of how to handle a comment post. Along with the knowledge of what's being posted by the forms. Along with the knowledge of what to return to the user on spams. Along with the knowledge of finding the current account's entries. Along with the knowledge of what sort of response to return to users on successful comment posts.

In addition, the controller communicates with the APIs of a bunch of different classes, not loosely coupled at all.

Also, wouldn't you consider the before_filter for getting the @Entry object to be premature optimization? Why isn't the @Entry object being instantiated in the create method? Are you, heaven forbid, thinking about the future where you might have multiple actions in the controller?

joevandyk commented Jun 22, 2012

@dhh he didn't prematurely abstract logic. He put the responsibility for what happens when you post a comment into a single class.

Your example has the controller assuming complete knowledge of how to handle a comment post. Along with the knowledge of what's being posted by the forms. Along with the knowledge of what to return to the user on spams. Along with the knowledge of finding the current account's entries. Along with the knowledge of what sort of response to return to users on successful comment posts.

In addition, the controller communicates with the APIs of a bunch of different classes, not loosely coupled at all.

Also, wouldn't you consider the before_filter for getting the @Entry object to be premature optimization? Why isn't the @Entry object being instantiated in the create method? Are you, heaven forbid, thinking about the future where you might have multiple actions in the controller?

@jeremy6d

This comment has been minimized.

Show comment
Hide comment
@jeremy6d

jeremy6d Jun 22, 2012

@agraves yeah but without more context or making lots of assumptions (which is what I think is going on here) how would I judge it?

jeremy6d commented Jun 22, 2012

@agraves yeah but without more context or making lots of assumptions (which is what I think is going on here) how would I judge it?

@holamendi

This comment has been minimized.

Show comment
Hide comment
@holamendi

holamendi Jun 22, 2012

I totally like the way you're doing it, I don't know if you are going to use that code somewhere else, but if not, I wouldn't write that business login in PostsController, I'm a "skinny controller, fat model" type of person, so I'd use an ActiveRecord callback as a second option, maybe.
I'm sorry about the contract.

holamendi commented Jun 22, 2012

I totally like the way you're doing it, I don't know if you are going to use that code somewhere else, but if not, I wouldn't write that business login in PostsController, I'm a "skinny controller, fat model" type of person, so I'd use an ActiveRecord callback as a second option, maybe.
I'm sorry about the contract.

@agraves

This comment has been minimized.

Show comment
Hide comment
@agraves

agraves Jun 22, 2012

@joevandyk "He put the responsibility for what happens when you post a comment into a single class."

Which is the dictionary definition of abstraction. Can you guess what my next question is?

agraves commented Jun 22, 2012

@joevandyk "He put the responsibility for what happens when you post a comment into a single class."

Which is the dictionary definition of abstraction. Can you guess what my next question is?

@agraves

This comment has been minimized.

Show comment
Hide comment
@agraves

agraves Jun 22, 2012

@jeremy6d I prefer to consider the ratio of: "lines of code solving the problem" to "lines of code massaging my program."

The whole purpose of this class is to abstract and organize the comment sending process. The senior dev made it clear that this didn't need to be abstracted yet, and you shouldn't write code that needs other code to organize it to begin with.

This smacks of the "fight complexity with complexity" mantra that plagues a certain other language.

agraves commented Jun 22, 2012

@jeremy6d I prefer to consider the ratio of: "lines of code solving the problem" to "lines of code massaging my program."

The whole purpose of this class is to abstract and organize the comment sending process. The senior dev made it clear that this didn't need to be abstracted yet, and you shouldn't write code that needs other code to organize it to begin with.

This smacks of the "fight complexity with complexity" mantra that plagues a certain other language.

@pedrosnk

This comment has been minimized.

Show comment
Hide comment
@pedrosnk

pedrosnk Jun 22, 2012

I think I would do something like @agraves not that what you did was wrong, But I'm intended to use the skinny controllers aproach.

pedrosnk commented Jun 22, 2012

I think I would do something like @agraves not that what you did was wrong, But I'm intended to use the skinny controllers aproach.

@RevoHoffman

This comment has been minimized.

Show comment
Hide comment
@RevoHoffman

RevoHoffman Jun 22, 2012

I think your code is fine, but like Uncle Bob, I wonder if there is more to it than this. I can't imagine a person being fired for such an innocuous piece of code. On the other hand, I can completely imagine someone being fired for lacking the necessary skills to convey their side without coming off like a complete douchebag.

My suggestion would be to look a bit deeper and consider that it wasn't a technical issue, but perhaps a soft-skill issue that got you canned.

RevoHoffman commented Jun 22, 2012

I think your code is fine, but like Uncle Bob, I wonder if there is more to it than this. I can't imagine a person being fired for such an innocuous piece of code. On the other hand, I can completely imagine someone being fired for lacking the necessary skills to convey their side without coming off like a complete douchebag.

My suggestion would be to look a bit deeper and consider that it wasn't a technical issue, but perhaps a soft-skill issue that got you canned.

@mguterl

This comment has been minimized.

Show comment
Hide comment
@mguterl

mguterl Jun 22, 2012

@agraves - I'm not sure how this is complexity. In fact I think the lack of indirection in the code makes it very clear to read and understand what is happening. No digging around in callbacks or before filters...

mguterl commented Jun 22, 2012

@agraves - I'm not sure how this is complexity. In fact I think the lack of indirection in the code makes it very clear to read and understand what is happening. No digging around in callbacks or before filters...

@BRMatt

This comment has been minimized.

Show comment
Hide comment
@BRMatt

BRMatt Jun 22, 2012

@agraves In my opinion AR callbacks are not a solution. Using them forces you to run an actual database transaction within your tests. With the code OP showed you can test that all the interactions (e.g. a message will be sent to facebook after a comment is posted) without hitting the database.

As an aside, it's best not to use after_save/after_update for doing things such as sending emails as the messages will get sent regardless of whether the transaction fully completes (e.g. if posting a comment and doing something else in the same transaction).

BRMatt commented Jun 22, 2012

@agraves In my opinion AR callbacks are not a solution. Using them forces you to run an actual database transaction within your tests. With the code OP showed you can test that all the interactions (e.g. a message will be sent to facebook after a comment is posted) without hitting the database.

As an aside, it's best not to use after_save/after_update for doing things such as sending emails as the messages will get sent regardless of whether the transaction fully completes (e.g. if posting a comment and doing something else in the same transaction).

@jakcharlton

This comment has been minimized.

Show comment
Hide comment
@jakcharlton

jakcharlton Jun 22, 2012

@joevandyk "He put the responsibility for what happens when you post a comment into a single class."

When every controller action is a single call to another class that does the coordination of individual steps, then you made a useless abstraction.

The only purpose of a controller is to coordinate the conversation between the view and the model - in this respect @dhh's example is better, responsibility lies in the right place.

That said, the original code is ok, nothing massively wrong, it's not fantastic, and it certainly isn't a firing matter - as others have said, this isn't the whole story or there's a lot of history.

jakcharlton commented Jun 22, 2012

@joevandyk "He put the responsibility for what happens when you post a comment into a single class."

When every controller action is a single call to another class that does the coordination of individual steps, then you made a useless abstraction.

The only purpose of a controller is to coordinate the conversation between the view and the model - in this respect @dhh's example is better, responsibility lies in the right place.

That said, the original code is ok, nothing massively wrong, it's not fantastic, and it certainly isn't a firing matter - as others have said, this isn't the whole story or there's a lot of history.

@agraves

This comment has been minimized.

Show comment
Hide comment
@agraves

agraves Jun 22, 2012

@mguterl The whole point of this class is just to let you call one method to trigger a whole pile of calls. What's the point of making your interface that granular if the first thing you do is abstract it out?

Then you reply "well, in case you need to..." and stop mid-sentence, realizing that you're optimizing prematurely / violating YAGNI.

agraves commented Jun 22, 2012

@mguterl The whole point of this class is just to let you call one method to trigger a whole pile of calls. What's the point of making your interface that granular if the first thing you do is abstract it out?

Then you reply "well, in case you need to..." and stop mid-sentence, realizing that you're optimizing prematurely / violating YAGNI.

@jeremy6d

This comment has been minimized.

Show comment
Hide comment
@jeremy6d

jeremy6d Jun 22, 2012

@agraves I appreciate that you described the parameters of your judgment (more than some people did; I'm opposed to fundamentalism in religion AND programming). I'm simply expressing my opinion that a piece of code is hardly worth analyzing this way without being considered as a part of a whole system. That doesn't mean you CAN'T analyze it, I just don't think you get much out of it that's useful. I don't see any comments here that give me solutions; only comments that make assumptions and then judge the programmer based on something totally invented. That's fine if that's how you want to spend your time, I guess, but I wanted to offer a heterodox opinion.

jeremy6d commented Jun 22, 2012

@agraves I appreciate that you described the parameters of your judgment (more than some people did; I'm opposed to fundamentalism in religion AND programming). I'm simply expressing my opinion that a piece of code is hardly worth analyzing this way without being considered as a part of a whole system. That doesn't mean you CAN'T analyze it, I just don't think you get much out of it that's useful. I don't see any comments here that give me solutions; only comments that make assumptions and then judge the programmer based on something totally invented. That's fine if that's how you want to spend your time, I guess, but I wanted to offer a heterodox opinion.

@agraves

This comment has been minimized.

Show comment
Hide comment
@agraves

agraves Jun 22, 2012

@BRMatt "Using them forces you to run an actual database transaction within your tests."

You need a transaction around the whole thing anyways, regardless of where the code lives, because the operation should be atomic! Furthermore, the original code is wrong because it persists the comment to the DB before you even check whether it's spam!

"As an aside, it's best not to use after_save/after_update for doing things such as sending emails as the messages will get sent regardless of whether the transaction fully completes"

Um, no. You shouldn't save the record until after the validation checks have gone through. It's easy to break the callback chain by adding a validation error or returning false from something up the chain.

The only nasty part is figuring out what to do if the save and fb post succeed and the email fails, though I suppose you could issue a fb delete and a db rollback.

agraves commented Jun 22, 2012

@BRMatt "Using them forces you to run an actual database transaction within your tests."

You need a transaction around the whole thing anyways, regardless of where the code lives, because the operation should be atomic! Furthermore, the original code is wrong because it persists the comment to the DB before you even check whether it's spam!

"As an aside, it's best not to use after_save/after_update for doing things such as sending emails as the messages will get sent regardless of whether the transaction fully completes"

Um, no. You shouldn't save the record until after the validation checks have gone through. It's easy to break the callback chain by adding a validation error or returning false from something up the chain.

The only nasty part is figuring out what to do if the save and fb post succeed and the email fails, though I suppose you could issue a fb delete and a db rollback.

@mguterl

This comment has been minimized.

Show comment
Hide comment
@mguterl

mguterl Jun 22, 2012

@agraves - I want to isolate how the controller interacts with my application, not because of something I may need to do in the future. Please don't assume you know why I'm going to say something.

mguterl commented Jun 22, 2012

@agraves - I want to isolate how the controller interacts with my application, not because of something I may need to do in the future. Please don't assume you know why I'm going to say something.

@agraves

This comment has been minimized.

Show comment
Hide comment
@agraves

agraves Jun 22, 2012

@jeremy6d Yeah, you're making a good point, and though I disagree with you somewhat, I respect it. It's totally valid to point out that the "right" pattern is highly contextual. Legacy code or extremely complex business logic flows could make this pattern not only appropriate but necessary.

That being said, I think if there were extenuating circumstances the original poster would have highlighted them, and assuming a standard, simple Rails app, I stand by my approach.

I heavily emphasize using the shortest readable solution to any problem at work, and that metric has served me well, but I recognize other smart people will do different things, especially in strange circumstances.

agraves commented Jun 22, 2012

@jeremy6d Yeah, you're making a good point, and though I disagree with you somewhat, I respect it. It's totally valid to point out that the "right" pattern is highly contextual. Legacy code or extremely complex business logic flows could make this pattern not only appropriate but necessary.

That being said, I think if there were extenuating circumstances the original poster would have highlighted them, and assuming a standard, simple Rails app, I stand by my approach.

I heavily emphasize using the shortest readable solution to any problem at work, and that metric has served me well, but I recognize other smart people will do different things, especially in strange circumstances.

@mguterl

This comment has been minimized.

Show comment
Hide comment
@mguterl

mguterl Jun 22, 2012

@agraves - you don't know if persisting the comment whether or not it is SPAM is wrong or not. Maybe they want to store comments either way and maybe SpamChecker#check_spam handles marking the comment so it is not visible.

mguterl commented Jun 22, 2012

@agraves - you don't know if persisting the comment whether or not it is SPAM is wrong or not. Maybe they want to store comments either way and maybe SpamChecker#check_spam handles marking the comment so it is not visible.

@agraves

This comment has been minimized.

Show comment
Hide comment
@agraves

agraves Jun 22, 2012

@mguterl That's what model methods are there for. Our friend has just let all of the complexity bleed up from the model only to add another class on top of it to simplify it back down.

agraves commented Jun 22, 2012

@mguterl That's what model methods are there for. Our friend has just let all of the complexity bleed up from the model only to add another class on top of it to simplify it back down.

@reinh

This comment has been minimized.

Show comment
Hide comment
@reinh

reinh Jun 22, 2012

This code was initially implemented with DCI (I did get the "go ahead" for this). The stakeholders had issues with DCI, as did I over time. I updated this code to use a more generic OOP approach, but kept the modeling-around-use-cases aspect. Still, they had issues with it. I got a call from the stakeholder saying "This is not working out. The philosophical differences between you and the new [senior dev] are just too great." This all happened with absolutely zero code reviews or "warnings".

Yep, that's a terrible way for management to handle the situation. Absolutely terrible.

reinh commented Jun 22, 2012

This code was initially implemented with DCI (I did get the "go ahead" for this). The stakeholders had issues with DCI, as did I over time. I updated this code to use a more generic OOP approach, but kept the modeling-around-use-cases aspect. Still, they had issues with it. I got a call from the stakeholder saying "This is not working out. The philosophical differences between you and the new [senior dev] are just too great." This all happened with absolutely zero code reviews or "warnings".

Yep, that's a terrible way for management to handle the situation. Absolutely terrible.

@agraves

This comment has been minimized.

Show comment
Hide comment
@agraves

agraves Jun 22, 2012

@mguterl Good point, I didn't think of that! I'm still suspicious, but...touché.

agraves commented Jun 22, 2012

@mguterl Good point, I didn't think of that! I'm still suspicious, but...touché.

@ntl

This comment has been minimized.

Show comment
Hide comment
@ntl

ntl Jun 22, 2012

Pretty cool bikeshed.

There's a degree of complexity that would warrant moving comment creation to a separate class. Given the code samples here, I'm on the "keep it in the controller" side, but our requirements wouldn't have to get much more complex to flip my opinion.

If your contract were indeed terminated over this one incident, then I agree with others that your problem is that you need to find better clients.

ntl commented Jun 22, 2012

Pretty cool bikeshed.

There's a degree of complexity that would warrant moving comment creation to a separate class. Given the code samples here, I'm on the "keep it in the controller" side, but our requirements wouldn't have to get much more complex to flip my opinion.

If your contract were indeed terminated over this one incident, then I agree with others that your problem is that you need to find better clients.

@mguterl

This comment has been minimized.

Show comment
Hide comment
@mguterl

mguterl Jun 22, 2012

@agraves - I don't want the spam checking to be lumped in with the model validations. I believe whether a comment is spam or whether the comment is valid are two separate concerns.

mguterl commented Jun 22, 2012

@agraves - I don't want the spam checking to be lumped in with the model validations. I believe whether a comment is spam or whether the comment is valid are two separate concerns.

@mguterl

This comment has been minimized.

Show comment
Hide comment
@mguterl

mguterl Jun 22, 2012

@agraves - I would want spam checking to happen outside of the response / request cycle in the application. In many solutions with Rails this would require you to persist the comment first, then pull it from the database in a worker.

mguterl commented Jun 22, 2012

@agraves - I would want spam checking to happen outside of the response / request cycle in the application. In many solutions with Rails this would require you to persist the comment first, then pull it from the database in a worker.

@agraves

This comment has been minimized.

Show comment
Hide comment
@agraves

agraves Jun 22, 2012

@mguterl Depends for me on whether it's trivial (validations) or expensive (workers / dirty flag).

agraves commented Jun 22, 2012

@mguterl Depends for me on whether it's trivial (validations) or expensive (workers / dirty flag).

@jakcharlton

This comment has been minimized.

Show comment
Hide comment
@jakcharlton

jakcharlton Jun 22, 2012

@mguterl spam checking outside the request cycle is a whole different requirement ... I suspect that wasn't the case here. If you did want that, I would do it a completely different way, pass the comment as a message to an asynch worker, and deal with everything beyond validation there - I definitely wouldn't save to the DB first (that's state based programming at its worst)

jakcharlton commented Jun 22, 2012

@mguterl spam checking outside the request cycle is a whole different requirement ... I suspect that wasn't the case here. If you did want that, I would do it a completely different way, pass the comment as a message to an asynch worker, and deal with everything beyond validation there - I definitely wouldn't save to the DB first (that's state based programming at its worst)

@unclebilly

This comment has been minimized.

Show comment
Hide comment
@unclebilly

unclebilly Jun 22, 2012

I agree with @mperham - do what is reasonable for the place you are contracting with. If they can't handle new ideas, or they don't like yours, either adjust or move on. Having rapport with your co-workers is as important as the code you write. If you wrote this code knowing it would piss someone off, then maybe it wasn't the right thing to do - even though you feel strongly about its objective worth. I'm willing to bet that you would be more valuable to that company if you had got shit done "the wrong way." After all, companies exist to do things and support people economically while doing them, not to produce the Ideal Codebase.

unclebilly commented Jun 22, 2012

I agree with @mperham - do what is reasonable for the place you are contracting with. If they can't handle new ideas, or they don't like yours, either adjust or move on. Having rapport with your co-workers is as important as the code you write. If you wrote this code knowing it would piss someone off, then maybe it wasn't the right thing to do - even though you feel strongly about its objective worth. I'm willing to bet that you would be more valuable to that company if you had got shit done "the wrong way." After all, companies exist to do things and support people economically while doing them, not to produce the Ideal Codebase.

@BRMatt

This comment has been minimized.

Show comment
Hide comment
@BRMatt

BRMatt Jun 22, 2012

@agraves

You need a transaction around the whole thing anyways, regardless of where the code lives, because the operation should be atomic! Furthermore, the original code is wrong because it persists the comment to the DB before you even check whether it's spam!

Yes, but if you're not using AR callbacks you can simply mock the .transaction call. Also that might be intentional; we don't know much about the requirements of this code. For all we know the customer wanted to review all spam comments in case of false positives, in which case persisting to the db is fine.

Um, no. You shouldn't save the record until after the validation checks have gone through. It's easy to break the callback chain by adding a validation error or returning false from something up the chain.

I wasn't talking about validation rules, I was talking about the comment observer which sends an email using the after_save callback. Suppose you're registering a user and creating an order for them in the same transaction. Using after_save, if user is created but the order creation fails then the user will still receive an email, even though their account no longer exists. Just because you've run the validation doesn't mean that the record will always be commited. Suppose there's a unique index on the a field, if two users submit the same form at the same time with the same value then it's still possible for both validations to succeed, however the last one to commit will fail.

The only nasty part is figuring out what to do if the save and fb post succeed and the email fails, though I suppose you could issue a fb delete and a db rollback.

Assuming you're sending the email in the background you can just retry later. Usually notifications aren't as big a deal as saving the record.

BRMatt commented Jun 22, 2012

@agraves

You need a transaction around the whole thing anyways, regardless of where the code lives, because the operation should be atomic! Furthermore, the original code is wrong because it persists the comment to the DB before you even check whether it's spam!

Yes, but if you're not using AR callbacks you can simply mock the .transaction call. Also that might be intentional; we don't know much about the requirements of this code. For all we know the customer wanted to review all spam comments in case of false positives, in which case persisting to the db is fine.

Um, no. You shouldn't save the record until after the validation checks have gone through. It's easy to break the callback chain by adding a validation error or returning false from something up the chain.

I wasn't talking about validation rules, I was talking about the comment observer which sends an email using the after_save callback. Suppose you're registering a user and creating an order for them in the same transaction. Using after_save, if user is created but the order creation fails then the user will still receive an email, even though their account no longer exists. Just because you've run the validation doesn't mean that the record will always be commited. Suppose there's a unique index on the a field, if two users submit the same form at the same time with the same value then it's still possible for both validations to succeed, however the last one to commit will fail.

The only nasty part is figuring out what to do if the save and fb post succeed and the email fails, though I suppose you could issue a fb delete and a db rollback.

Assuming you're sending the email in the background you can just retry later. Usually notifications aren't as big a deal as saving the record.

@eweise

This comment has been minimized.

Show comment
Hide comment
@eweise

eweise Jun 22, 2012

Part of the problem is that we don't know the context in which the code was written. This is clearly business logic and should reside wherever the rest of the business logic resides. If this is a monolithic rails app where the domain model boundary is defined from the controller level down, he should should follow that approach. On the other hand, if there are other entry points such as web services, then the logic should be pushed down one level into a more encapsulated domain model.

eweise commented Jun 22, 2012

Part of the problem is that we don't know the context in which the code was written. This is clearly business logic and should reside wherever the rest of the business logic resides. If this is a monolithic rails app where the domain model boundary is defined from the controller level down, he should should follow that approach. On the other hand, if there are other entry points such as web services, then the logic should be pushed down one level into a more encapsulated domain model.

@omarabid

This comment has been minimized.

Show comment
Hide comment
@omarabid

omarabid Jun 22, 2012

So if you did it wrong you get fired for it? I don't think it's reasonable to judge your performance on a single issue. You certainly can't be aware and well-rounded in everything. Did he make comments on your work before that?

omarabid commented Jun 22, 2012

So if you did it wrong you get fired for it? I don't think it's reasonable to judge your performance on a single issue. You certainly can't be aware and well-rounded in everything. Did he make comments on your work before that?

@dball

This comment has been minimized.

Show comment
Hide comment
@dball

dball Jun 22, 2012

It's easy to write models with callbacks and controllers with filters. It's hard to reason about them, particularly when you're not the author. It's cumbersome and error-prone to selectively skip the callbacks and filters when the inevitable use case arises where you don't want the callback behavior: when you're batch importing comments, for instance.

dball commented Jun 22, 2012

It's easy to write models with callbacks and controllers with filters. It's hard to reason about them, particularly when you're not the author. It's cumbersome and error-prone to selectively skip the callbacks and filters when the inevitable use case arises where you don't want the callback behavior: when you're batch importing comments, for instance.

@justinko

This comment has been minimized.

Show comment
Hide comment
@justinko

justinko Jun 22, 2012

Let me be clear. I have zero issues with creating controllers like this:

class CommentsController
  def create
    current_user.comments.create!(params[:comment])
    head :created
  end
end

A post-a-comment class is absolutely not needed in this case. Following the "rails way" is perfectly fine.

I find it ironic that @dhh, who does not hold the highest opinion towards OOP patt