Best way to thin out controller

Sorry for such a nub question but How would I move the majority of this logic to the model?

  def save_sub     page = Page.find(params[:id])     subpage = page.children.create(params[:form])        if       flash[:notice] = "Saved Changes"       redirect_to :action => "view_page", :id => subpage     else       render :action => "add_sub"     end   end

Why would you want to? The controller is the place for this logic surely?

Aye, that’s perfectly fine Controller code. Short and to the point.


I would suggest perhaps an add_child method on instances of Page. This way if the implementation of how Pages and child Pages are related in your model changes, your controllers are shielded from the change. The Law of Demeter (Law of Demeter - Wikipedia) is a good one to try and follow wherever realistic.

- Gabriel

Your call is unnecessary, create saves your object for
you. You can just check if the created object is a new_record?
(subpage.new_record?) or not to determine whether or not the save was
successful. And like was already mentioned the only other thing you
could do to improve this is to create a method on your page object
that hides the relationship from page to children but I don't think
it's worth the extra abstraction unless you feel like that part of
your architecture is going to change. Anyway, your call, what you
have now is fine.


Agreed with the others. Most of this code is about what gets presented in the browser, none of that stuff should be in the model in any way. If you want dry things up, one pattern I sometimes use is:

before_filter :load_object

private def load_object   @object = Object.find(params[:id]) if params[:id] end

But that may not be worth it depending on the rest of your controller.

It's a fine line between under-abstraction and over-abstraction, but I've found the parts you think are most unlikely to change are the ones that end up hurting the most to change unless you've taken reasonable preparatory measures :slight_smile:

- Gabriel