Controller DRYness question

rubyonrails-talk
1

I have some questions regarding how to avoid having to duplicate code in the controller between certain actions such as "new", "create", "edit", "update", etc.

Currently, my "new" and "update" actions both load a couple collections of reference data used to populate drop-down/select controls in the view. To avoid duplicating the same code in both actions, I am defining a before_filter:

[code] class MyController   before_filter :find_states, :only => [:new, :edit]   before_filter :find_categories, :only => [:new, :edit]

  ... # action definitions

  private

  def find_states     @states = State.find(:all, :order => "name")   end

  def find_categories     @categories = Category.find(:all, :order => "name")   end end [/code]

So this is all well and good, the new and edit actions are just fine.

However, my issue is really with the "create" and "update" actions which are responsible for actually updating the DB. In the case where the save or update operations fail, these methods will try to render the "new" or "edit" views. In this case, I need to make sure the reference data that is required is populated.

I see several solutions to this problem, but none of them particularly strike me as great solutions, or seem particularly DRY. So I guess I'm just looking for something a little better/cleaner/DRYer, however you want to put it.

Anyhow, my current ideas for solutions, none of which I really like, are as follows:   1. In the "create" and "update" actions, call the filter methods prior to rendering the new/edit views (don't like this, since if additional reference data is added later, I will have to add/remove more method calls)

  2. Add additional helper methods that set all of the reference data for a particular action (i.e. find_ref_data_for_new) (this is better than option 1, but would still potentially have to modify code in multiple locations)

  3. Add "create" and "update" to the list of actions included in the before_filter (this would work, but it just doesn't make sense, since "create" and "update" are really DB related actions, and only display data when some DB operation, or validation, fails)

Any help or suggestions would be greatly appreciated.

- Justin

2

ck.>

Anyhow, my current ideas for solutions, none of which I really like, are as follows:   1. In the "create" and "update" actions, call the filter methods prior to rendering the new/edit views (don't like this, since if additional reference data is added later, I will have to add/remove more method calls)

  2. Add additional helper methods that set all of the reference data for a particular action (i.e. find_ref_data_for_new) (this is better than option 1, but would still potentially have to modify code in multiple locations)

  3. Add "create" and "update" to the list of actions included in the before_filter (this would work, but it just doesn't make sense, since "create" and "update" are really DB related actions, and only display data when some DB operation, or validation, fails)

#1: Dislike this for your very reasons.

#3: Dislike this because I like the idea of parsimony in the execution of the application. In the create and update actions, you don't always need that reference data, and to fetch that data every time, is unnecessary.

#2: Seems to be the best compromise between too much coding, and too much execution.

3

Hey Justin,

I'll recommend that every data used to load drop down lists on the views should be loaded on the views, that avoids repetition on the controllers. For example a list of post categories you can load it like this:

<%= collection_select(:post, :post_category_id, PostCategory.find(:all), :id, :name, {}) %>

Hope it helps,

Elías

4

Elias Orozco wrote:

Hey Justin,

I'll recommend that every data used to load drop down lists on the views should be loaded on the views, that avoids repetition on the controllers. For example a list of post categories you can load it like this:

<%= collection_select(:post, :post_category_id, PostCategory.find(:all), :id, :name, {}) %>

Hope it helps,

El�as

That's a good suggestion, Elias. I'll add that, if the drop-down list is going to be in multiple views, I'd put it in a helper method. -Nick

5

Nick,

That's a great suggestion. I would actually prefer using a helper method over putting the call to ModelObject#find in the view itself. Something about calling find directly from the view just doesn't feel right to me. The only reason I'm not 100% opposed to it is because a call to find is usually going to be a 1-liner. Even so, it still just feels like it doesn't belong directly in the view itself.

I am new to Rails, but based on my perception of MVC, fetching data from the model is a task that is meant to be done by the controller, and the view should simply be handed that data by the controller. However, looking through the Rails documentation itself, I saw the same method mentioned by Elias (calling find directly in the view) in examples within the ActionView::FormOptionsHelper class, so I guess that is a Rails convention? Or was it just done for example purposes?

If you start having to order your reference data, and do other operations where it's not as simple as a call to find(:all), then I think that could really muddy up the view code. A helper method would certainly be preferable, perhaps something like category_select(...), which handles grabbing the data from the model. At least you're not shoving the find call directly in to the view, and you still gain the benefit of not having to call the same find() method from all over the controller.

So far, I have been absolutely blown away by almost all the features that Rails has to offer, but it almost seems like the designers of the framework overlooked, or didn't feel it was important, to add a simple, and DRY, way to fetch reference data from the controller. I am quite surprised by this, as it is a very common task to fetch reference data for the purpose of populating controls such as select/ drop-down boxes. I was also surprised at the lack of message board and blog posts about this subject. Perhaps, as I mentioned, the method that Elias made reference to is the Rails convention, and I am just missing something :slight_smile:

Thanks to everyone for all the great suggestions. Keep em coming.

Regards,

Justin

6

G'day Justin.

That's a great suggestion. I would actually prefer using a helper method over putting the call to ModelObject#find in the view itself. Something about calling find directly from the view just doesn't feel right to me. The only reason I'm not 100% opposed to it is because a call to find is usually going to be a 1-liner.

It might be a one-liner right now, but once you add error handling, it's longer. And what's error handling?...It's logic, which doesn't belong in views.

I am new to Rails, but based on my perception of MVC, fetching data from the model is a task that is meant to be done by the controller, and the view should simply be handed that data by the controller.

Yup! Absolutely.

However, looking through the Rails documentation itself, I saw the same method mentioned by Elias (calling find directly in the view) in examples within the ActionView::FormOptionsHelper class, so I guess that is a Rails convention? Or was it just done for example purposes?

There could be all sorts of reasons why the example's writer put the call to #find in the view. Regardless, "The Rails Way" is to not put logic in views.

So far, I have been absolutely blown away by almost all the features that Rails has to offer, but it almost seems like the designers of the framework overlooked, or didn't feel it was important, to add a simple, and DRY, way to fetch reference data from the controller.

I think you might be confusing the act of "fetching" reference data with the act of formatting and/or displaying it. Controllers share data (IE: variables) with views via instance variables, or passing variables to the ":locals" option of #render. That's pretty simple. If multiple controller actions need to share the same data, simply write a protected or private controller method, or add a method to the model in question, that all of these controller actions can call.

I am quite surprised by this, as it is a very common task to fetch reference data for the purpose of populating controls such as select/ drop-down boxes. I was also surprised at the lack of message board and blog posts about this subject. Perhaps, as I mentioned, the method that Elias made reference to is the Rails convention, and I am just missing something :slight_smile:

As you know, views shouldn't contain any sort of logic. If you're thinking about calling #find within a view, what will you do with the data that's returned? Most of the time, you'll need to check if any records were returned. After that, you'll iterate through the Array, and display some data.

That's "business logic", which doesn't belong in views. But it does belong in helpers! -Nick

7

Nick,

Thanks again for the reply.

I think I will probably go back to using either protected controller methods or helper methods to fetch the reference data. I agree with you 100% in that this type of logic simply has no place in the view.

As for my statement regarding the framework lacking a way to easily, and DRYly, fetch reference data... I am just surprised that there is not any kind of shortcut/helper for loading reference data now. Perhaps a function built in to ActiveController such as "load_ref_data" which could optionally take an argument that would specify the name of the action to load data for (would default to current action), and could potentially use some type of naming convention to call a method that gets all of the reference data for the action. I'm sure there are more clever ways to do this, and this is just off the top of my head.

So you might have something in you controller such as...

def new   load_ref_data   ... end

def create   ...   if @myObject.save     ...   else     load_ref_data :new     render :action => :new end

protected

def ref_data_new   @categories = Category.find(...)   @states = State.find(...)   ... end

You might even add an option to the render method itself, such as "render :action => :new, :include_ref_data => true".

I will probably end up rolling my own system, but I am surprised with all the things that they thought of for Rails, that they didn't think of something like this.

- Justin