Best way to thin out controller

Rails
1

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

2

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

Jason

3

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

On 5/14/07, Vinnie [email protected] wrote:

  flash[:notice] = "Saved Changes"
  redirect_to :action => "view_page", :id => subpage
else
  render :action => "add_sub"
end

end

–
http://www.web-buddha.co.uk

4

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.

5

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
6

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

7

On 5/14/07, Michael K. [email protected] wrote:

I don’t think it’s worth the extra abstraction unless you feel like
that part of your architecture is going to change

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