Ruby on Rails - Monthly top vote getter

Question

I need some advice on a voting system in rails that recognizes the top vote getter on a monthly basis. I have a system that works but being new to rails, I'm sure there are more efficient methods available. Below is a simplified version of my current setup(controller code omitted):

class Charity < ActiveRecord::Base
  has_many :votes
end

class Vote < ActiveRecord::Base
  belongs_to :charity
end

My schema is as follows:

ActiveRecord::Schema.define(:version => 20130310015627) do
  create_table "charities", :force => true do |t|
    t.string   "name"
    t.text     "description"
    t.date     "last_win"
    t.datetime "created_at",  :null => false
    t.datetime "updated_at",  :null => false
  end 
  create_table "votes", :force => true do |t|
    t.integer  "charity_id"
    t.datetime "created_at", :null => false
    t.datetime "updated_at", :null => false
  end
end

I'll be using the 'whenever' gem to run a cron job to determine the monthly winner and update the 'last_win' column of the charities table. The following code is where I'm questioning my efficiency:

vote_counts = Vote.count(:group => "charity_id")
most_votes = vote_counts.values.max
winning_ids = vote_counts.map{|k,v| v == most_votes ? k :nil }.compact
charities = Charity.find(winning_ids)
charities.each {|charity| charity.update_attributes(:last_win => Date.today)}

I'm sure there are many ways to do this better and would appreciate some suggestions. If you have suggestions on better ways to set up the votes table / associations, that would be appreciated too.

Thanks in advance, CRS

Answer 1

Something like this:

If there was only one winner, this would work I think

winner_id = Vote.group(:charity_id).order("count(*) desc").pluck(:charity_id).first
Charity.find(winner)id).update_attribute!(:last_win => Date.today)

You could modify it for ties:

most_votes = Vote.group(:charity_id).order("count(*) desc").count.first[1]
winners = Vote.group(:charity_id).having("count(*) = ?", most_votes).pluck(:charity_id)

Charity.where(:id => winners).update_all(:last_win => Date.today)

Make sure everything is indexed correctly in your database,

You can probably streamline it more, but the SQL is going to get more complicated.

Answer 2

The last two lines could be:

Charity.where(id:winning_ids).update_all(last_win:Date.today)

Which would translate into a single SQL update command, instead of issuing an update command for each winning charity.

The first part where you identify the winning charities looks okay, and since you're running it as a cron job you probably don't care if it takes a few minutes.

However, if you'd like to show the values in real time, you could add an after_create hook on Vote to update a counter for its owner charity (possibly in another table):

class Vote < ActiveRecord::Base
  belongs_to :charity
  after_create :increment_vote_count
  CharityVote.where(year:Time.now.year, month:Time.now.month, 
    charity_id:self.charity_id).first_or_create.increment!(:counter)
end