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 subpage.save
      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.

Jason

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 (http://en.wikipedia.org/wiki/Law_of_Demeter) is a good
one to try and follow wherever realistic.

- Gabriel

Your subpage.save 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.

-Michael
http://javathehutt.blogspot.com

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