refactor according "rails best practices"

used "rails best practices " today. details see:

http://github.com/flyerhzm/rails_best_practices

 

 

below are the refactor advices for my app :

./config/routes.rb:58 - not use default route
./config/routes.rb:59 - not use default route
./app/sweepers/backup_sweeper.rb:4 - law of demeter
./app/controllers/admin/self_study/page_groups_controller.rb:33 - move model logic into model (page_group called_count > 4)
./app/controllers/admin/self_study/page_parts_controller.rb:3 - move model logic into model (@page_part called_count > 4)
./db/migrate/20090108084125_create_scenarios.rb:2 - always add db index (task_cards => trainer_task_card_id)
........................400+ index adding........................
./db/migrate/20091105042309_add_extra_type_to_page_group.rb:2 - always add db index (headings => prompt_page_id)
./app/models/page.rb:212 - keep finders on their own model
./app/models/page.rb:234 - keep finders on their own model
./app/models/page_group.rb:111 - keep finders on their own model
./app/models/page_group.rb:115 - keep finders on their own model
./app/controllers/admin/self_study/page_group_pages_controller.rb:21 - move finder to named_scope
./app/controllers/admin/self_study/page_group_pages_controller.rb:34 - move finder to named_scope
./app/controllers/admin/self_study/page_statistics_controller.rb:16 - move finder to named_scope

Found 462 errors.

=========================================

Below are some refactors I made according the advices, I think most advices are valuable.

 

 

refactor on "page_group called_count > 4"

old code

def expire_cache_of(page_group)
  case page_group.class.to_s
  when "Preparation"
    expire_action(:controller => "/flex/preparations/#{page_group.id}")
  when "PublicContent"
    expire_action(:controller => "/flex/public_contents/#{page_group.id}")
  when "ExtraContent"
    page_group.pages.each do |page|
      expire_action(:controller => "/flex/extras/#{page.id}")
    end
  when "TaskCard"
    page_group.pages.each do |page|
      expire_action(:controller => "/flex/task_cards/#{page.id}")
    end
  end
end

 

new code

def expire_cache_of(page_group)
  expire_action(:controller => "/flex/#{page_group.class_name.tableize}/#{page_group.id}")
  page_group.pages.map(&:id).each { |id| expire_action(:controller => "/flex/task_cards/#{id}") }
 if page_group.class == TaskCard # not finished
end

 

 

refactor on "@page_part called_count > 4"

old code

in controller

def set_content
  @page_part = PagePart.find_by_id(params[:page_part])
  value = @page_part.part_value
  @page_part.update_attributes(:part_value => params[:value])
  @page_part.page.page_groups.each do |page_group|
    page_group.page_group_changes << PageGroupChange.create(:user => current_user.id,
:action => "<b>updated</b> page '#{@page_part.page.title}' part #{@page_part.part_key}'s
value form '#{value}' to '#{@page_part.part_value}'")
  end
  render :text => @page_part.part_value.format_for_html
end

 

new code

in controller

def set_content
  @page_part = PagePart.find_by_id(params[:page_part])
  @page_part.set_value(params[:value], current_user.id)
  render :text => @page_part.part_value.format_for_html
end

 

in model

def set_value(value, user)
  old_value = self.part_value
  self.update_attributes(:part_value => value)
  self.page.page_groups.each do |page_group|
    page_group.page_group_changes << PageGroupChange.create(:user => user,
:action => "<b>updated</b> page '#{self.page.title}' part #{self.part_key}'s value form
'#{old_value}' to '#{self.part_value}'")
  end
end

 

test code

describe "set_value(value, user)" do
  it "should sent part value well" do
    @page_group = PageGroup.create
    @page = @page_group.pages.create(:title => "test set_value")
    @page_part = @page.page_parts.create(:part_key => "key", :part_value => "old_value")
    @page_part.set_value("new_value", 1)
    @page_part.part_value.should == "new_value"
    @page_group.page_group_changes.first.user.should == 1
    @page_group.page_group_changes.first.action.should == "<b>updated</b> page
'test set_value' part key's value form 'old_value' to 'new_value'"
  end
end

 

refacor on "keep finders on their own model"

 

old code

def last_published_at
  if last_published = self.page_group_changes.find(:last,
:conditions => ["action=?", "<b>published</b>"])
    last_published_at = last_published.created_at
  else
    last_published_at = ""
  end
end

 

new code

in page_group_change model:
named_scope :published_changes, :conditions => { :action => "<b>published</b>" }

def last_published_at
  self.page_group_changes.published_changes.last.try(:created_at) || ""
end

 
old code

def unpublished_changes
  return self.page_group_changes.find(:all, :conditions => ["created_at > ?",
last_published_at], :order => "created_at DESC").map(&:description).join("<br/>")
end

 

new code

in page_group_change model:
named_scope :unpublished_changes_from, lambda { |time| { :conditions => [ "created_at > ?", time], :order => "created_at DESC"} }

def unpublished_changes
  page_group_changes.unpublished_changes_from(last_published_at).map(&:description).join("<br/>")
end