Navigation Menu

Skip to content

Instantly share code, notes, and snippets.

@justinko
Created May 30, 2012 19:40
Show Gist options
  • Save justinko/2838490 to your computer and use it in GitHub Desktop.
Save justinko/2838490 to your computer and use it in GitHub Desktop.
Am I doing it wrong?

Dear Rubyists,

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

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

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

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

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

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

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

Thank you.

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

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

@aq1018
Copy link

aq1018 commented Nov 17, 2012

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment