views:

99

answers:

3

I wanted to make a simple chart of users that have been created in the past month in my app. Like basically for each day in the past month I want to show the count of users that have registered that day. What I have so far:

# Controller
@users = User.count(:order => 'DATE(created_at) DESC', :group => ["DATE(created_at)"])

# View
<% @users.each do |user| %>
  <%= user[0] %><br />
  <%= user[1] %>
<% end %>

# Output
2010-01-10 2 
2010-01-08 11
2010-01-07 23
2010-01-02 4

Which is ok, but if no users were created on a given day, it should say "0" instead of not being there at all. How can I loop through each day in the last 30 days and show the count of users created on that day?

+3  A: 
date = Date.today-30

# Controller
@users = User.count(:conditions=>["created_at >= ?", date], :order => 'DATE(created_at) DESC', :group => ["DATE(created_at)"])
date.upto(Date.today) do |x|
  @users[x.to_s] ||= 0
end
@users.sort!

# View
<% @users.each do |user| %>
  <%= user[0] %><br />
  <%= user[1] %>
<% end %>
Nakul
You want @users.sort!
klochner
and it would be slightly cleaner to use '@users.each do |date,count| . . .'
klochner
Wouldn't they be sorted already? We used `:order` in the finder.
Joseph Silvashy
Not with the block that fills in the missing gaps (@users[x.to_s] ||= 0)
klochner
thanks klochner, fixed it
Nakul
+1  A: 

I think separation trumps the minimal performance gains here:

# Controller
@users = User.all(:conditions => ["created_at >= ?", Date.today.at_beginning_of_month])

# View
Date.today.at_beginning_of_month.upto(Date.today).each do |date|
  <%= date %>: <%= @users.select{|u| u.created_at == date }.size %>
end
floyd
Seems like too much logic in the view to me. I think the manipulation belongs in the controller -- and pre-processing seems like a much better idea than `select`ing 30 times.
Emily
The select is the only additional logic in the view, and it saves the controller 4 lines. My first priority is always skinny controllers. You're definitely right about all the selects, but if that were really a performance issue, I would add a method to User which puts the logic right in SQL. Something like User.new_accounts_per_day. But SQL is hard. But never in the controller.
floyd
At most I'd put the "||=0" part in the view, but certainly keep the count in the controller with SQL. That would give you a single line of code (skinny) in the controller, and efficient counting. Your implementation would be O(@users.size), and probably 100x slower for even smallish data sets.
klochner
You're absolutely right of course. Of course there's no metric for how much clearer my implementation is. I guess I'd rather write the clear, easy solution in 2 mins, come back when a render bottleneck is identified and optimize.
floyd
I like this implementation the most. The reason this solution seems a little more skinny than @Nakul's is because Floyd is counting from the `beginning_of_month` instead of the past month (or like past 30 days), therefore his is at most 2 lines shorter.
Joseph Silvashy
I guess the one things is that while this is much more readable it is slower for the DB, but I think I can optimize it. My biggest concern is that this would create and N+1
Joseph Silvashy
I think `Date.today.last_month` is better than `Date.today-30`
Joseph Silvashy
A: 

As @floyd commented on earlier, the code to do the SELECT belongs in the model:

class User < ActiveRecord::Base
  def self.count_new_users_per_day(cutoff_at)
    result = count(:all, :conditions => ["created_at >= ?", cutoff_at],
                         :group => "DATE(created_at)")
    # See http://ruby-doc.org/core-1.8.7/classes/Hash.html#M000163
    result.default = 0
    result
  end
end

The controller does logic and calls into the model layer:

class UsersController < ActionController::Base
  def index
    @cutoff_at = 30.days.ago.at_midnight
    @new_users_by_date = User.count_new_users_per_day(@cutoff_at)
    @dates = ((@cutoff_at.to_date) .. (@cutoff_at.to_date >> 1))
  end
end

And the view is only responsible for displaying the data the controller's setup for it:

# Chose to move the code to a partial
<%= render :partial => "user_count", :collection => @dates, :as => :date %>

# _user_count.html.erb
<td><%=h date.to_s(:db) %></td>
<td><%= number_with_delimiter(@new_users_by_date[date.to_s(:db)]) %></td>

Basically, since SQL won't return missing dates, you have to iterate over the full set of dates yourself, and ask the Hash/ResultSet if it has the correct value. In the model implementation above, I set the Hash's default value after the fact, giving me a clean way to get a zero when a value is missing.

François Beausoleil