How can I make this code DRY and KISS in my model?

Question

class Rating < ActiveRecord::Base
  attr_accessible :item_type, :item_id, :rating, :voters_up, :voters_down
  serialize :voters_up, Hash
  serialize :voters_down, Hash
  belongs_to :ranks, :polymorphic => true

  def self.up(item, user, iteration = 1)
    @rating = Rating.where(item_type: item.class.name, item_id: item.id).first
    @rating = Rating.create(item_type: item.class.name, 
                            item_id: item.id, 
                            rating: 0, 
                            voters_up: {users:[]}, voters_down: {users:[]}) unless @rating

    changed = nil
    if !@rating.voters_up[:users].include?(user.id) && !@rating.voters_down[:users].include?(user.id)
      if changed.nil?
        @rating.increment(:rating, 1)
        @rating.voters_up[:users] << user.id
        changed = true
      end
    end

    if @rating.voters_up[:users].include?(user.id) && !@rating.voters_down[:users].include?(user.id)
      if changed.nil?
        @rating.decrement(:rating, 1)
        @rating.voters_up[:users].delete user.id
        changed = true
      end
    end

    if @rating.voters_down[:users].include?(user.id) && !@rating.voters_up[:users].include?(user.id)
      if changed.nil?
        @rating.voters_up[:users] << user.id
        @rating.voters_down[:users].delete user.id
        @rating.increment(:rating, 2)
        changed = true
      end
    end

    @rating.save
    item.update_attribute(:rating_value, @rating.rating)
  end

  def self.down(item, user, iteration = 1)
    @rating = Rating.where(item_type: item.class.name, item_id: item.id).first
    @rating = Rating.create(item_type: item.class.name, 
                            item_id: item.id, 
                            rating: 0, 
                            voters_up: {users:[]}, voters_down: {users:[]}) unless @rating

    changed = nil
    if !@rating.voters_down[:users].include?(user.id) && !@rating.voters_up[:users].include?(user.id)
      if changed.nil?
        @rating.decrement(:rating, 1)
        @rating.voters_down[:users] << user.id
        changed = true
      end
    end

    if @rating.voters_down[:users].include?(user.id) && !@rating.voters_up[:users].include?(user.id)
      if changed.nil?
        @rating.increment(:rating, 1)
        @rating.voters_down[:users].delete user.id
        changed = true
      end
    end

    if @rating.voters_up[:users].include?(user.id) && !@rating.voters_down[:users].include?(user.id)
      if changed.nil?
        @rating.voters_down[:users] << user.id
        @rating.voters_up[:users].delete user.id
        @rating.decrement(:rating, 2)
        changed = true
      end
    end

    @rating.save
    item.update_attribute(:rating_value, @rating.rating)
  end

end

Answer 1

Here's a 2-part answer: First, a direct answer to your question, and then an alternative approach to the whole thing.

DRYing the current implementation
Firstly, I'd make sure that every "item" has_one :rating. Right now, you're doing a lot of manual record creation, and Rating has to leave all of its attributes accessible to mass-assignment. It'd also be nicer to simply say question.upvote or answer.upvote directly, instead of "manually" calling the Rating.up method and having to supply both user and item.

That can be done, provided you can store the current user somewhere that's accessible to the models. But let's keep it a little simpler and just go for a syntax like question.upvote(user)

To DRY out the "vote-enabled" models, try this:

# in app/concerns/votable.rb
require 'active_support/concern'

module Votable
  extend ActiveSupport::Concern

  included do
    has_one :rating, :as => :item
  end

  module ClassMethods
    def upvote(user)
      # Build the Rating record if it's missing
      build_rating if rating.nil?
      # Pass the user on
      rating.upvote user
    end

    def downvote(user)
      build_rating if rating.nil?
      rating.downvote user
    end
  end
end

Then, in a record that users can vote on, include that concern:

class Question < ActiveRecord::Base
  include Votable
  # ...
end

class Answer < ActiveRecord::Base
  include Votable
  # ...
end

Now the Question and Answer records will automatically build their own Rating record, if they don't have one already, and then pass the upvote/downvote calls on to that rating record.

As for Rating itself, here's what I'd do (I've changed the attribute names a little, but you can of course keep using yours):

class Rating < ActiveRecord::Base
  belongs_to :item, :polymorphic => true
  # Serialize stuff as straight-up Array
  serialize :upvoters, Array
  serialize :downvoters, Array
  # Create arrays after record initialization, so they're not nil on new records
  after_initialize :initialize_voters
  # Update the "owner's" rating cache after every save
  after_save :update_item

  # Note: No attr_accessible at all; it's not needed.

  # I'll skip the comments, as this should all be very easily readable.

  def upvote(user)
    if upvoted_by?(user)
      decrement :rating
      remove_upvoter(user)
    elsif downvoted_by?(user)
      increment :rating, 2
      add_upvoter user
      remove_downvoter user
    else
      increment :rating
      add_upvoter user
    end
    save
  end

  def downvote(user)
    if downvoted_by?(user)
      increment :rating
      remove_downvoter user
    elsif upvoted_by?(user)
      decrement :rating, 2
      remove_upvoter user
      add_downvoter user
    else
      decrement :rating
      add_downvoter user
    end
    save
  end

  def upvoted_by?(user)
    upvoters.include? user.id
  end

  def downvoted_by?(user)
    downvoters.include? user.id
  end

  private

    def add_upvoter(user)
      upvoters << user.id
    end

    def add_downvoter(user)
      downvoters << user.id
    end

    def remove_upvoter(user)
      upvoters.delete user.id
    end

    def remove_downvoter(user)
      downvoters.delete user.id
    end

    def initialize_voters
      upvoters ||= []
      downvoters ||= []
    end

    def update_item
      if item.present?
        item.update_attribute :rating_value, rating
      end
    end

end

And that's pretty much it. Now you can do Question.find(23).upvote(current_user) or whatever, and it should work out.

---

Going straight to the database
Alternatively, I'd suggest using "raw" database queries instead of models with serialized attributes and all that. All you really need is a simple table. It doesn't need an id column or a model-layer abstraction. All it needs to is have 4 columns: user_id, vote, item_type, and item_id. You can skip ActiveRecord models and such, since all you need to do is check for a certain user/item combo.

Make user_id, item_id and item_type the primary key (or add a uniqueness constraint) and you can do the entire logic in SQL (at least MySQL). If, for instance, user 23 votes on answer 42, it'd look like this:

INSERT INTO `votes` (`user_id`, `item_id`, `item_type`, `vote`)
VALUES (23, 42, "Answer", X)
ON DUPLICATE KEY UPDATE `vote`=IF(`vote`=X, 0, X);

where X is 1 for an upvote, or -1 for a downvote. That's actually all the upvote/downvote logic in one query.

Right after that, update the total rating for the post:

UPDATE `answers`
SET `rating_value`=(
  SELECT IFNULL(SUM(`vote`), 0)
  FROM `votes`
  WHERE `item_id`=42 AND `item_type`= "Answer"
)
WHERE id=42;

From time to time, you may want to do some clean-up to keep the table a little leaner (i.e. delete rows where the vote is zero, or when an associated item is deleted), but otherwise you should have a pretty robust solution with none of the overhead of Rails and ActiveRecord. And it can all be abstracted into a mixin like the concern above, and you can easily add the upvoted_by?(user)/downvoted_by?(user) methods there too with some simple SQL:

SELECT * FROM `votes` WHERE `vote`<>0 AND `user_id`=23 AND `item_id`=42 AND `item_type`="Answer";