Refactoring to RESTful

My name is Kyrre and I'm a 17-year-old web design student from Norway. I've only been doing Rails for about a month now, and because of my lack of mastery, I could sure use your help achieving my goals.

This is my remake of http://pastie.org/187538. My goals are to make it RESTful (1, 2), move more logic into the models (3) and generally clean things up. How am I doing so far?

1. http://scottraymond.net/2006/7/20/refactoring-to-rest 2. http://www.therailsway.com/2007/2/13/signout-part-1 3. Buckblog: Skinny Controller, Fat Model

Content:

config/routes.rb controllers/ads_controller.rb controllers/ads_activate_controller.rb controllers/categories_controller.rb controllers/parent_categories_controller.rb models/ad.rb models/category.rb models/parent_category.rb

That looks good to me, especially for someone with a month of Rails

I have a question about the nesting of posts under users:

map.resources :users, :member => { :suspend => :put, :settings => :get, :unsuspend => :put, :purge => :delete }, :has_many => [:posts]

You need a route that looks like users/3/posts/16 does :has_many =>[:posts] give you that? or do you need to nest it

map.resources :users, :member => { :suspend => :put, :settings => :get, :unsuspend => :put, :purge => :delete } do |users|    users.resources :post end

perhaps someone could chime in here.

what is the output of "rake routes" ?

My name is Kyrre and I'm a 17-year-old web design student from Norway. I've only been doing Rails for about a month now, and because of my lack of mastery, I could sure use your help achieving my goals.

This is my remake ofhttp://pastie.org/187538. My goals are to make it RESTful (1, 2), move more logic into the models (3) and generally clean things up. How am I doing so far?

1.http://scottraymond.net/2006/7/20/refactoring-to-rest 2.http://www.therailsway.com/2007/2/13/signout-part-1 3.Buckblog: Skinny Controller, Fat Model

Content:

config/routes.rb controllers/ads_controller.rb controllers/ads_activate_controller.rb controllers/categories_controller.rb controllers/parent_categories_controller.rb models/ad.rb models/category.rb models/parent_category.rb

--

config/routes.rb

ActionController::Routing::Routes.draw do |map|   map.resources :sites   map.resources :ads, :collection => { :feed => :get }, :member => { :ad_activate => :put }   map.resources :users, :member => { :suspend => :put, :settings => :get, :unsuspend => :put, :purge => :delete }, :has_many => [:posts]   map.activate '/activate/:user_activation_code', :controller => 'users', :action => 'user_activate', :user_activation_code => nil   map.signup '/join', :controller => 'users', :action => 'new'   map.login '/login', :controller => 'sessions', :action => 'new'   map.logout '/logout', :controller => 'sessions', :action => 'destroy'   map.settings '/settings', :controller => 'users', :action => 'settings'   map.resource :session   map.resource :pages   map.slug ':slug', :controller => 'ads', :action => 'list'   map.root :controller => 'main', :action => 'index'   map.connect ':controller/:action/:id'   map.connect ':controller/:action/:id.:format' end

--

controllers/ads_controller.rb

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 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.images(params["image_attachments"])       Mail.deliver_activation(@ad, @author.email)       flash[:notice] = "ad pending activation"     end   end   def create     @parents = ParentCategory.find :all, :order => 'name ASC'   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.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     @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   def feed     @ads = Ad.all_active     respond_to do |format|       format.rss { render :layout => false }       format.atom # index.atom.builder     end   end end

--

controllers/ads_activate_controller.rb

class AdsActivateController < ApplicationController   def create     @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 end

--

controllers/categories_controller.rb

class CategoriesController < ApplicationController   def index     @display_data = Category.display_paged_data(params[:page])     @parents = ParentCategory.find(:all)     @parent_categories = @parents.collect { |p| [p.name, p.id] }   end   def create     @category = Category.new     @category.name = params[:new_category]     @category.slug = params[:new_category].split.join.downcase     @category.parent_category_id = params[:ParentCategory][:id]     @category.save!     flash[:notice] = "new category created"     redirect_to :action => 'category'   end   def update     @category = Category.find_by_id(params[:id])     if @category.nil?       flash[:warning] = "invalid category"     end     if @category.update_attributes(params[:category])       flash[:notice] = "category updated"     else       if params[:category].nil?         flash[:warning] = "nothing to do"       else         flash[:warning] = "error updating category"       end     end     redirect_to :action => 'category'   end   def destroy     category = Category.find(params[:id])     category.destroy     flash[:notice] = "category deleted"     redirect_to :action => 'category'   end end

