Refactoring a simple method in my controller

Question

I'm having a tough time deciding how to refactor this method in my controller. The idea is that (in this case) it graphs the users that joined (or were created) in the past two weeks.

You might be wondering why I did the @graph_limit thing, and that is because I always want the day that has the most results to be the tallest bar on my bar chart (which in the view are just created with css by making the height of the <div> using css).

Basically I want to dry it up and... ya know just about improve this method as much as possible:

# Controller
def index

  two_weeks_ago = Date.today - 13.days

  @users_graphed = User.count(:conditions=>["created_at >= ?", two_weeks_ago], :order => 'DATE(created_at) DESC', :group => ["DATE(created_at)"])

  two_weeks_ago.upto(Date.today) do |day|
    @graph_limit = 100/@users_graphed.values.max.to_f
    @users_graphed[day.to_s] ||= 0
  end

end

Also I should mention, that you guys are probably going to rip my code to shreds... so I'm bracing for the outcome.

# View
<% @users_graphed.sort.reverse.each do |user| %>
  <li>       
    <% content_tag :div, :style => "height: #{number_with_precision(user[1] * @graph_limit, :precision => 2)}px; ", :class => "stat_bar" do %>
      <%= content_tag(:span, user[1]) unless user[1] == 0 %>
    <% end %>
  </li>
<% end %>

Ultimately and what my real goal here is to put this into my application controller and be able to chart any models by it's create_at times. maybe something like tasks.chart_by(2.weeks). How would you guys get this separated out into something I can use throughout the whole app?

Answer 1

I agree with Joseph that your controller here is doing a lot of work that should be done in the model. Any time you're specifying multiple find parameters in your controller, ask yourself whether or not that should be in your model instead.

You're doing a lot of iterating here that seems needless. Firstly, You shouldn't be calculating @graph_limit inside the loop. You're recalculating it 14 times, but the value is going to be the same every time. Do that outside the loop.

Secondly, that sort.reverse in your view sticks out. You're already sorting in your finder (:order => 'DATE(created_at) DESC'), and then you're sorting again in your view and then reversing it? You should instead be asking the database for the values in the final order you want them. Then to make your zero-filling code work you can just reverse it, doing Date.today.downto(two_weeks_ago) instead of upto.

I would say that you should really be doing this all in SQL, but unfortunately (as perhaps you've discovered) MySQL makes it difficult to fill in missing days without creating a calendar table to join against.

Answer 2

Thanks Jordan, per your ideas (which were really great by the way) I've created a helper that is like such:

def graph_by_time(klass, time_ago)
  time_range_start = Date.today - time_ago

  @elements_graphed = klass.count(:conditions=>["created_at >= ?", time_range_start], :order => 'DATE(created_at) DESC', :group => ["DATE(created_at)"])
  @graph_limit = 100/@elements_graphed.values.max.to_f

  time_range_start.upto(Date.today) do |element|
    @elements_graphed[element.to_s] ||= 0
  end

  return @elements_graphed.sort.reverse
end

The biggest issue here is zero filling the days which have no records associated with them, your method of switching to from upto to downto didnt work and only returned the records which did result in a integer other than zero.