Controller DRYness question

Rails
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:

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

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

Elias O. 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

4

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

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

On Nov 11, 11:15 am, Nick H. [email protected]

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

On Nov 16, 11:13 am, Nick H. [email protected]