public
Last active

Am I doing it wrong?

  • Download Gist
Plea.markdown
Markdown

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.

post_comment.rb
Ruby
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36
# 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

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

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

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.

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.

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.

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

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.

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

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.

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.

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

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.

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.

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

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?

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.

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.

:+1:

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.

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

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.

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.

Looks fine and... symmetrical. Thumbs up!

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.

@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 sounds like someone was writing PHP in Rails

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.

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.

'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

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.

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.

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

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.

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?

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.

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

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.

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

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

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

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

@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, 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.

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 you can't say it like that. It's "Should I stay or should I go?" :guitar:

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

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

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

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.

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.

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!

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

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

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.

@ mguteri:

I already told you.

@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:

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

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

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.

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

@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:

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.

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

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

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.

@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?

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

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

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!

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! :)

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.

@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".

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

@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

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 I'm not sure that you know what meta-programming is.

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

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

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.

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.

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

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.

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 fair enough. It's hard to discuss concepts like these without specific cases. This case was probably overuse, others are not.

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

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.

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

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.

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

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

@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."

@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?

@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?

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.

@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?

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

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.

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.

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

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

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

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

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

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

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.

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

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.

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

@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 Depends for me on whether it's trivial (validations) or expensive (workers / dirty flag).

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

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.

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

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.

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?

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.

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 patterns and principles, conveys us to use the patterns of Rails. I find Active Record callbacks, for example, to be the most unnecessary abstraction of all.

I was recently on a project where an AR model's test had a setup method that stubbed FIVE external services (called from callbacks). So no, Mr. Hansson, FUCK ME: https://twitter.com/dhh/status/216242159496609793

Another +1 from me, I wish more rails codebases looked like this. I would not sweat losing the contract.

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

If you don't do it much already, I thoroughly recommend doing code reviews and pair programming. Aside from the increase in code quality, validating your work with other developers goes a long way towards building your own confidence. Having one random developer who happens to be labeled 'senior' not like your code shouldn't really make a dent in your motiviation!

@agraves I'm guessing that this story is largely fictional and certainly biased, so I'm merely judging the fictional managers from the story. What I mean to say is if managers act like they are purported to act here, then they are terrible managers.

To put it simply, this project lacked technical leadership. When technical leadership was injected, via the "senior developer", I was ejected based on philosophical differences.

Here is a little more detail. I was working on the new comments feature for over a month. The diff with master was massive. When it came time to determine whether the branch would be merged, the senior dev, and some other members of the team, decided it could not be merged.

Next step: easier to let me go then have me redo the branch/feature (which also used Ember.js, they wanted straight jQuery...).

@justinko from what you are saying ...

The team had a way of doing things, and apparently agreed on it. You detached from the team for a month and diverged significantly from their codebase. You used a technology choice they didn't want, and presumably didn't match the rest of their solution.

I can't see them being in the wrong here from what you said.

@hcarvalhoalves agreed, I was just writting the same. I would also expect the LanguageDetector to be a bit more general (what if I want to detect language for something else than comment?) Or what happens if it's a spam? Well, you need to look into all the other classes to find out. If this is what SRP means, I'm not interested.

IMO this happens if design pattern wins over a common sense.

KISS, if you are going to use those methods (detect language, check for spam ...etc) exclusively on the Comment model, then what's wrong with the tight coupling? more classes = extra maintenance, that is my humble opinion.

I created a fork, with some pseudocode that I'd prefer for the example above:

https://gist.github.com/2975264

It's based on the ideas:
1. you need to separate persistence from your domain
2. you need to have a proper use-case, not a single request-class, like above.
3. notifications are notifications (twitter, fb), they are implemented with AOP
4. spam detection and language detection are separated (discussable) with an aspect.
5. there is an "app" object that wraps it all together.

People don't use inner classes enough. It can be annoying to track down how things work when there's a bajillion classes in separate files, but if they are all simple: consolidate them. Define mini-models inside your controller! (And mini-models inside your AR models too)

And when that makes your files too big, make folders and use modules to namespace.

PostComment is too generic, but PostsController::PostComment is perfect.

