2
\$\begingroup\$

I have a rails4 app. When a user comments on a post it should send a notification to all the guys who commented on the post and to the post creator. I have a working method in the controller, but it's kinda ugly and I'd like to refactor it.

Could somebody tell me what the rails convention for refactoring this?

Comment belongs_to :post, post has_many :comments

create#comment action 

    if @post_comment.save
      ((@post.users + [@post.user]).uniq - [current_user]).each do |post_commenter|
        Notification.create(recipient_id: post_commenter.id, sender_id: current_user.id, notifiable: @post_comment.post, action: "commented")
    end
    ....
end
\$\endgroup\$
1
\$\begingroup\$

First of all I would add a method to Post:

# app/models/post.rb
def notify_others_about_new_comment(comment)
  recipients = [user]               # the author of the original post
  recipients << users               # people that wrote a comment
  recipients.delete(comment.user)   # writer of the new comment

  recipients.uniq.each do |recipient|
    Notification.create(
      recipient_id: recipient.id, 
      sender_id: comment.user.id, 
      notifiable: self, 
      action: 'commented'
    )
  end
end

And just call that method in your controller:

if @post_comment.save
  @post.notify_others_about_new_comment(@post_comment)
  #...
end
\$\endgroup\$
6
  • \$\begingroup\$ spickermann, you mistyped it. you use comment.user instead of current_user. Could you correct it for other who may check this solution? I have 2 questions as well. 1. Is recipients.delete(current_user) better than recipients - [current_user] or it's just a matter of taste? 2. How do you decide on which instance you call the method from post, post_comment and current_user? \$\endgroup\$ Mar 22 '16 at 13:33
  • \$\begingroup\$ I decided to add the method to Post and call it on @post, because it felt to my that belongs into post, because a post is the instant that should (or could) know about all users involved into this conversation. There is no need for a comment or a current_user to know about other commenters. I think you should not pass current_user to that method. A comment, should know who the author was and I guess comment.user equals current_user, doesn't it? I prefer delete over - because it reads better. \$\endgroup\$ Mar 22 '16 at 14:02
  • \$\begingroup\$ So you think I always should use in these kind of cases object.user over current_user? I usually use current_user because it's more readable, but if you know any reason not to do so, then I will change it. \$\endgroup\$ Mar 22 '16 at 18:06
  • \$\begingroup\$ current_user only exists in the context of a controller. Whereas object.user exists in the model. If we want to move this functionality from the controller into the model (following Skinny Controller, Fat Model pattern) than you have to use object.user and cannot use current_user. \$\endgroup\$ Mar 22 '16 at 19:30
  • \$\begingroup\$ might also consider adding after_create :notify_others_about_new_comment and omitting that method call from the controller \$\endgroup\$
    – jtmarmon
    Mar 24 '16 at 20:18
1
\$\begingroup\$

As you tagged your question with "performance" and the notification creation process needs not to be very real-time, I think it's better to move it to the background. For example, you can create one or more processes which focus on creating notifications when they received messages. Rails application send messages to those processes through a message broker like RabbitMQ.

\$\endgroup\$
2
  • \$\begingroup\$ Aetherus, at the moment I'm doing it real-time with pusher. Maybe it's not the best idea, but I wan't users to get the notification right away without page load. \$\endgroup\$ Mar 22 '16 at 18:10
  • 1
    \$\begingroup\$ This feels very slim for an answer, can you expand on how that's an improvement over the existing solution? \$\endgroup\$
    – Quill
    Mar 22 '16 at 22:09
1
\$\begingroup\$

I think there are two possible solutions: Using a service object, or adding this to your Post class and utilizing Active Record callbacks.

Using a service object

As you'll see in a moment, there isn't much in the service object except a way to send these notifications. While there isn't much code, what you'll gain is decoupling code that interacts solely with your own system (your models and database) from code that interacts with outside resources (notifications).

First, to centralize the logic of finding the post users, without the post author, add a method to the Post class:

class Post < ActiveRecord::Base
  def other_users
    users - user
  end
end

Then the service object:

class PostService
  def add_comment(post, author, comment_text)
    new_comment = post.comments.create user: author, text: comment_text
    post.other_users.each { |user| CommentNotification.create user, new_comment }
    new_comment
  end
end

And your controller:

class CommentsController < ApplicationController
  def create
    post = Post.find params[:comment][:post_id]
    author = User.find params[:comment][:user_id]
    @comment = post_service.add_comment author, params[:comment][:text]

    if post.save
      redirect_to post_url(post)
    else
      render action: 'create'
    end
  end

  private

  def post_service
    @post_service ||= PostService.new
  end
end

Using ActiveRecord callbacks

Honestly my initial reaction was to use an after_save callback, but I also read spickermann's comment about shying away from ActiveRecord callbacks. While I've had problems with callbacks before, if you construct your code correctly, the callback shouldn't be a problem. I also don't think the controller should know much of anything about posts, comments or notifications. The controller should handle the basics of the HTTP request, and little more.

Adding an add_comment method to the Post class would be the appropriate place to enforce this logic:

class Post < ActiveRecord::Base
  after_save :send_comment_notifications

  def add_comment(author, text, notify_users = true)
    new_comment = Comment.new(post: self, user: author, text: text)
    comments << new_comment

    other_users.each do |user|
      comment_notifications << CommentNotification.create user, new_comment
    end if notify_users

    return new_comment
  end

  private

  def comment_notifications
    @comment_notifications ||= []
  end

  def other_users
    users - user
  end

  def send_commend_notifications
    comment_notifications.each do |notification|
      notification.send
    end

    @comment_notifications = nil
  end
end

The notify_users argument, true by default, allows you to disable notifications which solves Spickermann's major complaint about spamming notifications if you need to do a data import.


Aside: I took a few liberties with your other classes, so they might not exactly match your object model, but you should get the main idea.


  • The users you want to notify are the users for the post, excluding the creator of the post. I think that is best encapsulated in its own method: other_users

  • Create a class called CommentNotification which inherits from Notification, and place all logic for constructing that notification in there making this portable.

    class CommentNotification < Notification
      NOTIFICATION_ACTION = 'commented'
    
      def self.create(user, comment)
        base.create(
          recipient_id: user.id,
          sender_id: comment.post_user_id,
          notifiable: comment.post,
          action: NOTIFICATION_ACTION)
      end
    end
    
  • The controller becomes extremely slim:

    class PostCommentsController < ApplicationController
      def create
        post = Post.find params[:comment][:post_id]
        author = User.find params[:comment][:user_id]
        @comment = post.add_comment author, params[:comment][:text]
    
        if post.save
          redirect_to post_url(post)
        else
          render action: 'create'
        end
      end
    end
    
\$\endgroup\$
2
  • \$\begingroup\$ Greg, you mentioned service objects, but as far as I see this is the callback solution. Could you show it with service object? I would avoid using callbacks. \$\endgroup\$ May 25 '16 at 20:47
  • \$\begingroup\$ @SzilardMagyar: I added a service object example to my answer as well. \$\endgroup\$ May 26 '16 at 13:35

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.