Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
<% i = 0 %> 
<% @patients.each do |patient| %> 
 <tr class="<%= i % 2 == 0 ? 'Even' : 'Odd' %>">
    <td><%= link_to patient.id, patient %></td>
    <td><%= patient.user.username %></td>
    <td><%= patient.user.first_name %></td>
    <td><%= patient.user.last_name %></td>
    <td><%= patient.user.email %></td>
    <td><%= patient.user.active %></td>
    <td><%= patient.user.disabled %></td>
    <td>
      <ul class="Horizlist">
        <li><%= link_to 'Detail', patient %></li>
      </ul>
    </td>
  </tr>
<% i += 1 %>    

I don't like that I have to define an i variable, is there a better way?

share|improve this question
add comment

2 Answers

up vote 10 down vote accepted

Rails has a built in method, cycle, which will cycle between two more more options for you, so you don't have to manage the "Even"/"Odd" selection.

<% @patients.each do |patient| %>
  <%= content_tag :tr, :class => cycle('Even', 'Odd') do %>
    <%= content_tag :td, link_to(patient.id, patient) %>
    <% [:username, :first_name, :last_name, :email, :active, :disabled].each do |property| %>
      <%= content_tag :td, patient.user.send(property) %>
    <% end %>
    <td>
      <ul class="Horizlist">
        <%= content_tag :li, link_to('Detail', patient) %>
      </ul>
    </td>
  <% end %>
<% end %>

If you happen to use Haml, it looks quite nice. :)

- @patients.each do |patient|
  %tr{:class => cycle('Even', 'Odd')}
    %td= link_to(patient.id, patient)
    - [:username, :first_name, :last_name, :email, :active, :disabled].each do |property|
      %td= patient.user.send(property)
    %td
      %ul.Horizlist
        %li= link_to('Detail', patient)
share|improve this answer
    
Damn, how did I not know that? This is of course better than defining your own helper. +1 –  sepp2k Feb 25 '11 at 10:08
    
Also, if this is just for styling purposes: you can do that with CSS without classes. –  rightfold Jan 31 '12 at 19:43
add comment

First of all ruby has an each_with_index method, so you can do @patients.each_with_index do |patient, i| instead of keeping a counter manually.

However with the conditional inside the loop that is still too much logic for the view in my opinion. What I'd do is define a helper for this, which might look like this:

# For each item in the given collection, yield that item and embed the
# result in the given tag. The class attribute of that tag alternates
# between class1 and class2
def alternating(collection, tag, class1, class2, html => {})
  collection.each_with_index do |item, i|
    html[:class] = i % 2 == 0 ? class1 : class2
    content_tag(tag, html, false) do
      yield item
    end
  end
end

And then call it like this:

<% alternating(@patients, :tr, 'Even', 'Odd') do |patient| %> 
  <td><%= link_to patient.id, patient %></td>
  <td><%= patient.user.username %></td>
  <td><%= patient.user.first_name %></td>
  <td><%= patient.user.last_name %></td>
  <td><%= patient.user.email %></td>
  <td><%= patient.user.active %></td>
  <td><%= patient.user.disabled %></td>
  <td>
    <ul class="Horizlist">
      <li><%= link_to 'Detail', patient %></li>
    </ul>
  </td>
<% end %>
share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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