To +1 what many people have already said: testing! Much much easier to test discreet, single purpose objects.

@jacobo +1 on inner classes. i am a huge fan of FooController::BarPresenter since it's simple to track down, easy to test without a mock request, and i don't have to bloody think about class loading.

The code here seems almost irrelevant. @justinko wasn't fired for this code, or even necessarily over a philosophical difference about how to design software. It sounds like he was fired because it was more important for someone (he, his bosses, or both) to be right than to deliver a product.

A codebase isn't just the set of symbols and incantations you type to make a computer do something, it's also a living document of a team's shared understanding of what the software does and is supposed to do. It follows that a team should all be more or less in alignment about how the code should be structured so that everyone understands it. Code that nobody understands or agrees with is not maintainable because no one will maintain it. It's not more stable or solid, because developers who don't know or believe in these design principles will violate them, making the codebase less comprehensible and introducing even more bugs.

This could be read as me saying developers like Justin who believe in this kind of principle-driven development are being too clever, or that the client team were a bunch of idiots trying to write (as one earlier poster put it) "PHP in Rails." Both may be true. My point isn't about which style is correct — it's that the correct style is the one everyone can agree on, that people working together is more important than shipping perfect code.

And that being said, it's worth noting that just as most office memos are written at (at best) a middle school level (give or take some jargon), depending on the kind of organization and the work they're doing, most code is probably also written at a much lower level of sophistication than Justin's example above, and I'm not entirely sure that's a bug. A naïve structure is not a bad thing in itself, and it may represent a trade-off between correctness and writing code everyone can understand. A more sophisticated structure has its uses and benefits as well, but demands more from the engineers who'll have to live with and maintain it.

Too much speculation on the actual contract cancellation. They could've been looking for an excuse, he didn't merge branches more often, they realized he was too tall, etc. A gist won't tell you why he was canned but it highlights :-1: to premature abstraction. Agreed with @dhh in the initial assessment but extracting chunks of logic into a class does occur at some point as a method of refactoring - just not here.

The only true fact about this gist is GitHub needs to add the ability to disable notifications.

If there's already comment model, then I would probably say this was unnecessary.

This -github- comment model is for instance GARBAGE!. how can i stop the notifications?

So, it appears THIS was the REAL issue (from @justinko):

Here is a little more detail. I was working on the new comments feature for over a month. The diff with master was massive. When it came time to determine whether the branch would be merged, the senior dev, and some other members of the team, decided it could not be merged.

Next step: easier to let me go then have me redo the branch/feature (which also used Ember.js, they wanted straight jQuery...).

No way working in a branch for over a month is acceptable. Were you even merging up from master? Did you know going into it that they wanted straight jQuery? If introducing a new framework (like Ember), seems like you'd want to check with them, maybe do a quick prototype for them to evaluate, SOMETHING that ensures that you're headed down a decent path. Were there any code reviews from them along the way?

Again, I'm very, very sorry you were let go. But it seems clear that your approach and the issues with it being too hard to merge was just the "rubber meets road" moment that revealed a lack of coherent/consistent approach and maybe even a lack of communication/check-in. Probably not only your fault, but if you are contracting or new, you should learn to over-communicate and check-in early and often when doing something that you know is different.

I know that I wouldn't approach things the way you and/or the company did just for these reasons. Regardless of how you all got there, I have to say, as a hands-on-developer and a manager, that if I had someone working for a month on a feature that we needed and at the end of the month it ended up being too divergent to be reasonably used and/or I was surprised by the implementation, I probably would have done the same thing.

The merits of your approach are NOT the real issue here and all of the folks comments on SRP and other items really miss the point. Right, wrong, good, bad, you didn't write something they could reasonably incorporate, and they paid you for a month of work for it. Sorry.

I agree with this approach of separating out business logic / use cases but don't agree with your implementation.

Your PostComment collaborates with user, entry and attributes and have @comment created while doing post. I suspect the fact you pass in body and attributes and create @comment while doing "post" is purely to accomodate the "create" action of the controller. I would take comment creation out of "post" method and pass in comment either as a parameter to post or the contructor. I'd make the first few lines of the "post" method a "update_with_body_and_attributes" method in the comment model. This way if your comment creation logic changes it can evolve separately. Also, if you ever need to post an existing comment, you get it for free.

