Am I doing it wrong?

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

# 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

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