How do I make this controller RESTful?

rubyonrails-talk
1

Hello,

I'm trying to make this controller RESTful, but its post method uses two other methods -- select_category and show_form -- see them in action at http://demo.chuckslist.org/ads/post. How do I reduce all of this to just one method? Other advice on making this controller as RESTful and beautiful as possible would ofcourse be much obliged. Here's the controller:

class AdsController < ApplicationController   def show     @ad = Ad.find_by_id(params[:id])     if (@ad.nil?)       flash[:warning] = "ad doesn't exist"       redirect_to root_path     end   end   def destroy     if request.post?       @ad = Ad.find_by_activation_code(params[:id])       if (@ad.nil?)         flash[:warning] = "ad doesn't exist"         redirect_to root_path       else         @ad.destroy         flash[:notice] = "ad removed"         redirect_to root_path       end     end   end   def list     @category = Category.find_by_slug(params[:slug])     if @category       @ads = @category.ads.all_active       @requested_category = @category.name + ' in ' + @category.parent_category.name     else       @category = ParentCategory.find_by_slug(params[:slug])       if @category         @ads = @category.all_ads         @requested_category = @category.name       else         flash[:warning] = "invalid request"         redirect_to root_path       end     end   end   def post     @parents = ParentCategory.find :all, :order => 'name ASC'   end   def select_category     @parent_category = ParentCategory.find_by_id(params[:id])   end   def show_form     @category = Category.find_by_id(params[:id])   end   def new     if (params[:email] != params[:email_confirmation])       flash[:warning] = "e-mails don't match"       redirect_to :controller => 'ads', :action => 'post'     else       @author = Author.find_by_email(params[:email])       if @author.blank?         @author = Author.new         @author.email = params[:email]         @author.ip = request.env['REMOTE_ADDR']         @author.save       end       @ad = Category.find_by_id(params[:category]).ads.new       @ad.title = params[:title]       @ad.ad = params[:ad].gsub("\n", "<br />")       @ad.expiration = Time.now + 30.days       @ad.author = @author       @ad.author_ip = request.env['REMOTE_ADDR']       @ad.save       @ad.handle_images(params["image_attachments"])       Mail.deliver_activation(@ad, @author.email)       flash[:notice] = "ad pending activation"     end   end   def activate_ad     @ad = Ad.find_by_activation_code(params[:activation_code])     if (@ad.nil?)       flash[:warning] = "error activating ad"       redirect_to root_path     else       if @ad.activate_ad(params[:activation_code])         flash[:notice] = "ad activated"         Mail.deliver_activated(@ad, @ad.author.email)         redirect_to :action => 'edit', :activation_code => @ad.activation_code       else         flash[:warning] = "error activating ad"         redirect_to root_path       end     end   end   def manage     @ad = Ad.find_by_activation_code(params[:activation_code])     if (@ad.nil?)       flash[:warning] = "ad doesn't exist"       redirect_to root_path     else     end   end   def edit     @ad = Ad.find_by_activation_code(params[:activation_code])     if (@ad.nil?)       flash[:warning] = "ad doesn't exist"       redirect_to root_path     else     end   end   def update     @ad = Ad.find_by_activation_code(params[:activation_code])     if (@ad.nil?)       flash[:warning] = "ad doesn't exist"       redirect_to root_path     else       @ad.ad = params[:ad].gsub("\n", "<br />")       @ad.title = params[:title]       if @ad.save         @ad.handle_images(params["image_attachments"])         flash[:notice] = "ad updated"       else         flash[:warning] = "error updating ad"       end       redirect_to :controller => 'ads', :action => 'manage', :activation_code => @ad.activation_code     end   end   def feed     @ads = Ad.all_active     respond_to do |format|       format.rss { render :layout => false }       format.atom # index.atom.builder     end   end end

Take care!

-- Rubienubie

2

Can somebody please help me?

So far I've moved show_category and edit_form into my ad model, and edited ads_controller.rb to look like this:

