views:

278

answers:

4

I know that it is best to keep code out of the presentation layer. But, I am wondering how much is considered "acceptable". For example I populate an html select box with this line of code.

CodesecureProject.find(:all,:order => 'name').collect {|project| [project.name, project.id] }

Right now I have this line of code embedded in the form. What I am wondering if the community thinks if this is to much code and it should be first stored in an instance variable on the controller then the variable used in the form.

+1  A: 

I have a lot of the same code in my projects except I try to don't do any finds. In your case I would make an named scope

named_scope :order, lambda { |order| {:order => order}}

and make the code:

CodesecureProject.order(:name).collect {|project| [project.name, project.id] }

It's a little cleaner.

If you got a lot of select boxes which need a name and an id (I sure do sometimes), you could also try making a helper that excepts a ModelName and returns the array you need.

def magic_for_select(model)
  model.all.collect{|instance| [instance.name, instance.id]}
end
Maran
calling your scope "ordered_by" would actually make this code very natural to read!
Vincent Robert
+4  A: 

I'm not going to say I'd never do it (I'd be lying) but the code example given would make me nervous. I think I'd be more inclined to deliver the data to the select box from my controller. A helper method is another option if I notice I'm doing something more than once. I'm more likely to see the duplication in the controller than across distinct views.

If I'm using the same HTML component across multiple views, then I might find myself reaching for partials or wrapping the whole thing in a custom helper: project_select() or some such.

The more I work in the MVC world the more I find myself avoiding code in views. I have a feeling that some kind of Zen mastery will be achieved if I reach the zero code state, although the value of that in anything but philosophical terms is highly debatable.

Mike Woodhouse
+1  A: 

I would go a bit further than Maran. Generally I do the following:

  • Create a named_scope in the model to execute the find.
  • Call the named_scope from the controller and store the results in an instance variable.
  • Only put the instance variable in the view.

I would only use a helper if absolutely necessary. When going back over your code later, it's easier to make sense of things if you see your controller setting up the data that the view needs, rather than the view calling the helper (yet another file to look at).

insane.dreamer
+4  A: 

I use the following static method in a Site model to achieve something similar:

class Site
  def self.select_options
    Site.find(:all, :order => 'UPPER(name)').collect {|s| [s.name, s.id]}
  end
def

Then in my Domain view I call:

<%= f.select :site_id, Site.select_options %>

This works really well for these circumstances.

In your instance, you might try:

class CodesecureProject
  def self.select_options
    CodesecureProject.find(:all, :order => 'name').collect {|p| [p.name, p.id]}
  end
end

And then call it through the view with:

<%= f.select :codesecure_project_id, CodesecureProject.select_options %>
mlambie
If you have a list of something that doesn't change, such as states or countries, you can even do a class constant and then it's only loaded once per Rails instance.
Sarah Mei