Topic: DRY and RESTful Controllers

As I fall deeper into the Rails rabbithole, I find myself violating the DRY principle quite a bit, especially in my controllers. The problem stems from the fact that I rely so much on script/generate scaffold_resource... the result?

Now I am finally  beginning to understand why all these Rails vets tend to shun generated code... it's a great reference for best practices but it's no substitute for writing your own methods DRY from the start. More and more I am seeing that I need to let Rails natural inheritance work for me.

It occured to me that the bulk of my CRUD actions really belong at the application controller level,, After some fast Googling I saw that I was not alone...

http://geekonomics.blogspot.com/2006/07 … llers.html

http://dereksivers.com/rails-shared-controller.html

Heck I even found a plugin that does the same

http://rubyfurnace.com/plugins/crudgenerator

Is anyone else thinking along these same lines?

If so what does your setup look like?

Last edited by pimpmaster (2007-03-11 17:02:40)

Re: DRY and RESTful Controllers

I also found myself doing a simlar refactoring. I created a Resources controller (subclass of application controller) which the other CRUD based controllers could be a subclass of. This controller has a few before filters to handle permissions and set up the models. It worked marvolously. It made the app more secure and removed some duplication.

However, I caution you, be careful with this! It doesn't apply to every RESTful designed application. I consider it premature abstraction if it is applied too early. Instead, extract this behavior through refactoring when you see the duplication.

The reason being is because you can't tell how much behavior should be moved into this resources controller until the app is built. If you do it prematurely you run the risk of extracting too much of the behavior into the controller to the point where it becomes difficult to customize the behavior per controller and can lead to code which is worse than having the duplication.

I hope that makes sense.

Railscasts - Free Ruby on Rails Screencasts

Re: DRY and RESTful Controllers

That does make sense. I think my primary stumbling block with Rails is accepting its agility. I struggle so much to get things right the first time around. it's almost like I have refactorphobia.

I just need to accept the fact that my apps will never be perfect and months from now I will look at my code and marvel at my own ignorance.

Re: DRY and RESTful Controllers

I struggle with that too. It's hard to consistently refactor as you are coding.

One thing that really helps me is good test coverage. This way, when you refactor you instantly know whether or not you broke something. The refactoring I did previuosly with the Resources controller would be impossible without tests. This is because it involved so many controllers across the application and there's no way I could check everything after every little change. The tests reminded me when I was trying to refactor too much at one time.

Railscasts - Free Ruby on Rails Screencasts

Re: DRY and RESTful Controllers

As I build my first controllers I already see much duplication, especially when dealing with RESTful resources. I am considering utilizing this example as an application helper (the author seems to encourage it in application controller but Im not sure thats the best move)

http://geekonomics.blogspot.com/2006/07 … llers.html

Any thoughts?

Last edited by pimpmaster (2007-03-14 21:03:47)

Re: DRY and RESTful Controllers

Thanks for the link, pimpmaster.  I've been holding off my user admin pages because of duplicating all of those actions listed in that link.  Looks like a pretty good solution.

Re: DRY and RESTful Controllers

No problem BIGtrouble77

I'm actually surprised more people don't do this. Having 20 different controllers with the same 7 actions just seems obscene to me. (thats 140 repeats!)

As a side note, the code in that link is  little beyond my comprehension. I need to study it a bit more so I can bend it to a more RESTful standard. Unless somebody here has already done something similar for REST, this will become a pet project of mine.

Re: DRY and RESTful Controllers

UPDATE: This is becoming both frustrating and educational for me.

The way I see, it REST controllers should be pretty simple to refactor.. the CRUD is almost always identical and cases when it isn't can be easily overridden. You basically have the same exact methods/actions with the model name sprinkled throughout, so its really a matter of setting the model name dynamically. Easy right?

Ummm yeah... 

http://geekonomics.blogspot.com/2006/07 … llers.html