--

controllers/parent_categories_controller.rb

class ParentCategoriesController < ApplicationController   def show     @display_data = ParentCategory.display_paged_data(params[:page])   end   def create     @parent = ParentCategory.new     @parent.name = params[:parent_category]     @parent.slug = params[:parent_category].split.join.downcase     @parent.save!     flash[:notice] = "parent category created"     redirect_to :action => 'parent_category'   end   def update     @parent_category = ParentCategory.find_by_id(params[:id])     if @parent_category.nil?       flash[:warning] = "invalid parent category"       redirect_to :action => 'parent_category'     end     if @parent_category.update_attributes(params[:parent_category])       flash[:notice] = "parent category updated"     else       if params[:parent_category].nil?         flash[:warning] = "nothing to do"       else         flash[:warning] = "error updating parent category"       end     end     redirect_to :action => 'parent_category'   end   def destroy     parent = ParentCategory.find(params[:id])     parent.destroy     flash[:notice] = "parent category deleted"     redirect_to :action => 'parent_category'   end end

--

models/ad.rb

require 'uuid' class Ad < ActiveRecord::Base   belongs_to :category   belongs_to :author   validate :expiration_is_set   def expiration_is_set     if expiration.nil?       expiration = Time.now + 30.days     end   end   def expire!     self.expiration = Time.now - 1.day     self.save   end   def extend!     self.expiration = Time.now + 30.days     self.save   end   def reset!     self.active = false     self.save   end   def expired?     self.expiration < Time.now   end   def self.all_active     find(:all, :conditions => ['expiration > ? and active = ?', Time.now, true], :order => 'created_at 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 images(attachments)     return unless attachments.respond_to? :each     attachments.each do |image|       attachment = self.attachments.build(:uploaded_data => image)       attachment.save     end   end   def activate_ad(conf)     if self.activation_code == conf and !self.active?       self.active = true       if self.save         return true       else         return false       end     else       return false     end   end protected def before_create     self.activation_code = UUID.new     self.email = DateTime.now.to_s(:generate_hash) + $generatedKeyCount.to_s.rjust(5, '0') + "@" + DOMAIN     $generatedKeyCount = $generatedKeyCount + 1     if ($generatedKeyCount > 99999)       $generatedKeyCount = 1     end end end

--

models/category.rb

class Category < ActiveRecord::Base   has_many :ads   belongs_to :parent_category   validates_presence_of :name   validates_uniqueness_of :name, :case_sensitive => false   validates_length_of :name, :within => 4..40   validates_presence_of :slug   validates_uniqueness_of :slug, :case_sensitive => false   validates_length_of :slug, :within => 3..40 end

--

models/parent_category.rb

class ParentCategory < ActiveRecord::Base   has_many :categories do

def in_order

I guess posts should have been plural

map.resources :users, :member => { :suspend => :put, :settings => :get, :unsuspend => :put, :purge => :delete } do |users|    users.resources :posts # posts, not post end

@Ruby Freak: Yes, the has_many macro is the same as the nesting that you mention.

@Mister Blood Rails: You're off to a very good start here. In the AdsController it looks like you've got the 'new' and 'create' methods confused (in default Rails REST new gives you the form to fill out and create persists the data). Otherwise, the controllers look pretty good.

In moving to the 'fat model, skinny controller' concept, the blocks of code that have @some_instance_method on several consecutive lines are the ideal candidates to move into a model method.

A couple of other observations/suggestions: - In your current AdsController#new, the block that sets the @author could be reduced to:   @author = Author.find_or_create_by_email params[:email]   @author.update_attribute :ip, request.env['REMOTE_ADDR'] if @author.ip.blank?

- Look into using 'form_for'. This will allow you to 'scope' your input fields in such a way as to make create/new/update_attributes much simpler. These ActiveRecord::Base methods all accept a hash that works with the params hash provided by the controller. For example, your ads/new.html.erb could look something like:

<% form_for :ad, :url=>ads_path do |f| %> <%= label :title, 'Title' %> <%= f.text_field :title %> <%= label :title, 'Ad' %> <%= f.text_field :ad %> ... <% end %>

Then the code to create the ad would be simpler: @ad = Ad.new params[:ad].merge(:author=>@author, :author_id=> request.env['REMOTE_ADDR']) if @ad.save ...

Thanks a lot guys! Wonderful, wonderful advice! It's looking better and better every day now! I'll show you what I got ASAP...