CODEWITHSUNDEEP
ruby-on-rails
user3052988
user3052988

Reputation:

How to rewrite this Ruby to be DRYer and conform better to Rails' best practice

I'm fairly sure I've broken some best practises here, could anyone give me some guidance as to where this code should go. If it can't be improved (unlikely) please tell me too :)

<% year = '' %>
<% for activity in @activities %>
  <% if activity.date.strftime('%Y') == year %>
    <%= activity.date.strftime('%d %h') %>
    <%= activity.desc %>
    <br/>
  <% else %>
    <% year = activity.date.strftime('%Y') %>
    <%= year %>
    <br/>
    <%= activity.date.strftime('%d %h') %>
    <%= activity.desc %>
    <br/>
  <% end %>
<% end %>

The aim is to list all the activities in each year under a year "header". I'll tidy up the output later...

The controller simply proves @activities with Activity.all

EDIT: I understand the need for a helper but is it acceptable to add html to a helper, or do i need to do this in the view?

Upvotes: 0

Views: 44

Answers (3)

carpeliam
carpeliam

Reputation: 6769

  1. Please don't use formatted values as a basis for comparison. Many date/time classes (e.g. TimeWithZone) have a year method or thereabouts that give you access to the year. You should use strftime for display, not for logic.

  2. The moment you instantiate a variable in the view is the moment you know you probably should move this into a helper.

    def activities_header(activities)
  activities.group_by { |activity| activity.date.year }.each do |year, activities_by_year|
    concat year
    concat content_tag(:br)
    activities_by_year.each do |activity|
      concat activity.date.strftime('%d %h')
      concat activity.desc
      concat content_tag(:br)
    end
  end
end

I haven't tested this, but the idea here is, group the activities by year to start with. You can then call this from your view with <%= activities_header(activities) %>.

Upvotes: 0

Phlip
Phlip

Reputation: 5343

Write automated tests that cover those branches, then refactor to move that code into a helper method, and DRY it:

def format_dates(activities)
  year = 0
  activities.map do |activity|
    activity_stuff = activity.date.strftime('%d %h ') + activity.desc
    if activity.date.year == year
      activity_stuff
    else
      year = activity.date.year
      [ activity.date.strftime('%Y'),
       activity_stuff ]
    end
  end.flatten
end

Now the HTML.ERB code is just this:

<%=raw format_dates(@activities).join('<br/>') %>

Remember the rubric "smart models, thin controllers, and dumb views" - the helper is really model-side; you could also make that code a static method of Activity.

Upvotes: 1

Arjan
Arjan

Reputation: 9874

The code that is common for both if and else should not be present in either, so you don't have to repeat yourself there.

<% year = '' %>
<% for activity in @activities %>
  <% unless activity.date.strftime('%Y') == year %>
    <% year = activity.date.strftime('%Y') %>
    <%= year %>
    <br/>
  <% end %>
  <%= activity.date.strftime('%d %h') %>
  <%= activity.desc %>
  <br/>
<% end %>

Upvotes: 0

Related Questions