Refactoring to RESTful

Mister_Blood_R · April 27, 2008, 1:04pm

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?

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”, “
”)
@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”, “
”)
@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
find(:all, :order => ‘name ASC’)
end
end
def all_ads
@subcategories = self.categories
@results = []
@subcategories.each { |c| c.ads.all_active.each { |a| @results <<
a } }
return @results
end
end

Mister_Blood_R · April 27, 2008, 4:46pm

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” ?

On Apr 27, 4:03 am, Mister Blood R. [email protected] wrote:

2.http://www.therailsway.com/2007/2/13/signout-part-1
models/category.rb
map.resources :users, :member => { :suspend => :put, :settings
map.resource :session
    @ads = @category.all_ads
  flash[:warning] = "ad doesn't exist"
    @author = Author.new
  @ad.save
if (@ad.nil?)
else
end
end
–
if @ad.activate_ad(params[:activation_code])
end
end
@category = Category.find_by_id(params[:id])
end
@parent.name = params[:parent_category]
end
end
models/ad.rb
end
self.save
end
def activate_ad(conf)
end
end
validates_length_of :name, :within => 4…40
has_many :categories do

def in_order

Mister_Blood_R · April 27, 2008, 4:58pm

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

Mister_Blood_R · April 28, 2008, 7:10pm

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

@Mister Blood R.:
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
…

Mister_Blood_R · May 6, 2008, 2:46pm

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