Refactoring the Web Model (From James Herdman)

A bit of refactoring, via 

  http://github.com/jherdman/instiki/tree/master

app/models/web.rb

@@ -1,7 +1,27 @@
 class Web < ActiveRecord::Base
-  has_many :pages, :dependent => :destroy
+  ## Associations
+
+  has_many :pages,      :dependent => :destroy
   has_many :wiki_files, :dependent => :destroy
 
+  has_many :revisions,  :through => :pages
+
+  ## Hooks
+
+  before_save :sanitize_markup
+  after_save :create_files_directory
+
+  before_validation :validate_address
+
+  ## Validations
+
+  validates_uniqueness_of :address
+
+  validates_length_of :color, :in => 3..6
+
+  ## Methods
+
+  # @return [Wiki] a new Wiki instance
   def wiki
     Wiki.new
   end
@@ -13,34 +33,40 @@ class Web < ActiveRecord::Base
   end
 
   def add_page(name, content, time, author, renderer)
-    page = page(name) || Page.new(:web => self, :name => name)
+    page = page(name) || pages.build(:name => name)
     page.revise(content, name, time, author, renderer)
   end
 
+  # @return [Array<String>] a collection of all the names of the authors that 
+  #   have ever contributed to the pages for this Web
   def authors
-    connection.select_all(
-        'SELECT DISTINCT r.author AS author ' + 
-        'FROM revisions r ' +
-        'JOIN pages p ON p.id = r.page_id ' +
-        'WHERE p.web_id = ' + self.id.to_s +
-        ' ORDER by 1 '
-        ).collect { |row| row['author'] }        
+    revisions.all(
+      :select => "DISTINCT revisions.author",
+      :order  => "1"
+    ).collect(&:author)
   end
 
   def categories
     select.map { |page| page.categories }.flatten.uniq.sort
   end
 
+  # @param [String] name the name of some associated Page record to find
+  # @return [Page, nil] the associated Page record, or +nil+ if no record is 
+  #   found with the provided name
   def page(name)
-    pages.first(:conditions => ['name = ?', name])
+    pages.find_by_name(name)
   end
 
+  # @return [Page] the last associated Page record
   def last_page
-    return Page.first(:order => 'id desc', :conditions => ['web_id = ?', self.id])
+    pages.last
   end
 
+  # @param [String] name the name of some potential Page record
+  # @return [Boolean] whether or not a given Page record exists with a given 
+  #   name
   def has_page?(name)
-    Page.count(:conditions => ['web_id = ? AND name = ?', id, name]) > 0
+    pages.exists?(:name => name)
   end
 
   def has_redirect_for?(name)
@@ -52,11 +78,11 @@ class Web < ActiveRecord::Base
   end
 
   def has_file?(file_name)
-    WikiFile.find_by_file_name(file_name) != nil
+    wiki_files.exists?(:file_name => file_name)
   end
 
-  def file_list(sort_order = 'file_name')
-    WikiFile.all(:order => sort_order, :conditions => ['web_id = ?', id])
+  def file_list(sort_order="file_name")
+    wiki_files.all(:order => sort_order)
   end
 
   def pages_that_link_to(page_name)
@@ -67,30 +93,34 @@ class Web < ActiveRecord::Base
     WikiReference.pages_that_link_to_file(self, file_name)
   end
 
+  # @param [String] file_name the name of some WikiFile of interest
+  # @return [String, nil] the description of some WikiFile of interest, nil if 
+  #   the WikiFile could not be found
   def description(file_name)
-    file = WikiFile.find_by_file_name(file_name)
-    file.description if file
+    wiki_files.find_by_file_name(file_name).try(:description)
   end
 
+  # @return [Symbol] the type of markup used by this Web
   def markup
-    read_attribute('markup').to_sym
+    self[:markup].to_sym
   end
 
+  # @return [Hash] a Hash wherein the key is some author's name, and the 
+  #   values are an array of page names for that author.
   def page_names_by_author
-    connection.select_all(
-        'SELECT DISTINCT r.author AS author, p.name AS page_name ' +
-        'FROM revisions r ' +
-        'JOIN pages p ON r.page_id = p.id ' +
-        "WHERE p.web_id = #{self.id} " +
-        'ORDER by p.name'
-    ).inject({}) { |result, row|
-        author, page_name = row['author'], row['page_name']
-        result[author] = [] unless result.has_key?(author)
-        result[author] << page_name
-        result
-    }
+    data = revisions.all(
+      :select => "DISTINCT revisions.author AS author, pages.name AS page_name",
+      :order  => "pages.name"
+    )
+
+    data.inject({}) do |result, revision|
+      result[revision.author] ||= []
+      result[revision.author] <<  revision.page_name
+      result
+    end
   end
 
+  # OPTIMIZE Use the +delete_all+ with conditions for extra efficiency
   def remove_pages(pages_to_be_removed)
     pages_to_be_removed.each { |p| p.destroy }
   end
@@ -107,28 +137,39 @@ class Web < ActiveRecord::Base
     PageSet.new(self, pages, nil)
   end
 
+  # @return [String] uses the +address+ attribute for this record's parameter name
   def to_param
     address
   end
 
+  # Called by an +after_save+ hook. Creates the directory that houses this 
+  # Web's associated files.
+  #
+  # TODO Move this into the WikiFile model
   def create_files_directory
     return unless allow_uploads == 1
-    dummy_file = self.wiki_files.build(:file_name => '0', :description => '0', :content => '0')
+
+    dummy_file = wiki_files.build(
+      :file_name   => "0",
+      :description => "0",
+      :content     => "0"
+    )
+
     File.umask(0002)
-    dir = File.dirname(dummy_file.content_path)
+
     begin
-      require 'fileutils'
-      FileUtils.mkdir_p dir
+      dummy_file.content_path.parent.mkpath
       dummy_file.save
       dummy_file.destroy
     rescue => e
-      logger.error("Failed create files directory for #{self.address}: #{e}")
+      logger.error "Failed create files directory for #{address}: #{e}"
       raise "Instiki could not create directory to store uploaded files. " +
             "Please make sure that Instiki is allowed to create directory " +
-            "#{File.expand_path(dir)} and add files to it."
+            "#{dummy_file.content_path.expand_path} and add files to it."
     end
   end
 
+  # @return [Pathname] the path to the files for this record
   def files_path
     path = Rails.root.join("webs")
     if default_web?
@@ -138,6 +179,7 @@ class Web < ActiveRecord::Base
     end
   end
 
+  # @return [Pathname] the path to PNGs for this record
   def blahtex_pngs_path
     files_path.join("pngs")
   end
@@ -155,11 +197,6 @@ class Web < ActiveRecord::Base
     end
 
   protected
-    before_save :sanitize_markup
-    after_save :create_files_directory
-    before_validation :validate_address
-    validates_uniqueness_of :address
-    validates_length_of :color, :in => 3..6
 
     def sanitize_markup
       self.markup = markup.to_s
@@ -172,7 +209,8 @@ class Web < ActiveRecord::Base
       end
     end
 
+    # @return [Boolean] whether or not this record is considered the default Web
     def default_web?
-      defined? DEFAULT_WEB and self.address == DEFAULT_WEB
+      defined?(DEFAULT_WEB) && address == DEFAULT_WEB
     end
 end

test/unit/web_test.rb

@@ -1,5 +1,4 @@
 require File.dirname(__FILE__) + '/../test_helper'
-#require File.expand_path(File.dirname(__FILE__) + '/../test_helper')
 
 class WebTest < ActiveSupport::TestCase
   fixtures :system, :webs, :pages, :revisions, :wiki_references