<% uncategorized_courses = Course.visible.actual.uncategorized %>
<% if uncategorized_courses.present? %>
<h2>Other courses</h2>
<ul>
<% uncategorized_courses.each do |course| %>
<%= render :partial => "courses/course", :locals => {:course => course} if course.public? %>
<% end %>
</ul>
<% end %>
Refactorings
No refactoring yet !
steved
October 28, 2010, October 28, 2010 15:20, permalink
This doesn't look too bad. You might: 1) fetch the courses in the controller, 2) render the h2 and ul in a partial (presumably you have other collections of courses).
Not sure if the check for public? should happen in the controller or the view.
# courses controller
def index
@uncategorized_courses = Course.visible.actual.uncategorized
end
# index.html.erb
<%= render :partial => "courses/courses", :locals => {:courses => @uncategorized_courses, :title => 'Other'}
# _courses.html.erb
<% if courses.present? %>
<h2><%= title %> courses</h2>
<ul>
<% courses.each do |course| %>
<%= render :partial => "courses/course", :locals => {:course => course} if course.public? %>
<% end %>
</ul>
<% end %>
Adam
October 28, 2010, October 28, 2010 15:39, permalink
steved has the right idea, but I am inferring that this is part of a sidebar or some other element that is repeated on many pages, not just one action. Therefore, I propose you instantiate the Courses in a before_filter rather than the action itself. I also simplified the render :partial call. If you pass an object that quacks like ActiveModel::Naming, render :partial will automatically find the partial and assign the local variable in the same convention you used. If you pass an array of objects, it will also do the iteration for you.
class FooBarsController < ApplicationController
before_filter :set_uncategorized_courses
private
def set_uncategorized_courses
@uncategorized_courses = Course.visible.actual.uncategorized.public_courses
end
end
<% if @uncategorized_courses.present? %>
<h2>Other courses</h2>
<ul>
<%= render :partial => @uncategorized_courses %>
</ul>
<% end %>
papricek.myopenid.com
October 28, 2010, October 28, 2010 18:41, permalink
No, its just one action. However both solutions are elegant, thanks guys!
<% if @uncategorized_courses.present? %>
<h2>Other courses</h2>
<ul>
<%= render :partial => @uncategorized_courses %>
</ul>
<% end %>
Hi, how to write this more DRY?... :)