Trying to master Ruby. How can I optimize this method?

I'm learning new tricks all the time and I'm always on the lookout for better ideas.

I have this rather ugly method. How would you clean it up?

def self.likesit(user_id, params)

  game_id = params[:game_id]
  videolink_id = params[:videolink_id]
  like_type = params[:like_type]

  return false if like_type.nil?

  if like_type == "videolink"
    liked = Like.where(:user_id => user_id, :likeable_id => videolink_id, :likeable_type => "Videolink").first unless videolink_id.nil?
  elsif like_type == "game"
    liked = Like.where(:user_id => user_id, :likeable_id => game_id, :likeable_type => "Game").first unless game_id.nil?
  end

  if liked.present?
    liked.amount = 1
    liked.save
    return true
  else  # not voted on before...create Like record
    if like_type == "videolink"
      Like.create(:user_id => user_id, :likeable_id => videolink_id, :likeable_type => "Videolink", :amount => 1) 
    elsif like_type == "gam开发者_如何学Pythone"
      Like.create(:user_id => user_id, :likeable_id => game_id, :likeable_type => "Game", :amount => 1) 
    end
    return true
  end

  return false

end

I would do something like:

class User < ActiveRecord::Base  
  has_many :likes, :dependent => :destroy  

  def likes_the(obj)
    like = likes.find_or_initialize_by_likeable_type_and_likeable_id(obj.class.name, obj.id)
    like.amount += 1
    like.save
  end
end

User.first.likes_the(VideoLink.first)

First, I think its wrong to deal with the "params" hash on the model level. To me its a red flag when you pass the entire params hash to a model. Thats in the scope of your controllers, your models should have no knowledge of the structure of your params hash, imo.

Second, I think its always cleaner to use objects when possible instead of class methods. What you are doing deals with an object, no reason to perform this on the class level. And finding the objects should be trivial in your controllers. After all this is the purpose of the controllers. To glue everything together.

Finally, eliminate all of the "return false" and "return true" madness. The save method takes care of that. The last "return false" in your method will never be called, because the if else clause above prevents it. In my opinion you should rarely be calling "return" in ruby, since ruby always returns the last evaluated line. In only use return if its at the very top of the method to handle an exception.

Hope this helps.

I'm not sure what the rest of your code looks like but you might consider this as a replacement:

def self.likesit(user_id, params)
  return false unless params[:like_type]
  query = {:user_id => user_id,
           :likeable_id => eval("params[:#{params[:like_type]}_id]"), 
           :likeable_type => params[:like_type].capitalize}
  if (liked = Like.where(query).first).present?
    liked.amount = 1
    liked.save
  else  # not voted on before...create Like record
    Like.create(query.merge({:amount => 1})) 
  end
end

I assume liked.save and Like.create return true if they are succesful, otherwise nil is returned. And what about the unless game_id.nil? ? Do you really need that? If it's nil, it's nil and saved as nil. But you might as well check in your data model for nil's. (validations or something)