There is no return value nor error condition handling for LanguageDetector, SpamChecker, CommentMailer, PostToTwitter or PostToFacebook. Or do they communicate by mutating @comment? - I hope that's not the case. So, do I still send out a comment email if it's spam? If any of these wraps third party API that could raise exceptions, should that short circuit the whole process? Shouldn't we still post to facebook even we cannot post to twitter somehow?

@agraves, specifically about the example you posted: I used to structure code in the same way (which, I think, is a relatively standard way to structure Rails code), but I recently came into a project done in this way and it was really hard for me to grasp what the flow of any given feature is.

In theory, moving things to orthogonal behavior to observers makes sense, but you lose a very important property--the ability to communicate what gets done exactly when a comment is created. In that respect, I think @dhh's example is clearer, in that you can easily see what's going on.

IMO, @dhh's example fails in a few areas:

One of the before filters (#set_entry) should be specifically used on instances where you know you have an id, therefore doesn't apply to #index or #new, forcing you to add an :except clause. #reject_spam is even more restrictive, only applying to #create and #update (more except clauses). While this may not be a problem, it still adds more complexity than what you'd get if you treated the filters as simple methods that you'd call from the controller action.

Another area where @dhh's example will begin to fall apart is when you need to add more actions, and more helper methods to that controller. Suddenly you have a mess of private methods which you aren't exactly sure about where they're being used until you search for the method name.

Personally, as an individual with a very limited mental capacity, I like to see related code wrapped in a logical abstraction. This allows me to reason about the behavior as a whole, and allows me to easily grasp what it does. In that respect, I think you may have the same problem, but on a different place (Comment). In the standard Rails structure, some classes tend to grow a lot, with lots of callbacks that are used on different contexts and prevent you from easily grasping what exactly is done in each context.

So I'd tend to lean to the way @justinko did it, maybe not abstracting it so much, but I guess that can only be judged when you know the whole context and can look at all the source code that comprises this example.

I think we need to realize that this pattern (Callable as object) is partly a workaround for the fact that you can't easily define methods inside other methods (like, say, in Scheme), but I also don't understand what's all the fuss about unnecessary abstraction. Sure, if you extract a class for every line of code, that definitely seems like unnecessary obstraction. But wrapping related behavior in a different class and then calling it from somewhere else is only introducing a single class. Why's that different from introducing a method, say, in Comment? You still have to jump to a different file to see it.

Anyway, this is big enough already so, to summarize, my main point is. Behavior should be as explicit as possible and grouped in small modules that allow you to grasp them as a whole and clearly understand them. In my experience that will make the code easier to maintain, and conceptually simpler. I think we should reject the mantra of oversimplification as much as the mantra of overabstraction.

Just wondering, how many of people proposing OO style had Java experience before.

I'm going to post a little something that could potentially help you all with coming up with the best solution for this problem, and it has nothing to do with knowing the correct design pattern yourself. When you engage in a contract with a client as a consultant or a contractor especially (though this is pretty applicable when you work for them too), you have the power to decline the contract, and to set your terms. Whenever I engage in contracts with clients, one of the first things I do is redefine the problem they are asking me to solve in cultural terms. They may have contacted you for a "rails guru with x years experience and has used y javascript technologies" but most likely, they are really looking for someone who will a) add value to their business in a reasonable period of time and b) fit in with their process and culture. The latter is the most important for you to be viewed as successful in their eyes (though the business will judge your team's output on the former, which is usually out of your control).

On some teams there is an unwritten understanding that innovation in technologies and patterns is expected. There won't be a wiki, book, or documentation whatsoever to point to their "right" way of doing things and you have a lot of leniency in coming up with an approach for implementing a feature. At others, there are specific patterns the team has already agreed upon as being "the best way" and your default approach should be to use them. When you engage the client to start the contract, it serves you well to find out whether they are looking to you for help with improving their architecture and patterns, or simply to get features completed and to market within the patterns already in place.

