Рефакторинг простой метод в моем контроллере

Question

Мне трудно решить, как изменить этот метод в моем контроллере. Идея заключается в том, что (в данном случае) он отображает пользователей, которые присоединились (или были созданы) в течение последних двух недель.

Вам может быть интересно, почему я сделал @graph_limit, и это потому, что я всегда хочу, чтобы день с наибольшим количеством результатов был самым высоким столбцом на моей гистограмме (которые в представлении просто создаются с помощью css by делая высоту <div> используя css).

В основном я хочу высушить его и ... вы знаете, как можно лучше улучшить этот метод:

# 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

Также я должен отметить, что вы, ребята, вероятно, собираетесь разорвать мой код в клочья ... так что я готов к результату.

# 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 %>

В конечном счете, и моя настоящая цель заключается в том, чтобы поместить это в мой контроллер приложений и иметь возможность наметить любые модели в разы create_at. может быть что-то вроде tasks.chart_by(2.weeks). Как бы вы, ребята, разбили это на что-то, что я могу использовать во всем приложении?

Answer 1

Я согласен с Джозефом, что ваш контроллер выполняет большую работу, которую следует выполнить в модели. Каждый раз, когда вы указываете несколько find параметров в вашем контроллере, спросите себя, должно ли это быть в вашей модели.

Вы делаете много итераций здесь, которые кажутся ненужными. Во-первых, вы не должны вычислять @graph_limit внутри цикла. Вы пересчитываете это 14 раз, но значение будет одинаковым каждый раз. Сделайте это вне цикла.

Во-вторых, что sort.reverse по вашему мнению торчит. Вы уже сортируете в своем искателе (:order => 'DATE(created_at) DESC'), а затем вы снова сортируете в своем представлении и затем переворачиваете его? Вместо этого вам следует запросить в базе данных значения в том порядке, в котором вы их хотите. Затем, чтобы ваш код с нулевым заполнением работал, вы можете просто изменить его, выполнив Date.today.downto(two_weeks_ago) вместо upto.

Я бы сказал, что вы действительно должны делать все это в SQL, но, к сожалению (как вы, возможно, обнаружили), MySQL затрудняет заполнение пропущенных дней без создания таблицы календаря для объединения.

Answer 2

Спасибо, Джордан, за ваши идеи (которые, кстати, были действительно замечательными) я создал помощника, подобного таковому:

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

Самая большая проблема здесь - ноль, заполняющая дни, с которыми не связано ни одной записи, ваш метод перехода с upto на downto не сработал и возвращал только записи, которые приводили к целому числу, отличному от нуля.