Rails: Helper Refactoring

I have a helper which I feel is ridiculous, but I haven't been able to think of a way to improve it. Here's the helper in question:

# Shows Admin Menu Button
def admin_toggle_button
  if user_signed_in? && ( current_user.has_role?(:admin) || ( @collection && can?(:curate,@collection) ) )
    if session[:admin_menu] == :on
      link_to( 'Admin Tools', 开发者_如何学Pythonedit_shared_path(:admin_menu => :off), :remote=>true, :class => 'selected', :id => 'admin_toggle_button', :title => 'Hide Admin Menu' )
    else
      link_to( 'Admin Tools', edit_shared_path(:admin_menu => :on), :remote=>true, :id => 'admin_toggle_button', :title => 'Show Admin Menu' )
    end
  end
end

In my application's menubar I call admin_toggle_button and this helper determines whether or not that button should be present and what its state is.

For the admin menu button to be present there needs to be a logged-in user, and that user needs to be an administrator OR that user needs to be viewing a collection which he is allowed to curate (edit).

My question is: Are helper methods like this normal -- ie do you find you need methods this complex from time to time -- or am I missing something? Can you suggest a way to improve this method?

---

You can make a method for the first condition so you can reuse it elsewhere.

def can_view_admin_stuff?
  user_signed_in? && ( current_user.has_role?(:admin) || ( @collection && can?(:curate,@collection) ) )
end

def admin_toggle_button
  return '' unless can_view_admin_stuff?

  if session[:admin_menu] == :on
    link_to( 'Admin Tools', edit_shared_path(:admin_menu => :off), :remote=>true, :class => 'selected', :id => 'admin_toggle_button', :title => 'Hide Admin Menu' )
  else
    link_to( 'Admin Tools', edit_shared_path(:admin_menu => :on), :remote=>true, :id => 'admin_toggle_button', :title => 'Show Admin Menu' )
  end
end

But yeah, it's normal I guess to find methods like especially if you're only using the condition for the link. If you're using it for other parts of your code, it'd be nice to have a helper for it.

---

That method isn't terribly complex. The most complicated part is the access checking and even that isn't too bad. One of the reasons for helpers is to keep stuff like that out of your views so you're not doing anything wrong.

The repetition between the two link_to variants might need some attention. You could restructure it a little but:

def admin_toggle_button
  return '' if !user_signed_in? || !( current_user.has_role?(:admin) || ( @collection && can?(:curate,@collection) ) )
  opts = {
    :id     => 'admin_toggle_button',
    :remote => true,
    :title  => 'Show Admin Menu'
  }
  admin_menu = :on
  if session[:admin_menu] == :on
    opts[:title] = 'Hide Admin Menu'
    opts[:class] = 'selected'
    admin_menu   = :off
  end
  link_to('Admin Tools', edit_shared_path(:admin_menu => admin_menu), opts)
end

This approach highlights the differences between the two possible link_to calls. If session[:admin_menu] != :on is more common then maybe you'd want to reverse the logic to start opts and admin_menu with the "Hide Admin Menu" settings and then adjust them to the != :on case as needed.

def admin_toggle_button
  return '' if !user_signed_in? || !( current_user.has_role?(:admin) || ( @collection && can?(:curate,@collection) ) )
  opts = {
    :class  => 'selected',
    :id     => 'admin_toggle_button',
    :remote => true,
    :title  => 'Hide Admin Menu'
  }
  admin_menu = :off
  if session[:admin_menu] != :on
    opts[:title] = 'Show Admin Menu'
    opts.delete(:class)
    admin_menu   = :on
  end
  link_to('Admin Tools', edit_shared_path(:admin_menu => admin_menu), opts)
end