That being said, regardless of what their cultural approach towards innovation in the code is, it has served me well in my career to adopt a policy of early communications when it comes to doing anything that might be new to a team. Whether I'm brought in to setup good patterns or not as soon as I find a decision point where I am asking myself whether people will want to use my pattern or they may have thought through this already, I stop what I'm doing and get some face time with technical stakeholders and run it by them. While it's true that you can't do this at every opportunity (analysis paralysis/being overly paranoid about stepping on someone's toes) it's better to over-communicate than to come in at the end with "check out this great pattern I came up with!" when you're invested with several days of work that may or may not meet their technical acceptance criteria.

The early communication @eavonius explains is a good thing. You must be quite experienced thou, to find out these kind of things about your client in that early stage.

I personally am totally for the so called "over-engineering", meaning proper interfaces, early abstraction of reused, moving parts, etc which your "senior developer" disliked. Having been in similar situations I now tend to "accept" a "lower" level of abstraction, respecting the client's architectural view instead of insisting on introducing a new class or whatever. I no longer see it as my job to force clients to go the way I would usually go. Instead of being angry and seeing it as a personal fail I couldn't enforce my design, I take a deep breath and wait for the evening to come so I can work on my open-source projects - where I can over-engineer at will!

Very interesting discussion, thanks Justin.

One of the things I really like about rails is though it might not be the most classic example of existing OO principles it's got enough abstraction to build features fast and gives you 80% of the functionality you need to build any reasonably compelling web or mobile application. Many other camps (.NET and Java, I'm looking at you) suffer from having such an open palette you come onto a project with works of art for architecture and tons of strong opinions (stronger than these related to nuances around rails) without being vetted by a big open source community like we have here.

I like having patterns a community or team has generally accepted as "good enough" when starting on something. Most of us, unless we're working for a technology company that sells APIs, are in the business of providing customer value and innovating around usability and solving their problems, not technical ways of doing it. I love it too when the way I solve a problem on a project results in some "aha" moment or unique pattern, but if that comes at the expense of actually delivering something the business can use, it's not going to be long before they find someone else who can get features out faster to replace me.

@eavonius

Funny you mention speed :)

Besides the addition of a class (PostComment), @dhh's code and mine are largely the same; He argues to keep it in the controller, I argue to put the algorithm in a separate class.

The biggest gain from my implementation is that I can test the logic outside of Rails. What I mean is, I don't have to "load" the Rails environment to test the creation of a comment. The difference in test speed is huge, which leads to easier and faster refactoring.

I don't understand how someone can say testability doesn't matter. Have they ever tried to unit test a 100+ line function? Or a system call? Or a DSL? In the context of this example, if I keep it all in the controller, I have to deal with things that have nothing to do with the algorithm. For this controller, all I care about is its responses - not whether it posted the comment to Twitter!

People that haven't been affected by slow test suites should stop expressing their opinions because they are clueless.

Controllers in Rails SUCK because they can't be PORO and you can't test them in isolation. You need to load Rails with all its stuff to run a test for a class that handles your domain logic. It's not about using fancy patterns, it's about fast, isolated unit tests. We can discuss how to do it, sure, but one thing is certain - DON'T handle domain logic in controllers because it is not easy to unit test them.

I'd like to add that having your domain logic completely isolated from Rails (that is, testable without loading it) also helps a lot when upgrading Rails versions, in big upgrades such as 2.3.x -> 3.x. Because effectively your application hasn't changed, only Rails has, so why the hell would you be updating every single file in your application.

You did it right, in my opinion. You have a single, clearly named class that completely encapsulates the logic required to create a comment.

It doesn't matter that there's only one place that logic is being used: who cares? Code isn't for the system, code is for future programmers. A future programmer would see the call in the controller to PostComment.new.post (or whatever) and just have to think "A ha! I need to look at the PostComment class to see all those details.".

Write controller filters and model callbacks and now the future programmer has to think about how many different pieces of the system fit in to come up with a complete story of what happens when a comment is posted.