Picking apart the above, I see how the author has set his instance variables through the clever use of string interpolation.. ( for the clueless, that looks like #{this} ) Very impressive indeed.

Unfortunately, this code is not working for me at all. It fails to generate a RESTful route and when I make a form_for, the view bombs out on the first line (right at the point where the URL is set).

What is encouraging is that even a noob like myself can see the problem with this code. I mean, just look at this create action..

def create
instance_variable_set("@#{controller_name}", current_model.create(params_hash))
respond_to do |wants|
  wants.html {redirect_to :action=>:index}
end
end

This is where we leave the glorious land of REST and return to the days of yore, when actions were explictly programmed into redirects.. I mean it's great there is a respond_to block in there and all, but how is this even remotely RESTful if the routes and conditions are not specified? No wonder my view bombs! The action should look more like this (pardon my butchery):

def create
    @current_model = current_model.capitalize.new(params[:current_model])

    respond_to do |format|
      if @current_model.save
        flash[:notice] = '#{current_model.capitalize} was successfully created.'
        format.html { redirect_to current_model_url(@current_model) }
      else
        format.html { render :action => "new" }
      end
    end
  end


Of course this doesnt work either because I haven't yet learned how to parse method_names in different situations. Put an @ in front of a method_name and *POOF* it's an instance variable!

Argh... so much voodoo going on here!

I'm not letting this setback discourage me though. Sure it's holding my production up right now, but it's also an opportunity to learn how my code is working, which will just make me a better programmer at the end of the day.

PS- One thing I really can't figure out from the original code (or find much reference on) is that params_hash dealey..... what is going on there exactly??

Re: DRY and RESTful Controllers

I recommend extracting the "voodoo" into methods in the controller which you can use. For example:

# in controller
  def model_name
    params[:controller].singularize
  end

  def model_class
    model_name.camelize.constantize
  end

  def current_model
    instance_variable_get "@#{model_name}"
  end

  def current_model=(current_model)
    instance_variable_set "@#{model_name}", current_model
  end


The current_model getter and setter methods is where the magic is happening. This is using an instance variable with the same name as the model itself. For example,  in the Products controller, it will use an instance variable called @product to store the name of the model.

As for the redirects, it's probably best to not use named routes:

  def create
    self.current_model = model_class.new(params[model_name])
    respond_to do |format|
      if current_model.save
        flash[:notice] = "#{current_model.capitalize} was successfully created."
        format.html { redirect_to :action => 'show', :id => current_model }
      else
        format.html { render :action => "new" }
      end
    end
  end

Notice how the instance variable is never referred to directly, it always uses the current_model method.

pimpmaster wrote:

PS- One thing I really can't figure out from the original code (or find much reference on) is that params_hash dealey..... what is going on there exactly??

That's a custom method that is defined in the controller. I don't really think it's necessary since params[model_name] is just as easy.

Railscasts - Free Ruby on Rails Screencasts

Re: DRY and RESTful Controllers

And now for the lecture. wink

My theory is that there is real duplication, and then there's fake duplication. Just because the code looks the same, doesn't mean it's real duplication.

Instead of determining duplication on how the code looks line for line, base it on the behavior of the code and how changing it should effect the other "duplication". If changing one part of the duplication should change the others as well, then that is real duplication. If not, then I consider it fake duplication.

If you are building the site interface first (which I recommend) then there is likely not 100% real duplication across the controllers. First of all, you don't need all 7 CRUD actions for every controller. Some don't need an index, they just need the create/update/destroy actions. In the "index" and "show" actions you likely need to set up more instance variables than just the model. You may not want to always redirect to the same action (index/show) after creating/editing a page. You might want to change the flash message, or create two models at a time instead of one, etc.

In other words, you need flexibility in the controllers. If every controller action is exactly the same, ask yourself if you are building the site interface first. I'm finding controllers often have differences. This is the true reason why scaffolding and generators are bad. See the post by core member Jamis Buck Scaffolding's place for more info on this subject.

Yes, you can override the 7 CRUD actions in the subclasses, but you need to override the entire thing. But if you just want to change one little thing (say the flash message or redirect) then overriding the entire action becomes silly because you are back with the duplication that you tried so hard to avoid. You can apply some more refactorings to remove the duplication further, maybe add callbacks for determining the flash message and redirect location, but this will likely result in a big mess. Why? Because the duplication is fake, it is deceptive. You need more flexibility in the controller actions.

This is why I cautioned you in the beginning about how much you move into this abstract controller. IMO, abstracting all 7 actions is going too far. You may be wondering how my controller looks then? I'll show you:


# In an attempt to refactor the permission handling and fetching of models in most controller actions
# this abstract resources controller should be used as the superclass for all controllers that
# are managing a model with basic CRUD operations. The controller should have the plural name of the model.
class ResourcesController < ApplicationController
 
  before_filter :find_or_initialize, :only => [:show, :new, :create, :edit, :update, :destroy]
  before_filter :update_attributes, :only => [:create, :update]
  before_filter :authorize
 
  protected
   
    def find_or_initialize
      if params[:id].nil?
        self.current_model = model_class.new
      else
        self.current_model = model_class.find(params[:id])
      end
    end
 
    def update_attributes
      current_model.attributes = params[model_name]
    end
   
    def authorize
      unless authorized?
        flash[:error] = 'You are not authorized to complete that action'
        redirect_to logged_in? ? home_path : new_session_path
        false # return false so action isn't called
      end
    end
 
    def authorized?
      permission_actions.all? do |action|
        current_user.can?(action, model_name, current_model)
      end
    end
 
    def permission_actions
      case params[:action].to_sym
        when :index, :show  then [:view]
        when :new, :create  then [:view, :create]
        when :edit, :update then [:view, :update]
        when :destroy       then [:view, :destroy]
        else []
      end
    end
 
    def model_name
      params[:controller].singularize
    end

    def model_class
      model_name.camelize.constantize
    end

    def current_model
      instance_variable_get "@#{model_name}"
    end

    def current_model=(current_model)
      instance_variable_set "@#{model_name}", current_model
    end
 
end


Notice how every method in here is protected? There are no actions here, it is just before filters. Normally I would not perform this kind of abstraction, but the complex permission and authorization handling called for it. The model needs to be created before the authorization can take place because the user is asked if he has permission to alter it.

With this abstract controller, take a look at what a typical subclass looks like:

class UsersController < ResourcesController
  def index
    @users = User.find(:all)
  end
 
  def show
    @topics = @user.topics.find_ordered
  end
 
  # render new
 
  def create
    if @user.save
      session[:user_id] = @user.id
      flash[:notice] = "Successfully registered"
      redirect_to home_path
    else
      render :action => 'new'
    end
  end
 
  # render edit
 
  def update
    # RESEARCHME Why is "kind" protected from mass assignment?
    @user.kind = params[:user][:kind] unless params[:user][:kind].blank?
    if @user.save
      flash[:notice] = "Successfully updated profile"
      redirect_to user_path(@user)
    else
      render :action => 'edit'
    end
  end

end


In this case the @user instance variable is already set based on the before filters. All that needs to happen is the saving and setting of other attributes in the create/update actions. The flash message, redirect, etc. needs more flexibility (fake duplication) so that is why it is not extracted into the ResourcesController.

Hopefully this made some sense.

Railscasts - Free Ruby on Rails Screencasts

Re: DRY and RESTful Controllers

Hey all,

That is my blog over at http://geekonomics.blogspot.com and I just wanted to share some experience and thoughts on this topic since I have used the code and adapted to change over the past few months.

1) I found that the number one way I was using it was to bootstrap controllers during development.  Unless you have a really super simple system, your controllers are going to need to change and be unique over time.

