6
\$\begingroup\$

I have two helper methods that return li for view with the li for current view having id as selected.

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

And the view file has

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

How can I write this better?

\$\endgroup\$
7
\$\begingroup\$

First of all let's talk about method names:

The get_ prefix is usually not used in ruby - you'd just leave it off.

Further the name get_li (or just li) is not very descriptive of what the method does. It tells me that it will produce an <li>, sure, but it doesn't tell me that the <li> will contain a link. From the method name I would just assume that this is a general helper method I can use whenever I want a list item. It should probably be called something like list_item_with_link or possibly navigation_entry, assuming that the <li><a>s produced by this method are meant as entries in a navigation bar/menu.

The name of the argument is also not very descriptive. I assume you called it a because of the <a> tag, but a does not really contain an a tag, just the information you use to build one. I'd call it something like link instead.


Your methods should also be commented. Sure in the case of short methods like this, it's easy enough to tell what it does from looking at the code, but there should still be comments explaining what the method does, when and how it's intended to be used and what the arguments should look like.


Now to the actual code: Instead of building the li tag using string concatenation, you should use the content_tag helper instead, which builds HTML-tags containing content, like <li>.

I also feel that the code would become nicer if the the logic to determine whether a link points to the current page was factored into its own method. I'm sure it could be useful in other places as well.


On a general design note, it seems wrong that you're restricting the links to only contain :controller and :action. Maybe you would later like to add a link to an action with a specific parameter. Your current design would not allow that.

For this you could just use your current system and just remove the :text key out of the hash after retrieving it and then passing the rest to link_to. However this would break if you ever have an action to which you want to supply a parameter named text.

So what I would do is the separate the link text from the link location. For example an array where the first element is the link text and the second is the location might work. This way ["Meetings List", {:controller => 'meetings', :action => 'index'}], ["Meetings List", {:controller => 'meetings'}] and ["Meetings List", {:controller => 'meetings', :action => 'index', :show_only_active => true}] would all be valid links.

Instead of an array, it could be even nicer to use a struct/class. This way you could also put the logic of determining whether the link points to the current page and how to turn the link to HTML into that class.


With all those points, my code would probably look like this:

# 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

You'd then use Link.new("title", :controller => "foo", :action => bar) inside get_tab_options to create a link.


As a last note, it also seems a bit icky to me to hardcode which links will be available in which controller into a helper method. It seems that this should rather be handled in the controllers.

\$\endgroup\$
  • \$\begingroup\$ When i tried this i got undefined method `link_to' for ActionView::Helpers:Module error. Am I missing any thing here? \$\endgroup\$ – ssri Mar 14 '11 at 5:33
  • \$\begingroup\$ @ssri: Sorry about that. Fixed now. \$\endgroup\$ – sepp2k Mar 14 '11 at 13:57

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.