Simple helper method - Am I doing it right?

Question

New to Ruby and Rails.

I am developing a simple app for where you can register teams, players etc and it looks like this:

Team has_many Players Players belongs_to Team

When I want to show the player in view(normal users):

<%= @player.name %> - <%= playerteam %>

and in the admin view it looks like this:

<% @players.each do |player| %>
    <tr>        
        <td><%= player.id %></td>   
        <td><%= player.name %></td>
        <td><%= playerteam(player) %></td>
        <td><%= owner(player) %></td>               
    </tr>
<% end %>

and the helper method:

def playerteam(player = nil)      
  if player != nil
    if player.team_id == nil
      return "No team"
    else
      @team = Team.find(player.team_id)
      return @team.name
    end
  else              
    if @player.team_id == nil
      return "No team"      
    else
      @team = Team.find(@player.team_id)
      return @team.name
     end
  end
end

It works but it is not pretty or "Ruby Sexy"

At first it was only used from the normal view but then when I wanted to use it from the admin-view also I had to add the parameter with a default value and the extra if-clause.

Are there a better way?

Answer 1

def playerteam(player = nil)      

    if (player || @player).team_id == nil
      return "No team"
    else
      @team = Team.find((player || @player).team_id)
      return @team.name
    end

end

Seems to be enough in your case. Or you can made the (player || @player) in your view

<% @players.each do |player| %>
    <tr>        
        <td><%= player.id %></td>   
        <td><%= player.name %></td>
        <td><%= playerteam((player || @player)) %></td>
        <td><%= owner(player) %></td>               
    </tr>
<% end %>

with helper simplest because you define all the time a player

def playerteam(player)      
    if player.team_id == nil
      return "No team"
    else
      @team = Team.find(player.team_id)
      return @team.name
    end

end

Answer 2

Looks like you can just do:

def playerteam(player = @player)
  if player.team_id == nil
    return "No team"
  else
    @team = Team.find(player.team_id)
    return @team.name
  end
end

But you may also want to tidy your code up a little:

def playerteam(player = @player)
  if player.team
    return player.team.name
  else
    return "No team"
  end
end

If you do want to set @team to the player's team then I'd do it somewhere other than a helper method as it is very confusing to be setting attributes in a helper method. You are then relying on the helper method having been called to access @team.

Answer 3

You can definitely refactor that!

def playerteam(player = nil)      
  player ||= @player
  return "No team" if player.team_id.nil?
  Team.find(player.team_id).name
end

Answer 4

I wouldn't use a helper method at all. Helper Methods shouldn't be used to retrieve model data. That's what's the model is for.

you can do something like this:

normal users:

<%= @player.name %> - <%= player.team ? player.team.name : 'No Team' %>

admin:

<% @players.each do |player| %>
    <tr>        
        <td><%= player.id %></td>   
        <td><%= player.name %></td>
        <td><%= player.team ? player.team.name : 'No Team' %></td>
        <td><%= owner(player) %></td>               
    </tr>
<% end %>

You could probably do something similar with the owner-helper

to avoid the ? :-if-else syntax in every view you can add this to your player model

class Player < ActiveRecord::Base
  def team_name
    team ? team.name : "No Team"
  end
end

Then you views look like this:

normal users:

<%= @player.name %> - <%= @player.team_name %>

admin:

<% @players.each do |player| %>
    <tr>        
        <td><%= player.id %></td>   
        <td><%= player.name %></td>
        <td><%= player.team_name %></td>
        <td><%= owner(player) %></td>               
    </tr>
<% end %>

IMHO: This would be much more "Rails Sexy" ;-)