2) Edge Rails and now 1.2 has had a better replacement for this type of bootstrapping for some time now.  script/generate scaffold_resource ...   This method usually saves more typing that CrudController ever will.

3) It can be dangerous!  What if you dont actually want a destroy action for one of your controllers?  You then need to remember to either overwrite this method or un-subclass that controller.

Jake

Re: DRY and RESTful Controllers

Actually, that makes a boatload of sense, ryan

Instead of a master class to lump all methods together, a more modular subclass of actions would assist me much more to control related behaviors. Thanks for giving us a peek in the toolbox. This stuff should be in a book (hint, hint)

Also many thanks to jakehow for dropping in and keeping my out of trouble smile

Re: DRY and RESTful Controllers

PS. I just learned what constantize does... coooool !

Re: DRY and RESTful Controllers

Okay, been really digging into your examples here and am pleasantly surprised how much I understand. The lights in my head are switching on!

All the same, you are using some curious goodies. One in particular is

find_or_initialize

This seems to be a recent addition to Rails and not much info is out there (AWDWR mentions it once and google aint much help either) From what I can gather, this acts as both a create and update method, but I am not sure how it is triggered.

Examples:

1. Will the URLs models/new and models/1;edit point to this action automatically?

2. In my views should I still use form_for(:model, :url => models_path)?