Your tests can be a great indicator of what a future programmer will have to think about when they reason about your code. If you only have to load one class that you are reasoning about: then you can be reasonably assured that a future programmer should at least be able to follow the high level logic. If you have to load all of Rails in order to reasonably test your logic: then it's likely that your logic is spread out through the application and will be difficult for the future programmer to easily reason about.

Initially for few moments, the code looked alright. However after staring for some time, I feel that code of post method lacks consistency. There are few methods that manipulate @comment, then there are calls to helpers such as "LanguageDetector.new(@comment).set_language" followed by invocation of methods that do not accept any parameters. post_to_twitter is a single line method that does not accept any parameter, but its implementation is single line and is similar to previous 3 lines where other helpers are invoked. I am not a big fan of methods that do not accept parameters and do not return any parameters and full of side effects. Also, I felt that post method had codes with three level of abstractions - direct manipulation of @comment, invocation of helper and invocation of a method that internally invoked a helper.

I will agree to @DHH that these could have been done in the Model and still would have had appeared organized. If one truly needs helpers, they should act on the parameters passed and should not rely on instance variables as was the case in post_to_twitter.

@srikanthps I agree. That's what code reviews are for! Thanks.

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

    PostToTwitter.new(@user, comment).post              if comment.share_on_twitter?
    PostToFacebook.new(@user, comment).action(:comment) if comment.share_on_facebook?

    comment
  end
end

I think it's good for you not working in an environment which has a different and strange philosophy on how code should be written anyway.

Once more, proof that Rails just doesn't scale.

Know that kind of "senior" developers too ;)

Ruby is an object oriented programming language. To program in Ruby without using object oriented principles is retarded. To eschew these practices is to undermine the very foundation of the language, a crime for which the punishment is buggy, error-prone, inefficient, illogical, and unfathomable code.

@justinko I think your code is fine, for what it's worth. I'd encourage a more regular review schedule in future though, but be confident that your solution is clean and elegant, and don't let this get you down.

Good luck!

@gavinlaking dude, never heard of functional Ruby?

Ruby is an Object-Oriented Programming language. For an OO language, it has
exceptionally good support for some functional-style idioms, which are
particularly useful when implementing "query" (as opposed to "command")
methods. But to organize a large Ruby program along other than OO lines is
to work against the grain of the language. For better or for worse, Ruby is
NOT an acceptable Lisp.

On Thu, Jun 28, 2012 at 4:39 PM, Gavin Laking <
reply@reply.github.com

wrote:

Ruby is an object oriented programming language. To program in Ruby
without using object oriented principles is retarded.

I agree with the sentiment, but I dislike the use of the term "retarded" in
this context. We are not in 8th grade here.

It sounds like the senior dev wasn't onboard with the DCI-ish approach and maybe felt he was loosing an understanding of a codebase he may one day need to work on. Personally, I like your code. A project needs standards/constraints/conventions/consistency which the whole team understands and endorses. All in all I don't think there is an issue with your code quality, but your code style. Not a case of better/worse, just different, with different Pros/Cons. No right or wrong, but the senior dev is paying ;)

@SleepTillSeven I've read a bit about it, but I stand by my thoughts that if you're using an OOP language then using good OO principles is de rigueur. For example, I personally don't like to use or see ActiveRecord callbacks, simply because in my experience they are a source of unexpected/hidden/obscured behaviour in applications. @justinko appears to be trying to keep functionality throughout his app in self-contained units which will simplify testing, make the app more extensible as business concerns change and easier to understand what is happening to those who have to maintain it in the future.

@avdi / all, my apologies, the "retarded" comment wasn't particularly helpful or mature.

@gavinlaking I support your use of any word you want to use, regardless of whether other people like it or not. I don't come here for the language police.

We are not in 8th grade here.

@avdi speak for yourself! no actually I agree.

@justinko I agree with the peeps telling you that the contract-killing problem here was not design philosophy but mismanagement, but I would definitely say take anything beyond that with a grain of salt. there's people saying it was "obviously your fault" or "obviously the other person's fault" but there's absolutely no solid evidence to back up either position. people project and assume in these conversations, because people-skills problems and project-mgmt problems have a huge subjective component. you should filter their remarks accordingly.