class AdsController < ApplicationController   def index     @category = Category.find_by_slug(params[:slug])     if @category       @ads = @category.ads.all_active       @requested_category = @category.name + ' in ' + @category.parent_category.name     else       @category = ParentCategory.find_by_slug(params[:slug])       if @category         @ads = @category.all_ads         @requested_category = @category.name       else         flash[:warning] = "invalid request"         redirect_to root_path       end     end   end   def show     @ad = Ad.find_by_id(params[:id])     if (@ad.nil?)       flash[:warning] = "ad doesn't exist"       redirect_to root_path     end   end   def create     @parents = ParentCategory.find :all, :order => 'name ASC'   end   def new     if (params[:email] != params[:email_confirmation])       flash[:warning] = "e-mails don't match"       redirect_to create_ads_path     else       @author = Author.find_by_email(params[:email])       if @author.blank?         @author = Author.new         @author.email = params[:email]         @author.ip = request.env['REMOTE_ADDR']         @author.save       end       @ad = Category.find_by_id(params[:category]).ads.new       @ad.title = params[:title]       @ad.ad = params[:ad].gsub("\n", "<br />")       @ad.expiration = Time.now + 30.days       @ad.author = @author       @ad.author_ip = request.env['REMOTE_ADDR']       @ad.save       @ad.handle_images(params["image_attachments"])       Mail.deliver_activation(@ad, @author.email)       flash[:notice] = "ad pending activation"     end   end   def edit     @ad = Ad.find_by_activation_code(params[:activation_code])     if (@ad.nil?)       flash[:warning] = "ad doesn't exist"       redirect_to root_path     else     end   end   def update     @ad = Ad.find_by_activation_code(params[:activation_code])     if (@ad.nil?)       flash[:warning] = "ad doesn't exist"       redirect_to root_path     else       @ad.ad = params[:ad].gsub("\n", "<br />")       @ad.title = params[:title]       if @ad.save         @ad.handle_images(params["image_attachments"])         flash[:notice] = "ad updated"       else         flash[:warning] = "error updating ad"       end       redirect_to edit_ads_path, :activation_code => @ad.activation_code     end   end   def destroy     if request.post?       @ad = Ad.find_by_activation_code(params[:id])       if (@ad.nil?)         flash[:warning] = "ad doesn't exist"         redirect_to root_path       else         @ad.destroy         flash[:notice] = "ad removed"         redirect_to root_path       end     end   end   def feed     @ads = Ad.all_active     respond_to do |format|       format.rss { render :layout => false }       format.atom # index.atom.builder     end   end end

3

I guess I'm on my own with this one.

4

Rubienubie wrote:

I guess I'm on my own with this one.

sorry, but you must understand, that most people answering here are professional programmers with something like a working day. we jump in here once in a while and answer what we can. this may depend on our knowledge and on the question. not so many people will have the time to analyze that much code. especially not during the week. (so since it's friday today tour chances are, somebody will do during weekend. otherwise keep your questions as short as possible and don't expect people to jump for cosmetical cleanup stuff.

5

@Thorsten: If you feel that way then just ignore it and move on. No reason to get ruffled by someone you'll probably never meet. Moreover, the community as a whole is more likely to grow and evolve if we don't eat our young. :slight_smile:

@rubienubie: There seems to be a fundamental misunderstanding about REST at play here. Your first post in this thread suggests that you are worried about having more than one method that responds to an HTTP post but that is not a limiting factor. For simplicity we'll say that a resource is an object that responds on a particular address. It's response can be tailored based on two things: actions and the http verbs associated with those actions.

So, for your AdsController you're actually dealing with _two_ resources. One resource is the collection of ads and the other is an individual ad (really, many of these but one pattern to code). For the collection, then:

.../ads #address of the ads collection

can respond in different ways based on the HTTP verbs. Without qualification a GET maps to the index and a POST maps to create.

Similarly,

.../ads/1 # address of an individual ad

can respond in different ways. A PUT will #update it and a DELETE will #destroy it while a GET will #show it.

But you also get another degree of movement when you add actions. For example,

.../ads/1/do_something

Could add as many as four different responses based on whether you do a GET, POST, PUT, or DELETE to that address. You'd map these in routes.rb like so:

map.resources :ads, :collection=>{:do_something_else=>_verb or any_}, :member=>{:do_something=>_verb or any_}

So technically you could keep your controller very much the way it was and make it "RESTful". Your refactoring is moving in a cleaner direction, though. One thing I'd add is to consider the "fat model, skinny controller" suggestion: http://www.therailsway.com/2007/6/1/railsconf-recap-skinny-controllers. You've really got a lot of business logic tied up in your controllers that should not be there.

6

@Andy: Oops, i hope nobody misunderstood my comment. it was not the intention to keep people from asking whatever they want or how long they want. i just wanted to give a tip, that clear and compact questions may get answered faster and others like this may take some time. (i think i answered quite some questions about "best practices" or more complicated issues myself.)

i didn't want to criticize Rubienubie or the question.