Another interesting bit right here:

:find_or_initialize, :only => [:show, :new, :create, :edit, :update, :destroy]

I assume you are filtering these actions through your method, or perhaps its the other way around, because I fail to see how your method relates to these other actions (especially destroy)

Sorry to pop so many questions, but I am really trying to understand the hows and whys instead of being a copy/paste loser.

Re: DRY and RESTful Controllers

"find_or_initialize" is a custom method that is defined in the controller:

    def find_or_initialize
      if params[:id].nil?
        self.current_model = model_class.new
      else
        self.current_model = model_class.find(params[:id])
      end
    end

This method is set up as a before_filter which is called before many different actions. This is so it sets up the instance variable before calling the primary action (show/new/create, etc.).

For example, when the "/users/create" action is called, the params[:id] will not be specified so it will make a new User model and and assign it to an instance variable of "@user".

When the "/users/edit/1" action is called the params[:id] is already established so it will find a user with id 1 and set it to the @user instance variable. This is so the actions in the controller subclasses don't have to set this variable. I also do this because I need to authorize the user later on, and it needs to have a model to determine if the user has permission to access it.

Railscasts - Free Ruby on Rails Screencasts

Re: DRY and RESTful Controllers

Wow.. I think my IQ just went up a couple of points.

Thanks for taking the time to lecture! smile

Re: DRY and RESTful Controllers

Ryan and pimpmaster, this is definitely a great discussion!

Ryan, I have the followings to clarify with you.  Thank you very much.

Q1:

redirect_to logged_in? ? home_path : new_session_path

I could not find 'logged_in?' method in Resources Controller.

Q2:

def authorized?
  permission_actions.all? do |action|
    current_user.can?(action, model_name, current_model)
  end
end

I can understand what 'permission_actions' method is doing.  However, what does the .all? (in permission_actions.all?) do?  Also, I could not find 'current_user.can?' in Resources Controller.

Q3:

Does Resources Controller have a corresponding resources database table?

Re: DRY and RESTful Controllers

weelkoh0 wrote:

I could not find 'logged_in?' method in Resources Controller.

This isn't shown. It's just a method in the ApplicationController which returns true/false depending on if the user is logged in or not.

weelkoh0 wrote:

I can understand what 'permission_actions' method is doing.  However, what does the .all? (in permission_actions.all?) do?  Also, I could not find 'current_user.can?' in Resources Controller.

The "all?" method is provided by Ruby on an enumerable. Basically it loops through each item (each permission action) and runs the code in the block for each one. It then returns true only if each loop returns true.

The current_user.can? method is a method in the User model which handles the permissions. Unfortunately it's too complicated for me to go into detail here.

weelkoh0 wrote:

Does Resources Controller have a corresponding resources database table?

Nope.

Railscasts - Free Ruby on Rails Screencasts

Re: DRY and RESTful Controllers

Revisiting this thread with an interesting plugin

http://soylentfoo.jnewland.com/articles … ontrollers

I just installed it and am poking around.. so far it seems pretty solid.

Curious what you guys think of this one

Re: DRY and RESTful Controllers

I would probably try make_resourceful over resource_this. It's been around longer and at first glance it looks easier to customize.

Railscasts - Free Ruby on Rails Screencasts