После утомительного утомления через взгляды и контроллеры мы наконец-то достигли той части рефакторинга, которая становится приключением: хвататься за соломинку. Второй угадал все. Навязчиво запускать тесты. Воспитание тонкой надежды на то, что наше вновь обретенное понимание — это действительно понимание, а не просто еще одно несостоятельное предположение.
В предыдущей статье мы анализировали первую строку метода compute , тщательно и тщательно отслеживая ее. Мы обнаружили, что графики связаны с датами, скорее всего, на какой-то еженедельной основе за прошедший год.
Далее мы распутаем код, который обрабатывает необработанные данные в правильном формате для использования диаграммой.
Давным-давно
module Stats class RunningTimeData include Rails.application.routes.url_helpers attr_reader :title, :dates, :key, :today, :y_legend, :y2_legend, :x_legend, :values, :links, :values_2, :x_labels, :y_max def initialize(title, timestamps, key, now) @title = title @dates = timestamps.map(&:to_date) @key = key @today = now.to_date end def compute @actions_running_per_week_array = convert_to_weeks_from_today_array # cut off chart at 52 weeks = one year @count = [52, total_weeks_of_data].min # convert to new array to hold max @cut_off elems + 1 for sum of actions after @cut_off @actions_running_time_array = cut_off_array_with_sum(@actions_running_per_week_array, @count) @max_actions = @actions_running_time_array.max # get percentage done cumulative @cumulative_percent_done = convert_to_cumulative_array(@actions_running_time_array, dates.count ) @url_labels = Array.new(@count){ |i| url_for(:controller => 'stats', :action => 'show_selected_actions_from_chart', :index => i, :id=> @key, :only_path => true) } @url_labels[@count]=url_for(:controller => 'stats', :action => 'show_selected_actions_from_chart', :index => @count, :id=> "#{@key}_end", :only_path => true) @time_labels = Array.new(@count){ |i| "#{i}-#{i+1}" } @time_labels[0] = "< 1" @time_labels[@count] = "> #{@count}" # Normalize all the variable names @y_legend = I18n.t('stats.running_time_legend.actions') @y2_legend = I18n.t('stats.running_time_legend.percentage') @x_legend = I18n.t('stats.running_time_legend.weeks') @values = @actions_running_time_array.join(",") @links = @url_labels.join(",") @values_2 = @cumulative_percent_done.join(",") @x_labels = @time_labels.join(",") # add one to @max for people who have no actions completed yet. # OpenFlashChart cannot handle y_max=0 @y_max = 1+@max_actions+@max_actions/10 end def total_weeks_of_data weeks_since(dates.last) end def weeks_since(date) days_since(date) / 7 end def days_since(date) (today - date).to_i end def convert_to_weeks_from_today_array convert_to_array(dates, total_weeks_of_data+1) {|date| [weeks_since(date)] } end # uses the supplied block to determine array of indexes in hash # the block should return an array of indexes each is added to the hash and summed def convert_to_array(records, upper_bound) a = Array.new(upper_bound, 0) records.each { |r| (yield r).each { |i| a[i] += 1 if a[i] } } a end # returns a new array containing all elems of array up to cut_off and # adds the sum of the rest of array to the last elem def cut_off_array_with_sum(array, cut_off) # +1 to hold sum of rest a = Array.new(cut_off+1){|i| array[i]||0} # add rest of array to last elem a[cut_off] += array.inject(:+) - a.inject(:+) return a end def convert_to_cumulative_array(array, max) # calculate fractions a = Array.new(array.size){|i| array[i]*100.0/max} # make cumulative 1.upto(array.size-1){ |i| a[i] += a[i-1] } return a end end end
Итак, история начинается с переменной @actions_running_per_week_array .
Выполнение действий — это то, что мы видели раньше, и его можно перевести в незаконченные TODO .
Дело в том, что нам все равно, что они TODO. Мы извлекли метки времени made_at из TODO, привели их к датам, и это все, что интересует график. Простые точки данных.
Также кажется ненужным кодировать имя типа в имя.
def datapoints_per_week convert_to_weeks_from_today_array(dates, total_weeks_of_data+1) end
convert_to_weeks_from_today_array принимает два аргумента, оба из которых доступны во всем классе. Нет необходимости проходить мимо них. Снятие их оставляет нас с ложной абстракцией:
def datapoints_per_week convert_to_weeks_from_today_array end
Использование метода приводит к смутному ощущению дежавю.
def datapoints_per_week convert_to_array(dates, total_weeks_of_data + 1) {|date| [weeks_since(date)] } end
Все тело метода — это вызов другого метода, который принимает аргументы, доступные всему классу.
Вставьте это!
Сделайте шаг назад и подумайте о методах, которые мы только что описали. Это началось так:
def datapoints_per_week convert_to_weeks_from_today_array end def convert_to_weeks_from_today_array convert_to_array end def convert_to_array # several lines of complicated logic end
Если определение метода содержит одну строку, и эта строка является просто очередным вызовом метода, то лучше, если эти два имени метода того стоят.
Сравните вышеприведенное со следующим методом, который мы определили в ходе предыдущего рефакторинга .
def total_weeks_of_data weeks_since(dates.last) end
В последнем примере оба имени имеют смысл. Они на разных уровнях абстракции, и вместе они рассказывают хорошую историю.
Обнаружение точек данных
Давайте посмотрим на datapoints_per_week с нашим тестом test_stuff , чтобы выяснить, как структурированы данные.
def test_stuff now = Time.utc(2014, 1, 2, 3, 4, 5) timestamps = [ now - 5.minutes, now - 1.day, now - 7.days, now - 8.days, now - 8.days, now - 30.days, ] stats = Stats::RunningTimeData.new("title", timestamps, "key", now) assert_equal "something", stats.datapoints_per_week end
Распространение данных здесь идет от нескольких минут назад до месяца назад. Мне особенно интересно выяснить, что происходит за неделю без каких-либо данных.
Ошибка выглядит следующим образом:
Expected: "something" Actual: [2, 3, 0, 1]
Индекс массива представляет количество недель назад, а значение — количество точек данных, которые произошли на этой неделе.
Вот как создается массив точек данных:
def datapoints_per_week a = Array.new(total_weeks_of_data+1, 0) dates.each {|date| [weeks_since(date)].each {|i| a[i] += 1 if a[i] } } a end
Защитите свои глаза, дети, у нас есть вложенная итерация!
Внутренний цикл перебирает массив из одного элемента. Это кажется немного бессмысленным. Все, что нам нужно, это индекс:
dates.each {|date| i = weeks_since(date) a[i] += 1 if a[i] }
Нет никакого способа, которым weeks_since когда-либо будет возвращать nil или false , поэтому мы можем удалить постфикс if . Пока мы на этом, давайте переименуем frequencies .
def datapoints_per_week frequencies = Array.new(total_weeks_of_data + 1, 0) dates.each {|date| frequencies[weeks_since(date)] += 1 } frequencies end
Точки данных были сгруппированы по неделям. Далее похоже, что мы собираемся его усечь.
# cut off chart at 52 weeks = one year @count = [52, total_weeks_of_data].min
Комментарий предполагает, что мы хотим показывать данные только к определенной дате. Это имеет смысл, но тогда вы можете задаться вопросом, почему SQL-запрос не просто ограничивает данные на основе определенной даты отсечения.
Возможно, это еще не все.
Если мы посмотрим, как используется @count , это станет еще более запутанным.
# convert to new array to hold max @cut_off elems + 1 # for sum of actions after @cut_off @actions_running_time_array = cut_off_array_with_sum(datapoints_per_week, @count)
Предполагая, что @cut_off фактически означает @count , этот комментарий противоречит предыдущему.
Либо он обрезает данные за год, либо суммирует даты за пределами отсечения в виде одной записи. Я не понимаю, как это может быть и то и другое.
Нам нужно ткнуть в это, чтобы увидеть, что на самом деле происходит.
def test_stuff chart = Stats::RunningTimeData.new("title", [], "key", Time.now) per_week = [1, 0, 3, 4, 5] assert_equal [], chart.cut_off_array_with_sum(per_week, 1) assert_equal [], chart.cut_off_array_with_sum(per_week, 2) assert_equal [], chart.cut_off_array_with_sum(per_week, 3) assert_equal [], chart.cut_off_array_with_sum(per_week, 4) assert_equal [], chart.cut_off_array_with_sum(per_week, 5) assert_equal [], chart.cut_off_array_with_sum(per_week, 6) end
По одному неудачи заполняют пробелы.
# datapoints_per_week = [1, 0, 3, 4, 5] [1, 12] # cut off at 1 [1, 0, 12] # cut off at 2 [1, 0, 3, 9] # cut off at 3 [1, 0, 3, 4, 5] # cut off at 4 [1, 0, 3, 4, 5, 0] # cut off at 5 [1, 0, 3, 4, 5, 0, 0] # cut off at 6
Метод cutoff не отбрасывает данные, он объединяет все переполнения в один слот. Кроме того, он также заполняет любые пустые слоты в течение требуемого периода времени с 0.
@count похоже на слишком общее имя. На самом деле это говорит нам о том, сколько недель данных будет отображаться на графике, независимо от того, сколько недель данных доступно в исходных данных.
Переименование count в total_weeks_in_chart и actions_running_time_array в actions_running_time_array рассказывает несколько более actions_running_time_array историю. Это многословно, но пока мы не разобрались со всеми концепциями, будет сложно выбрать лучшее имя.
def datapoints_per_week_in_chart cut_off_array_with_sum(datapoints_per_week, total_weeks_in_chart) end
Обратите внимание на знакомый шаблон: аргументы доступны глобально, а определение метода состоит из одного вызова другого метода.
Вставка cut_off_array_with_sum дает нам:
def datapoints_per_week_in_chart a = Array.new(total_weeks_in_chart + 1) { |i| datapoints_per_week[i]||0 } a[total_weeks_in_chart] += datapoints_per_week.inject(:+) - a.inject(:+) a end
Это выглядит немного страшно, но может быть несколько упрощено:
def datapoints_per_week_in_chart frequencies = Array.new(total_weeks_in_chart) {|i| datapoints_per_week[i].to_i } frequencies << datapoints_per_week.inject(:+) - frequencies.inject(:+) end
Двигаясь дальше, код вводит совершенно новый термин: cumulative_percent_done .
Я не совсем уверен, что сделано это слово, которое мы ищем. Мы имеем дело с незаконченными TODO, а не с законченными TODO. И даже так, нам все равно .
Давайте переименуем его в cumulative_percentages :
def cumulative_percentages convert_to_cumulative_array(datapoints_per_week, dates.count) end
Как обычно, мы можем встроить содержащийся метод:
def cumulative_percentages a = Array.new(datapoints_per_week_in_chart.size) {|i| datapoints_per_week_in_chart[i]*100.0/dates.count } 1.upto(timestamp_counts.size-1) {|i| a[i] += a[i-1]} a end
Это выглядит более чем пугающе. Это много делает.
Было бы полезно назвать понятия, поэтому давайте выделим некоторые методы.
Первая часть создает массив процентов по неделям.
def percentages_per_week Array.new(datapoints_per_week_in_chart.size) {|i| datapoints_per_week_in_chart[i]*100.0/dates.count } end
Подумай о том, что это делает.
- Он создает новый массив такой же длины, что и существующий массив, и
- Каждое значение в массиве выводится из соответствующего значения в исходном массиве.
Другое слово для этого — карта .
def percentages_per_week datapoints_per_week_in_chart.map {|count| count * 100.0 / dates.count } end
Этот метод тоже делает совсем немного. Именование блока значительно очищает его.
def percentages_per_week datapoints_per_week_in_chart.map(&percentage) end def percentage Proc.new {|count| count * 100.0 / dates.count} end
Полностью переработанная логика cumulative_percentages выглядит следующим образом:
def cumulative_percentages running_total = 0 percentages_per_week.map {|percentage| running_total += percentage } end
Это не так страшно, как казалось всего минуту назад.
уборка
Остальная часть compute может быть взорвана с помощью простых извлечений. Полученный код можно увидеть ниже.
Это даже дольше, чем оригинал, но концепции, изначально скрытые за загадочными именами на нечетных уровнях абстракции, теперь вышли на поверхность. Имена правильные? Возможно нет. Это был не рефакторинг, чтобы создать хорошее расположение кода, это был рефакторинг, чтобы узнать, что там есть.
Встроенный, встроенный, экстракт, встроенный, экстракт, экстракт.
Это ритм рефакторинга, когда кодовая база еще не нашла правильные абстракции.
Результатом этого рефакторинга является понимание, а не хороший код.
Для этого мы собираемся сделать один последний проход в этом. Мы зададим вопрос, если бы это были две вещи, какими бы они были? и из пепла, если этот рефакторинг, появится крошечная, связная абстракция.
module Stats class RunningTimeData include Rails.application.routes.url_helpers attr_reader :title, :dates, :key, :today def initialize(title, timestamps, key, now) @title = title @dates = timestamps.map(&:to_date) @key = key @today = now.to_date end def compute # FIXME: delete call from controllers end def y_legend I18n.t('stats.running_time_legend.actions') end def y2_legend I18n.t('stats.running_time_legend.percentage') end def x_legend I18n.t('stats.running_time_legend.weeks') end def values datapoints_per_week_in_chart.join(",") end def links url_labels.join(",") end def values_2 cumulative_percentages.join(",") end def x_labels time_labels.join(",") end def y_max # add one to @max for people who have no actions completed yet. # OpenFlashChart cannot handle y_max=0 1 + datapoints_per_week_in_chart.max + datapoints_per_week_in_chart.max/10 end private def url_labels urls = Array.new(total_weeks_in_chart) {|i| url(i, key) } urls << url(total_weeks_in_chart, "#{key}_end") end def url(index, id) options = { :controller => 'stats', :action => 'show_selected_actions_from_chart', :index => index, :id=> id, :only_path => true } url_for(options) end def time_labels labels = Array.new(total_weeks_in_chart) {|i| "#{i}-#{i+1}" } labels[0] = "< 1" labels[total_weeks_in_chart] = "> #{total_weeks_in_chart}" labels end def total_weeks_in_chart [52, total_weeks_of_data].min end def total_weeks_of_data weeks_since(dates.last) end def weeks_since(date) (today - date).to_i / 7 end def cumulative_percentages running_total = 0 percentages_per_week.map {|count| running_total += count} end def percentages_per_week datapoints_per_week_in_chart.map(&percentage) end def percentage Proc.new {|count| (count * 100.0 / dates.count)} end def datapoints_per_week_in_chart frequencies = Array.new(total_weeks_in_chart) {|i| datapoints_per_week[i].to_i } frequencies << datapoints_per_week.inject(:+) - frequencies.inject(:+) end def datapoints_per_week frequencies = Array.new(total_weeks_of_data + 1, 0) dates.each {|date| frequencies[weeks_since(date)] += 1 } frequencies end end end