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


У меня есть две вспомогательные методы, которые возвращают ли для просмотра с Li для текущего вида, имеющий ID, как некоторые.

def get_li(a)
  li = "<li"
  li += ' id=selected' if (a[:controller] == params[:controller] && a[:action] == params[:action])
  li += ">" + link_to(a[:text], { :controller => a[:controller], :action => a[:action]}) + "</li>"
end

def get_tab_options
  if (params[:controller] == 'sessions' || params[:controller] == 'users')
    [{:text => "Login", :controller => 'sessions', :action => 'new'},
     {:text => "Sign Up", :controller => 'users', :action => 'new'}]
  elsif (params[:controller] == 'meetings')
    [{:text => "Meetings List", :controller => 'meetings', :action => 'index'},
     {:text => "Create Meeting", :controller => 'meetings', :action => 'new'},
     {:text => "Running Meetings", :controller => 'meetings', :action => 'monitor'}]
  end
end

И вид файла

 %ul
   -get_tab_options.each do |a|
     = get_li(a)

Как я могу написать это лучше?



383
6
задан 10 марта 2011 в 04:03 Источник Поделиться
Комментарии
1 ответ

Прежде всего давайте поговорим об именах способ:

В get_ префикс обычно не используется в Ruby - ты просто оставил его.

Далее имя get_li (или просто ли) не очень описательный, что метод делает. Он говорит мне, что он будет производить

  • есть , конечно, но это не говорит мне, что
  • будет содержать ссылку. Из названия метода, я бы просто предположить, что это общее вспомогательный метод я могу использовать, когда я хочу, чтобы элемент списка. Надо, наверное, было бы назвать что-то вроде list_item_with_link или, возможно, navigation_entry, предполагая, что
  • ы, изготовленных по этой технологии, предназначены в качестве записей в меню навигации/.

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


    Ваши методы также должны быть прокомментированы. Уверен, что в случае короткого методов, как это, это легко достаточно, чтобы сказать, что он делает, глядя на код, но там все равно должны быть комментарии, поясняющие, что метод делает, когда и как это предназначается, чтобы использоваться, и какие аргументы должен выглядеть.


    Теперь собственно код: вместо того чтобы строить ли тег с помощью конкатенации строк, вы должны использовать content_tag помощник, которая создает HTML-теги, содержащие, как

  • .

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


    На общий дизайн, обратите внимание, мне кажется неправильным, что ты ограничившись одной только содержат :контроллер и действие. Может быть, вы позже хотите добавить ссылку на действие с определенным параметром. Ваш текущий дизайн не позволит.

    Для этого вы можете просто использовать вашу текущую систему и просто удалить :текст ключа из хэш-код после его получения и затем передает остальные в link_to. Однако это будет перерыв, если вы когда-нибудь действие, которое вы хотите указать параметр с именем текст.

    Так что я бы сделать, это отделить текстовую ссылку с ссылку. Например массив, где первый элемент-это текст ссылки и второе место может сработать. Таким образом ["список встреч", {:контроллер => 'встреч', :действие => 'индекс'}], ["список встреч", {:контроллер => 'встреч'}] и ["список встреч", {:контроллер => 'встреч', :действие => 'индекс', :show_only_active => истина}] все ссылки.

    Вместо массива, это может быть даже приятнее использовать структуру/класс. Таким образом, вы могли бы также положить логику определения того, является ли ссылка на текущую страницу и как превратить ссылку в HTML в этом классе.


    Со всех этих точек, мой код будет, вероятно, выглядеть так:

    # Class to represent links.
    # First argument to new will be the text to be displayed for the link and the second
    # argument will be the location that the link points to as a hash.
    Link = Struct.new(:text, :location) do
    include ActionView::Helpers

    # Turns this link into an HTML link
    def to_html
    link_to(text, location)
    end

    # Returns true if this link points to the same controller and action as the given
    # location hash.
    def points_to?(other_location)
    # If no action is specified, the :index action is used
    # This way the locations {:controller => :foo} and {:controller => :foo, :action => :index}
    # will be treated as equal
    action = location[:action] || :index
    other_action = other_location[:action] || :index
    location[:controller] == other_location[:controller] && action == other_action
    end
    end

    # Returns an li tag containing a link to the location represented by the given Link object.
    # The tag's id will be "selected" if the link points to the current action and controller.
    def navigation_item(link)
    selected = link.points_to?(params) ? "selected" : nil
    content_tag(:li, link.to_html, :id => selected)
    end

    Вы бы затем использовать ссылку.новый("название", :контроллер => "фу", :действие => бар) внутри get_tab_options , чтобы создать ссылку.


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

  • 7
    ответ дан
    sepp2k 10 марта 2011 в 11:03 Источник Поделиться