long story short, you probably need to get better at choosing projects, and/or handling people issues within those projects. could be that the company was fucked from jump and you should have never gotten on board to begin with. could be that you mishandled it and made a mess where there didn't need to be one. only you can decide for sure.

I'd also really urge you to take DHH's aggressive attitude with a grain of salt. the guy is not always nice. everybody knows it, including him. if it helps, keep in mind that he also created a framework which transformed a lot of careers for the better, so his personal abrasiveness may be balanced out by this non-specific, impersonal generosity.

re the code, DHH's disregard for OOP is not something I endorse, and I totally disagree with his testing philosophy, but I think his version is much tidier than yours. (of course I'd also echo his caveat that things change if you re-use that logic elsewhere.)

the most important thing in my opinion is how clear the code is to read, and how easily you can test it. DHH's solution is short and concise. your code is too repetitive and has too many lines. Ruby code is usually bad when it has that many lines in it, and when they're structured that way. to be fair, however, DHH hasn't supplied the tests which would accompany his solution, and in my personal opinion, Rails controller testing is pretty dooky.

if we had a version of your code which could compete with DHH's in terms of its readability, I think this would be a very interesting debate, but right now I think arguing philosophy is pointless. you need to write succinct code before subtle distinctions between philosophies matter.

What I don't understand in the original submission is how check_spam short-circuits a spammy request. There is no control flow for it, so either an exception is raised (yuck) or SpamChecker sets some flags (yuck) on the @comment plus some other hidden conditionals you have not shown. Your coworker was right, this is not good design.

The only thing I find strange is that this class is actually just a method in disguise (receives the arguments on the constructor, does the work on some other method). This can be a necessity (you capture the context needed to do the action in one place but want to execute that action in another place), code kinkyness, or wrong (using a class for this is too complex. It should be a method in a module).

For exemple, why aren't calls like this:

    LanguageDetector.new(comment).set_language

turned into something like this:

    comment.lang = LanguageDetector.detect(comment.text)

Either way, you're either 100% right or 99% right. Apart from this (very debatable) OO point, your code is correct on all accounts.

Your code is very much ok for what it does. @dhh alternative solution is retared as is his cluttered mega-framework. The reason why Ruby is going a bit down now, in popularity, is that the initial Rails bubble is slowly disappearing. So don't worry about the Rails way, your code is from OOP perspective very reasonable (SRR), and OOP will stay here longer than Rails.

@pkoch

LanguageDetector accepts an object with the following assumptions:

1.) Responds to update_attribute
2.) Has a content attribute
3.) Has a lang attribute

LanguageDetector#set_language will "detect" the language from content and set the lang attribute (via update_attribute). I feel this is an algorithm that should be encapsulated.

With that said, I would be completely fine with this:

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.lang = LanguageDetector.detect(comment.content) if comment.valid? # don't want to hit the language detection external service unless the comment will be persisted to the datastore
    comment.save!

    SpamChecker.new(comment).check_spam
    CommentMailer.new(comment).send_mail

    PostToTwitter.new(@user, comment).post              if comment.share_on_twitter?
    PostToFacebook.new(@user, comment).action(:comment) if comment.share_on_facebook?

    comment
  end
end

Even better, your PostComment is perfect example of Command pattern, and it's trivial to turn it into asynchronously executed one. The only thing belonging to controller, is comment validation to return the right response.

@justinko But that approach couples LanguageDetector with an object with that requirements for no good reason. You want to guess the language of the text represented by that string. So, take just the string. If you don't need baggage, travel light. :P

This has other benefits. For example, what if you had an object were two of it's attributes where to be checked for language? LanguageDetector would need to be informed of the fields.

Something as simple as this:

paper.abstract_lang = LanguageDetector.detect(paper.abstract_text)
paper.body_lang = LanguageDetector.detect(paper.body_text)

