You are building a boring blog application. A blog has many comments. Your client gives you this user story:
So that I can report inappropriate comments, as a user, I can mark a comment as spam.
So you add a spam boolean attribute to the Comment model, and use the same
to comments to handle the update:
class CommentsController < ApplicationController def update @comment = Comment.find(params[:id]) if @comment.update_attributes(params[:comments]) flash[:notice] = "Comment updated successfully" # redirect_to ... else flash[:error] = "Something broke" # redirect_to ... end end end
By sending the right params down from the view, this can still work. But a day passes by and people start abusing the darn spam button. This is the internet after all. So your client adds a couple of stories:
So that I’m aware I screwed up, as a user, I receive an email notification when my comments get spammed. So that I can moderate spammed comments, as an admin, I receive an email notification when comments are spammed.
There’s now more going on when we spam a comment. A first shot at implementing these stories could make use of ActiveRecord’s dirty attributes to figure out if the spam boolean changed from false to true, in which case we deliver the two notifications:
class Comment < ActiveRecord::Base before_save :send_spam_notifications, :if => marking_as_spam? def send_spam_notifications Mailer.comment_owner_spam_notification(self).deliver Mailer.admin_spam_notification(self).deliver end def marking_as_spam? self.spam_changed? && self.spam? end end
Nice and clean, right? Wrong.I think there’s a couple of problems with this. First of all, we are cluttering our comment class with conditional callbacks. Sometimes you have no choice, but I’d rather avoid them when possible. Secondly, we’re using the same HTTP endpoint and controller action for two very different things. One is that of editing my comment, and the other is for marking a comment as spam — which is arguably an SRP violation (PDF link).
We have a couple of options for cleaning up. We could create a
#update_spam_status action on the comments controller, but this deviates from
our otherwise RESTful API. Instead, let’s break it out to it’s own resource
Spams, and instead of a
PUT to comment, let’s
POST to spam:
class SpamsController < ApplicationController def create @comment = Comment.find(params[:id]) if @comment.mark_as_spam flash[:notice] = "Comment marked as spam" # redirect_to ... else # ... end end end class Comment < ActiveRecord::Base def mark_as_spam self.spam = true send_spam_notifications if valid? save end end
Our view can make use of a
button_to that POSTs to the
instead of relying on updating via the comments resource. Note how a RESTful
resource does not need to match 1-to-1 with a model, and this creates a much
cleaner interface overall — with a few advantages:
- You clean up your model by creating a
#mark_as_spammethod that is clear and to the point, untangling some of the callback soup that can start to build up on models.
- You are creating a level of abstraction that allows you to code to an
interface, making it trivial to change
#mark_as_spam’s implementation to anything else, like an Akismet call, or a more complex Spam class.
- Splitting it out allows us to properly scope the CommentsController#update’s
finder to the current user (
@comment = current_user.comments.find(params[:id])), allowing any user to mark comments as spam, and only commenters to update their own comments.
- You can easily specify a more meaningful flash message for each case.
The question then arises, when should we split out to a new resource? As always, it depends. For the most part it’s whenever it will help clean up your models and controllers. I find that if the update triggers any logic at all, it’s worth breaking out to its own resource and corresponding model method that encapsulates that logic.