LikesControler for a Post

Question

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.

Answer 1

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

Answer 2

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.