5
\$\begingroup\$

I have this controller that I call via Ajax, where I update the likes of a post.

I have two methods in the user model:

  • already_like_post?: Where I pass the posted ID and return true or false if the post is already liked.
  • dislike_post: Delete the user like for given post.

class LikesController < ApplicationController
    def create
        if !current_user.already_liked_post? params[:post_id]
            like = Like. new user: current_user, post_id: params[:post_id]
            if !like.save
                render json: like.errors
            end
        else
           current_user.dislike_post params[:post_id]
        end
        amount_of_likes = Post.find(params[:post_id]).likes.count
        render json:{status:"success",likes: amount_of_likes}
    end
end

These are the methods in the user model:

def already_liked_post?(post_id)
    self.likes.where(post_id: post_id).size == 1
end

def dislike_post(post_id)
    self.likes.find_by(post_id: post_id).destroy
end

Can you give a review of this? I don't know if this looks good or if it is the Rails way.

\$\endgroup\$
1
  • \$\begingroup\$ How do you record dislikes, I believe you also have a Dislike model? or keep it under the Like model but having an attribute to determine whether it was "liked" or "disliked"(bloody). \$\endgroup\$
    – olleh
    Aug 20 '15 at 8:25
4
\$\begingroup\$

Some notes:

  • Use 2-space indentation.
  • Move all the logic to the model.
  • DB deletes can also fail.
  • No need for self.
  • If you use object instead of object_id as argument, code is usually more declarative.
  • In case of error, you are performing a double render.

I'd write:

class LikesController < ApplicationController
  def create
    post = Post.find(params[:post_id])

    if current_user.toggle_like(post)
      render(json: {status: "success", likes: post.likes.count})
    else
      render(json: {status: "error"}, status: :unprocessable_entity)
    end
  end
end

class User < ActiveRecord::Model
  # Toggle like for post. Returns boolean with the status of the operation.
  def toggle_like(post)
    likes_for_post = likes.where(post_id: post.id)

    if likes_for_post.exists?
      likes_for_post.destroy_all.all?(&:destroyed?)
    else
      likes.new(post: post).save
    end
  end
end
\$\endgroup\$
5
  • \$\begingroup\$ Thanks, I really liked that I even did the refactoring. Two notes: 1. The method exist?, its named exists?. 2. It's throwed an error when I called the destroy method instead I had to use the destroy_all method. Thanks. \$\endgroup\$
    – Nivla
    Aug 5 '15 at 12:44
  • \$\begingroup\$ You are spot-on on both notes, updated. \$\endgroup\$
    – tokland
    Aug 5 '15 at 15:11
  • 1
    \$\begingroup\$ @Nivla: I've refactored toggle_like to make sure it returns a meaningful boolean. \$\endgroup\$
    – tokland
    Aug 6 '15 at 9:34
  • \$\begingroup\$ Great, what means .all?(&:destroyed?), the current record already destroyed? \$\endgroup\$
    – Nivla
    Aug 6 '15 at 15:04
  • \$\begingroup\$ Yes, it makes sure all the Like objects (usually, just one), have been destroyed. "destroy" may not suceed (it returns false) for example if a before_destroy callbacks stops it. \$\endgroup\$
    – tokland
    Aug 6 '15 at 15:50
2
\$\begingroup\$

Architecturally this looks fine. The database end of might need some tweaking, however.

amount_of_likes = Post.find(params[:post_id]).likes.count

This results in two queries:

SELECT * FROM posts WHERE post_id = ...
SELECT COUNT(*) FROM likes WHERE post_id = ...

This will load the Post, then do a count(*) on the likes relation. Why not get the count by the Like model instead:

amount_of_likes = Like.where(user_id: current_user.id).count

(assuming the Like and User models are related in that way)

This does a SELECT COUNT(*) FROM likes WHERE user_id = ... which would avoid loading the Post object.

The only other thing I would recommend is to return a 422 HTTP response code if the Like failed to save:

    if !like.save
        render json: like.errors, status: 422
    end
else
    ...
end
...
render json: { likes: amount_of_likes }

This would eliminate the need for a "status" flag. Switching the HTTP response code would prevent this route in your application from being used as a JSONP request, but if it's just normal same-domain AJAX or cross-domain AJAX with the proper permissions in place, then the status code would be ideal.

The error handler in JavaScript on the client would handle the validation errors.

\$\endgroup\$
1
  • \$\begingroup\$ According to a deleted answer I converted to a comment on the question: You forgot to add post_id on the where query. def already_liked_post?(post_id) Like.where(user_id: current_user.id, post_id: params[:post_id]).count > 0 end (I personally don't know if this is correct or not) \$\endgroup\$ Aug 20 '15 at 10:22

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.