# Refactoring to RESTful

**URL:** <https://www.ruby-forum.com/t/refactoring-to-restful/226006>\
**Category:** Rails\
**Created:** [April 27, 2008, 1:04pm UTC](https://www.ruby-forum.com/t/refactoring-to-restful/226006 "2008-04-27T13:04:11Z")\
**Posts on this page:** 5\
**Page:** 1

<div class="post-metadata">

**Author:** ![Mister\_Blood\_R](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/m/7cd45c/32.png) [@Mister\_Blood\_R](https://www.ruby-forum.com/u/Mister_Blood_R)\
**Post date:** [April 27, 2008, 1:04pm UTC](https://www.ruby-forum.com/t/refactoring-to-restful/226006/1 "2008-04-27T13:04:11Z")

</div>

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](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](http://scottraymond.net/2006/7/20/refactoring-to-rest)
2. [http://www.therailsway.com/2007/2/13/signout-part-1](http://www.therailsway.com/2007/2/13/signout-part-1)
3. [Buckblog: Skinny Controller, Fat Model](http://weblog.jamisbuck.org/2006/10/18/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”, “  
”)  
@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

---

<div class="post-metadata">

**Author:** ![Mister\_Blood\_R](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/m/7cd45c/32.png) [@Mister\_Blood\_R](https://www.ruby-forum.com/u/Mister_Blood_R)\
**Post date:** [April 27, 2008, 4:46pm UTC](https://www.ruby-forum.com/t/refactoring-to-restful/226006/2 "2008-04-27T16:46:32Z")

</div>

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. [removed\_email\_address@domain.invalid](mailto:removed_email_address@domain.invalid) wrote:

> 2.[http://www.therailsway.com/2007/2/13/signout-part-1](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

---

<div class="post-metadata">

**Author:** ![Mister\_Blood\_R](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/m/7cd45c/32.png) [@Mister\_Blood\_R](https://www.ruby-forum.com/u/Mister_Blood_R)\
**Post date:** [April 27, 2008, 4:58pm UTC](https://www.ruby-forum.com/t/refactoring-to-restful/226006/3 "2008-04-27T16:58:59Z")

</div>

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

---

<div class="post-metadata">

**Author:** ![Mister\_Blood\_R](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/m/7cd45c/32.png) [@Mister\_Blood\_R](https://www.ruby-forum.com/u/Mister_Blood_R)\
**Post date:** [April 28, 2008, 7:10pm UTC](https://www.ruby-forum.com/t/refactoring-to-restful/226006/4 "2008-04-28T19:10:00Z")

</div>

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

---

<div class="post-metadata">

**Author:** ![Mister\_Blood\_R](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/m/7cd45c/32.png) [@Mister\_Blood\_R](https://www.ruby-forum.com/u/Mister_Blood_R)\
**Post date:** [May 6, 2008, 2:46pm UTC](https://www.ruby-forum.com/t/refactoring-to-restful/226006/5 "2008-05-06T14:46:00Z")

</div>

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