would need to alter LanguageDetector's implementation to support that kind of logic. So, a true design nazi could argue that it's an SRP violation. Except PostToFacebook (would have to read it to see what the argument's for), I could say this for any other class you're using.

However, do note this is tuning a very fine point. You already have the responsibilities reasonably well separated. Using @avdi's terms, changing this would be refactoring, not refurbishing.

I might go either way on whether the logic belongs in the model.

Imagine a scenario where you decide to move posting items to facebook or twitter to an external process with retries, and need to take care of error conditions. Maybe you have a requirement that comments appear on a twitter feed in the same order as posted locally. Or where comments which may be spam (I assume today you just throw an exception) gave the user the same feedback ("your comment will appear momentarily") but instead emailed the comment to a moderator to approve or deny.

The integration with external systems may become more complex and require a state machine. At this point, you probably need to have state persisted, and if you don't then decide to refactor and move some of this business logic into the model layer you will be exposing implementation details to the controller and increasing coupling.

The reverse side: comment in this case is both a noun and a verb. The act of commenting kicks off a business process of verifying appropriateness, saving data locally, and sharing with several social websites. The object for a comment represents the local concept of comments. For that reason, it may be worth having code outside of the Comment model class deal with the submission, verification, and external sharing of comments. This could be separate from both the local comment model and the comment-submitting controller.

But as long as you have a decent test strategy, the code seems maintainable and easily refactored should the business requirements change.

Does anyone know how to unsubscribe from this gist? :)

On Fri, Jun 29, 2012 at 4:59 PM, Alexey Petrushin
reply@reply.github.com
wrote:

Does anyone know how to unsubscribe from this gist? :)

If you have GMail, it's called 'Mute'. :P

@pkoch

The difference is my implementation is at a higher level of abstraction.

To support multiple "attributes", I could just change it to this:

LanguageDetection.new(comment, :abstract_lang, :body_lang).set_language
# or
LanguageDetection.new(comment).set_language(:abstract_lang, :body_lang)

Is this completely overkill? Perhaps. And as I said, I would be completely fine with your approach as well :)

I also agree with @pkoch that this class is a method in disguise and even agree with your senior. If you don´t need this code in more "modules" I see this as premature optimization - simply test this code with a controller test. Other aspects:

  • I dislike the names "PostToTwitter" or "PostToFacebook". Maybe that´s just personal taste but I would call them "TwitterPoster" and "FacebookPoster".
  • I don´t understand that code "LanguageDetector.new(@comment).set_language"? Calling a set method without an argument seems strange to me.
  • I personally dislike methods named "action" - this is like calling a method "do" or "make". Pick more precise names for methods.
  • Do you really need instances of "LanguageDetector", "SpamChecker", "PostToTwitter" and "PostToFacebook"? Remember that objects bundle state (variables) AND state transformations (methods), so maybe you applied premature optimization there as well? Sometimes simply inventing a method/function named "Util.detect_language(:comment)" is good enough.

Don´t get me wrong your overall code is quite ok. I think there are other reasons why you lost your contract.

@JustinKo:

We're mostly discussing things that aren't ours to discuss, I think.
Only adding more features and having a more complex/demanding
architecture will prove one of us wrong (or both! :P). That's not the
main point here.

On Fri, Jun 29, 2012 at 5:39 PM, thoefer wrote:

Don´t get me wrong your overall code is quite ok. I think there are other reasons why you lost your contract.

+1. My speculation is that the problem isn't your code, but rather
your eagerness to do things right off the bat. Some people will label
you as a "time waster" for that. They're morons[1]. As you can see
from the overwhelming responses in this thread, your lost client did
you a favor but avoiding a doomed cooperation.

By the love of $DIETY, keep improving your skills and looking for
people who can derive value (and pay you in return) from them. :)

Best of luck to your future endeavors!

[1]: thoefer: I'm not calling you a moron. Your points are valid.
However, instead of calling it "premature optimization", I would call
it "premature structure". I think both Don Knuth and spartan style
would agree. ;)

Did your code do what it was supposed to do, quickly? Congratulations, it is "correct". All other considerations are either style/preference, or perception of future problems given current design, the latter only coming with experience (with both Rails as well as the business in question).

Thus, this looks like an HR problem more than a technical one.

