How do I make this controller RESTful?

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

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

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

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.

@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.

@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.