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

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