Personally, I always lean towards designs that are easily testable and that don't require me to fire up the whole Rails stack. But this, again, is just preference. And I do tend to fall into the YAGNI trap. Us programmers love to be clever. Don't get too clever, too soon. (note to self)

A senior developer once had a problem with me using the Ruby ternary logic operator. At all. I was like WTF? Well, do you know what the real problem was? He didn't like beer or coffee, and he was very afraid of puppies. Very afraid. Of puppies. :) Which is to say, we didn't mesh too well. It's OK, it happens. And I learned a lot from him anyway (he was the first guy to pound into my brain the importance of test coverage). And he is fairly successful now. Might even have a book or 2 out with his name on it.

This is an HR problem. The rest of you are enjoying your mental masturbation, however. :)

All due respect to the author of the last comment, getting code to do what it's supposed to do is the lowest threshold for acceptability in my book. Getting code to clearly communicate its intent and to be easy and safe to modify is a higher bar, but is the standard by which I judge any code that's intended to work for longer than a day. We spend more time reading code than writing code.

@dball Was halfway through saying the thing, but much less respectfully. +1

@dball Exactly! Like I say, "Code isn't for the system, code is for future programmers."

Could this be a case of rubbishing someone else's work so that the new "consultant" gets to take over the project and become a hire? I've seen it happen many times before.

I've been in similar situations before. You have to pick yourself up and find another contract. It's the risk you take.

For the record, I agree with other comments that it's probably premature refactoring but not something you should get fired over.

Respect, for posting though. A good way to get a great deal of feedback. I hope it helped you.

Throw in another two cents, whether it was premature or not, get back on that horse and keep going. We can't afford to lose programmers to bosses that don't understand.

To me the bigger problem is that someone fired you for something in your code. That's crazy. And if it wasn't for something in your code, they fired you for something else but couldn't be bothered to tell you what. That's also crazy.

I think the merits of one coding style over the other have been well covered, but this post makes me think more about the 'appropriateness' of straying from the norm in an existing codebase. So, under what context is the Senior dev actually correct in resisting this code? There is a tone in many comments here that the proposed code was better 'without question', and that these things can be judged in a vacuum, without need for any context, but that is rarely true. I've joined projects where I wanted to change a lot of things right away, and I knew that they were 'hands down' the best way to do things, but I think you have to choose your battles and introduce new concepts when appropriate. I've certainly seen a lot of people jump to a new thing and propose that it is the better way to do things, without really being able to articulate why in any concrete way. Then the 'new' thing is tainted, not because it is wrong, but because it is being proposed by someone that is not able to explain it, teach it, improve it, etc. I've been in Rails codebases where you can see a clear 'RailsCast flavor of the week' approach, with overlapping gems, copy&paste code with no tests, etc., and I think there is value in taking the time to understand a thing before you push it to production. How would the conversation have gone if they did a code review or paired? Great discussion, and great that Justin is seeking feedback on the code, and how to improve. I merely want to point out that there might be a component of working in a team, seeking the counsel of others (that employ/work with you), and other general Contract Developer skills at play here

+1 @jmccaffrey

I was once in a position where I fought to move my organization into better code and craftsmanship. I fought hard for object-oriented design, TDD, craftsmanship, and even professionalism, which was against the flow established by many higher than me. I knew I was right, I was seeing positive improvements in my own work and those who followed me, and I was getting buy-in from many. However, there were some who fought it (including some higher), and a big split developed in the department. Given enough time and bitterness, myself and similarly-minded devs left.

If you have a group of people using "use cases" and the others are just dumping code in the controller, it doesn't matter who is right or wrong -- the team is no longer working together. If the split is on fundamental principles, then perhaps it is best to separate.

Not even going to argue about the code or coding style, but I'd like to offer something in the human aspect.

  1. It is conceivable that @dhh wants us to believe close coupling is good because this makes a project more difficult to switch to framework other than his.
  2. It is conceivable that the 'senior developer' thought it'd be beneficial to eliminate his 'strong competitor'.

All too often developers think too much about programs, but left out the human factors. [ And conspiracy ( theories ) abound... ]

Please sign in to comment on this gist.

Something went wrong with that request. Please try again.