Problem
How can I make my model DRY and KISS?
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
Solution
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";