В предыдущей статье мы использовали технику «золотого мастера» для вывода всего тела HTTP-ответа в файл. Этот файл был быстрой и грязной проверкой того, что наши изменения не изменили поведение системы.
После проведения теста мы удалили логику из шаблонов представления и в конечном итоге удалили целый шаблон, вызвав один шаблон из двух разных действий контроллера. Этот рефакторинг улучшил слой представления, но в процессе он добавил больше логики и больше дублирования к некоторым уже неопрятным действиям контроллера.
В этой статье мы собираемся обратить наше внимание на контроллер.
Подведение итогов
У нас есть два действия контроллера, каждый из которых содержит 40 строк кода.
Эти 80 строк содержат запросы, вычисления, простой поиск данных, вызовы вспомогательных методов и 18 назначений переменных экземпляра.
Готовьтесь, вот с чем мы имеем дело:
def actions_visible_running_time_data @title = t('stats.current_running_time_of_incomplete_visible_actions') @actions_running_time = current_user.todos.not_completed.not_hidden.not_deferred_or_blocked. select("todos.created_at"). reorder("todos.created_at DESC") @max_weeks = difference_in_weeks(@today, @actions_running_time.last.created_at) @actions_running_per_week_array = convert_to_weeks_from_today_array(@actions_running_time, @max_weeks+1, :created_at) # cut off chart at 52 weeks = one year @count = [52, @max_weeks].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, @actions_running_time.count ) @url_labels = Array.new(@count){ |i| url_for(:controller => 'stats', :action => 'show_selected_actions_from_chart', :index => i, :id=> "avrt", :only_path => true) } @url_labels[@count]=url_for(:controller => 'stats', :action => 'show_selected_actions_from_chart', :index => @count, :id=> "avrt_end", :only_path => true) @time_labels = Array.new(@count){ |i| "#{i}-#{i+1}" } @time_labels[0] = "< 1" @time_labels[@count] = "> #{@count}" @y_legend = t('stats.running_time_legend.actions') @y2_legend = t('stats.running_time_legend.percentage') @x_legend = 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 render :actions_running_time_data, :layout => false end def actions_running_time_data @title = t('stats.running_time_all') @actions_running_time = current_user.todos.not_completed.select("created_at").reorder("created_at DESC") # convert to array and fill in non-existing weeks with 0 @max_weeks = difference_in_weeks(@today, @actions_running_time.last.created_at) @actions_running_per_week_array = convert_to_weeks_from_today_array(@actions_running_time, @max_weeks+1, :created_at) # cut off chart at 52 weeks = one year @count = [52, @max_weeks].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, @actions_running_time.count ) @url_labels = Array.new(@count){ |i| url_for(:controller => 'stats', :action => 'show_selected_actions_from_chart', :index => i, :id=> "art", :only_path => true) } @url_labels[@count]=url_for(:controller => 'stats', :action => 'show_selected_actions_from_chart', :index => @count, :id=> "art_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 = t('stats.running_time_legend.actions') @y2_legend = t('stats.running_time_legend.percentage') @x_legend = 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 render :layout => false end Если ваши глаза просто замерзли, и вам захотелось хныкать, ползти в постель и читать комиксы с фонариком, то, будьте уверены, переживание, которое вы испытываете, совершенно нормально.
Однако, если вы станете сами и сравните два метода построчно, вы обнаружите, что эти два метода почти одинаковы.
Почти .
Всего три различия.
- Ключ, используемый для заполнения
@title - Запрос, который хранит вещи в
@actions_running_time - Идентификатор
:idвurl_for
Что я действительно хочу это:
def something_something_data title = t('some.title.key') records = Stats::SomeStuff.for(current_user) @something = Stats::Something.new(title, records, "a_key") render :something, layout: false end
Беспорядок все еще там, скрывается в Stats::Something . Мы не будем беспокоиться о том, чтобы сделать этот код читабельным , мы просто переместим его с безопасностью золотых мастер-тестов, которые мы использовали ранее.
Предварительное начало
Во-первых, мы должны убедиться, что тесты проходят:
ruby -Itest test/functional/lockdown_test.rb
Если это не так, то это потому, что бормотают часовые пояса и бормотают , и вам нужно просто переместить received.html файл.html поверх approved.html файл .html.
Далее, мы собираемся немного перестроить код, чтобы отделить его от одного и того же в наших двух методах.
Извлеките :id использованный в url_for в переменную экземпляра и url_for его в верхнюю часть метода с другими отличиями.
Если вы думаете eeew, другая переменная экземпляра ??!? тогда я приветствую ваши инстинкты. Мы все еще собираемся сделать это таким образом, потому что это позволит нам легче перемещать код.
Запустите тесты еще раз, чтобы убедиться, что рефакторинг переменных извлечения сработал.
Теперь у нас есть 3 строки различий в верхней части двух методов, за которыми следуют 37 строк кода, которые идентичны в обоих местах.
Обратите внимание, что самая последняя строка, render , является совершенно законным контроллером, и мы не будем ее трогать.
В хорошей форме рефакторинга мы собираемся скопировать , а не вырезать весь идентичный код (кроме рендера) и вставить его в app/models/stats/running_time_data.rb :
module Stats class RunningTimeData def initialize(title, actions_running_time, key) @title = title @actions_running_time = actions_running_time @key = key end def compute # 36 lines of scary code end end end
Контроллер все еще имеет весь свой код, тесты должны проходить.
Теперь Stats::RunningTimeData экземпляр Stats::RunningTimeData внутри одного из действий контроллера и вызовите compute для него. Пока мы не будем использовать результаты, мы просто посмотрим, сможет ли код работать.
def actions_running_time_data @title = t('stats.running_time_all') @actions_running_time = current_user.todos.not_completed.select("created_at").reorder("created_at DESC") @key = "art" @data = Stats::RunningTimeData.new(@title, @actions_running_time, @key) @data.compute # 37 more lines of code end
Тесты не выполняются со следующим сообщением:
NoMethodError: undefined method `difference_in_weeks' for #<Stats::RunningTimeData:0x007fd367920d28>
Преследование зависимостей
Нам не хватает вспомогательного метода. Мы собираемся скопировать его из контроллера в наш новый класс Stats::RunningTimeData .
Когда мы делаем это, мы получаем новое сообщение об ошибке.
NoMethodError: undefined method `difference_in_days' for #<Stats::RunningTimeData:0x007fba3dfedec8>
Когда мы приводим это, тест жалуется:
NoMethodError: undefined method `utc' for nil:NilClass
Вот код, который взрывается:
# assumes date1 > date2 def difference_in_days(date1, date2) return ((date1.utc.at_midnight-date2.utc.at_midnight)/SECONDS_PER_DAY).to_i end
Итак, date1 или date1 — ноль. Прослеживая код, оказывается, что date1 представлен переменной экземпляра @today которая не определена нигде в нашем новом классе.
Контроллер определил это значение в фильтре before.
Убедитесь, что RunningTimeData принимает значение и сохраняет его в переменной экземпляра с именем @today , а затем передает его, когда объект создается в контроллере:
@data = Stats::RunningTimeData.new(@title, @actions_running_time, @key, @today)
Это исправляет одно сообщение об ошибке и дает нам новое. На этот раз нам не хватает постоянной Stats::RunningTimeData::SECONDS_PER_DAY .
Скопируйте это и следуйте за NoMethodError сообщений NoMethodError пока вы не перетащите все следующее в новый класс из контроллера.
-
convert_to_weeks_from_today_array -
convert_to_array -
cut_off_array_with_sum -
convert_to_cumulative_array
Помните, что вам нужно копировать. Если вы удалите какой-либо код из контроллера, пройдет еще несколько часов, прежде чем что-либо пройдет снова.
Угадай, откуда я это знаю.
Последняя ошибка — это еще одно NoMethodError , но это не метод, определенный в контроллере, это url_for , который является одним из помощников URL Rails.
Это нормально, мы можем включить их!
module Stats class RunningTimeData include Rails.application.routes.url_helpers # … end end
Это, наконец, проходит испытания.
Как это выглядит?
Ну, мы собирались выделить 40 строк кода в новый класс, и теперь у нас есть 100 строк кода.
Более того, все эти строки кода все еще существуют в контроллере.
Вещи еще не намного лучше.
Тем не менее, это дает нам представление о масштабе проблемы, и этот код контроллера со временем исчезнет, поэтому я собираюсь назвать это победой.
Очень маленькая, очень грязная победа.
В обход контроллера
Контроллер имеет экземпляр нового объекта Stats::RunningTimeData хранящийся в @data , что означает, что представление имеет к нему доступ.
В первой строке представления мы используем @title:
&title=<%= @title -%>,{font-size:16},&
Если мы добавим attr_reader :title в Stats::RunningTimeData , то мы можем изменить представление следующим образом:
&title=<%= @data.title -%>,{font-size:16},&
Конечно, теперь тесты не пройдены, потому что у нас есть два действия, использующих одно и то же представление, а одно из действий еще не имеет переменной @data .
Самое простое, что нужно сделать, это просто установить другое действие с некоторыми @data . Скопируйте две строки вниз к другому действию, и тесты должны пройти снова.
Теперь, по одному, attr_reader для значения, необходимого в представлении, и поменяйте местами представление для чтения из объекта @data .
Не забудьте запускать тесты для каждого изменения.
Это должно быть гладко, но это не так. Во второй строке, когда мы @data.y_legend мы получаем ошибку с самым специфическим diff:
< &y_legend=&lt;p>stats.running_time_legend.actions</p>,10,0x736AFF& --- > &y_legend=Actions,10,0x736AFF&
В теле ответа говорилось « Actions , но теперь оно говорит: <p>stats.running_time_legend.actions</p> .
Что здесь происходит?
Вставьте вызов pry из Stats::RunningTimeData#compute чтобы мы могли спросить, где определен метод t .
binding.pry @y_legend = t('stats.running_time_legend.actions')
В командной строке pry наберите t , что приведет к появлению удобной трассировки стека:
[1] pry(#<Stats::RunningTimeData>)> t ArgumentError: wrong number of arguments (0 for 1) from /Users/kytrinyx/.gem/ruby/2.1.0/gems/RedCloth-4.2.9/lib/redcloth/erb_extension.rb:16:in `textilize'
RedCloth! Это не то, что я ожидал. t() в модели — это textilize() , в то время как t() в translate I18n контроллера, оба из которых имеют сокращение t() .
Мы можем исправить это, явно вызвав t на I18n внутри Stats::RunningTimeData#compute :
@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')
Тесты снова проходят, и мы можем вернуться к утомительному, один за другим процессу предоставления attr_reader s в Stats::RunningTimeData .
attr_reader :title, :y_legend, :y2_legend, :x_legend, :values, :links, :values_2, :x_labels, :y_max
Больше никаких сюрпризов нет, и мы можем сократить действия контроллера:
def actions_visible_running_time_data title = t('stats.current_running_time_of_incomplete_visible_actions') actions_running_time = current_user.todos.not_completed.not_hidden.not_deferred_or_blocked. select("todos.created_at"). reorder("todos.created_at DESC") @data = Stats::RunningTimeData.new(title, actions_running_time, "avrt", @today) @data.compute render :actions_running_time_data, :layout => false end def actions_running_time_data title = t('stats.running_time_all') actions_running_time = current_user.todos.not_completed.select("created_at").reorder("created_at DESC") @data = Stats::RunningTimeData.new(title, actions_running_time, "art", @today) @data.compute render :layout => false end
Вывод
Это лучше?
Контроллер и представление теперь совместно используют только одну переменную экземпляра. Это определенно улучшение.
Сами действия контроллера уменьшены до 5 строк, что лучше, чем 40, но все же не идеально. Запросы все еще безобразны. Мы могли бы убрать их в какой-то другой объект.
Раздражает, что мы должны вызывать compute на данных.
А как насчет Stats::RunningTimeData ?
Это довольно ужасно, на самом деле. Это 100 строк. У него нет дублирования, но есть метод вычисления монстра и несколько непонятных вспомогательных методов.
Другими словами, мы еще не закончили. Закатайте рукава, перекусите и приготовьтесь к рефакторингу этой модели!