Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

System I'm building has Users and Articles. Users can upvote/downvote Articles and cancel their votes if they want to. Each vote changes article's score. Score equals to sum of all vote values together.

So I created three models: User, Article, ArticleVote. Even though the most natural way for me would be to be able to do user.upvote(article), for separation of concerns reasons I've put it in ArticleVote model:

class ArticleVote < ActiveRecord::Base
  belongs_to :article
  belongs_to :user

  before_destroy :before_destroy

  validates :value, inclusion: {in: [1, -1]}

  def self.upvote(user, article)
    cast_vote(user, article, 1)
  end

  def self.downvote(user, article)
    cast_vote(user, article, -1)
  end

private

  def self.cast_vote(user, article, value)
    vote = ArticleVote.where(user_id: user.id, article_id: article.id).first_or_initialize
    vote.value = value
    vote.save!
    article.score += value
    article.save!
  end

  def before_destroy
    article.score -= value
    article.save
  end
end

So now I call upvote via ArticleVote.upvote(user, article) which doesn't look too bad, but because of Rails' behavior, this doesn't work very good.

If I want to cancel an upvote for example:

article = Article.find(528953)
# article.score = 5 at this point (for example)
vote = ArticleVote.where(user: current_user, article: article).first.destroy
# article.score = 5 !!!!
article.reload
# article.score = 4

So even though I already fetched an Article from database, and used instance of it to find ArticleVote I have to fetch it again to get the actual score value. I would expect ActiveRecord to automatically set vote.article to reference my article, so when I modify it's state, I wouldn't have to fetch it once again.

Is there a way I can avoid this unnecessary article.reload call and still have valid article.score value?

share|improve this question
    
Could you just tell which rails version are you using? –  BroiSatse Mar 30 at 23:36
    
Rails 4, not sure about exact version number, typing from phone –  Igor Pantović Mar 30 at 23:38

2 Answers 2

up vote 2 down vote accepted

I would start with creating two subclasses of your votes, using rails STI (you need to add one string 'type' column to your model - You can get rid of value though) and use rails counter_cache feature for both subclasses.

class ArticleVote < ActiveRecord::Base

  def initialize(*args)
    raise 'This class cannot be initialized' if self.class == ArticleVote
    super
  end

  validate :user, presence: true

end

class UpVote < ArticleVote
  belong_to :article, counter_cache: true
end

class DownVote < ArticleVote
  belongs_to: article, counter_cache: true
end

With this in place, I would create three associations on your Article model, and define score method:

class Article < ActiveRecord::Base
  has_many :votes, class_name: 'ArticleVotes'
  has_many :up_votes
  has_many: down_votes

  def score
    (up_votes_count || 0) - (down_votes_count || 0)  
  end
end

You can create new votes using article.up_votes.create(user_id: user.id) (similar for down_votes).

Counter cache will keep track of a number of down and up votes (including destroying existing votes) and this is extremely cheap to subtract those values. Note that you need two extra columns in you article model (up_votes_count and down_votes_count, no score column needed).

Now the only thing left is to keep track of user changing their votes. To do this, I would add before_create filter on Vote model:

class ArticleVote < ActiveRecord::Base
  belongs_to :article

  def initialize(*args)
    raise 'This class cannot be initialized' if self.class == ArticleVote
    super
  end

  validate :user, presence: true

  before_create(prepend: true) do
    existing_record = article.article_votes.find_by(user_id: user_id)
    return false if existing_record && existing_record.class == self.class
    existing_record && existing_record.destroy
    true
  end
end

This filter will check whether given user already voted on given article. If he did and he is trying to vote again, do nothing (return false) otherwise remove previous vote and create new one.

There is a potential problem here with condition race. The only way to ensure record uniqueness is to create database constraint like unique index.

Note that, while testing it in the console, counters columns behaves as normal column - it is stored in memory and will not checked whether the value changed in a database or not. Hence you need to reload the model to see the changes:

article.score #=> 0
article.up_vote.create(user_id: User.first.id)
article.score #=> 0         # Counters not updated
article.reload.score #=> 1  # But column in a database has been updated
share|improve this answer
    
I went with this in the end and it worksgreat :) –  Igor Pantović Apr 21 at 19:12

Rails just isn't taking chances, so when you call vote.article it's performing a new DB query, and thus creating a new instance rather than grabbing a cache. And that's a good thing! You might run into trouble otherwise, since you call save. If it was the same Article instance everywhere, you'd suddenly be calling save on it from a non-obvious point in the code.

I.e. pretend that Rails does reuse the same Article instance, and that instance has other unsaved changes. Perhaps these changes aren't valid, and in that case, the save call in ArticleVote would fail although you're only trying to change the article's score. It would also update the article's updated_at timestamp, although that may not be relevant; an article receiving or losing a vote doesn't really mean that the article itself was updated.

So, for one I'd recommend using article.update_column(:score, article.score - value) in you before_destroy method. And, yes, you'll still need to call reload elsewhere.

However, the underlying issue is that you're modifying an article "from the outside". I'd recommend a different approach altogether, with no articles.score column in the database, and using Rails' caching API instead:

class Article
  has_many :votes, class_name: "ArticleVotes"

  # score is not part of the DB schema;
  # it's computed and cached as necessary
  def score
    # get or update the cached score
    Rails.cache.fetch(score_cache_key) { votes.sum(:value) }
  end

  def flush_score_cache
    Rails.cache.delete(score_cache_key)
  end

  # let's not use or override the built-in
  # #cache_key method for this, since that
  # relies on the updated_at timestamp.
  def score_cache_key
    "article/#{id}/score" # or something similar with the ID in it
  end
end

(In the above, you may want to handle the possibility of the record being new, and thus having a nil id.)

And in ArticleVote:

class ArticleVote
  after_commit :flush_article_score

private

  def flush_article_score
    article.flush_score_cache
  end
end

Now whenever a vote is added, removed or updated, the article's score cache will be busted, forcing a article.score to do a proper recount if/when it's called.

This also has the advantage that the article's score remains consistent with the actual votes: It's computed entirely from what's in the database. Alternatively, you can simply increment/decrement the cached value instead of flushing it, and still avoid having a database column.

Of course, there are some drawbacks too. Since it is a cache the usual disclaimers apply (you have to be diligent about busting it when necessary, or you'll hit a stale cache, etc..). And if you've got heavy voting traffic you might end up busting the cache so often it just always does the SUM() query. In that case, you could still go with your current solution of having a database column that you increment and decrement, but I might still want to stick that in a different table to avoid the aforementioned issues with implicitly calling save on an article (or, like now, overwriting an updated score with a older one if you forget to reload). You could also consider a more "raw SQL" approach of actually incrementing or decrementing rather than writing the value directly, i.e. UPDATE ... SET score=score + 1 WHERE .... That would avoid race conditions. Optimally, you might do a bit of both: A database column holding the canonical score of an article on disk, and a memory cache you use most of the time for reads, so you only have to hit the database for writes.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.