Refactored. Added specs for performance testing.

app/controllers/admin/file_controller.rb

@@ -2,8 +2,7 @@ class Admin::FileController < ApplicationController
   include DirectoryArray
 
   def index
-    @assets = Asset.find(:root)
-    @asset_lock = AssetLock.lock_version
+    @root_asset = Asset.root
   end
 
   def new
@@ -33,6 +32,9 @@ def new
 
   def remove
     @asset = Asset.find(params[:id], params[:v])
+    # TODO: Refactor @asset.pathname.nil? to make @asset invalid, so
+    #       the following line would read unless @asset.valid?
+    #       this would also clean up a lot of the specs (one would remove the reader for pathname)
     unless @asset.pathname.nil?
       if request.post?      
         if @asset.destroy

app/helpers/admin/file_helper.rb

@@ -3,8 +3,9 @@ module Admin::FileHelper
   include DirectoryArray
 
   def render_children(asset, locals = {})
-    @current_asset = asset
-    locals.reverse_merge!(:level => 0, :simple => false, :asset_lock => AssetLock.lock_version).merge!(:asset => asset)
+    locals.reverse_merge!(:level => 0, :simple => false).merge!(:asset => asset)
+    # TODO: should call render :partial => asset
+    # ADVANTAGE: will allow directories and files to have separate partials
     render :partial => 'children', :locals =>  locals
   end   
 
@@ -20,52 +21,48 @@ def expanded_rows_with_asset_lock
     @expanded_rows
   end
 
-  def expanded?
-    show_all? || (@current_asset.root? if @current_asset.respond_to?(:root?)) || expanded_rows_with_asset_lock.include?(@current_asset.id)
+  def expanded?(asset)
+    show_all? || (asset.root? if asset.respond_to?(:root?)) || expanded_rows_with_asset_lock.include?(asset.id)
   end
 
-  def icon_for
-    image_tag @current_asset.icon, :alt => '', :class => 'icon'
-  end
-
-  def asset_size
-    "#{@current_asset.pathname.size} Bytes" if @current_asset.is_a?(FileAsset)
+  def icon_for(asset)
+    image_tag asset.icon, :alt => '', :class => 'icon'
   end
 
-  def link_or_embed_field_for
-    return "" unless @current_asset.is_a?(FileAsset)
-    template_code = @current_asset.embed_tag
+  def link_or_embed_field_for(asset)
+    return "" unless asset.is_a?(FileAsset)
+    template_code = asset.embed_tag
     %Q{
       <input type="text" value="#{h(template_code)}"
        style="width: 100%"
        onclick="this.focus();this.select()" />
      }
   end    
 
-  def expander
-    return nil if (@current_asset.is_a?(FileAsset) or @current_asset.children.empty?)
-    image(expanded? ? "collapse" : "expand", 
+  def expander_for(asset)
+    return nil if (asset.is_a?(FileAsset) or asset.children.empty?)
+    image(expanded?(asset) ? "collapse" : "expand", 
           :class => "expander", :alt => 'toggle children', 
           :title => '')     
   end    
 
-  def spinner
+  def spinner_for(asset)
     image('spinner.gif', 
-            :class => 'busy', :id => "busy-#{@current_asset.id}", 
+            :class => 'busy', :id => "busy-#{asset.id}", 
             :alt => "",  :title => "", 
             :style => 'display: none;')
   end    
 
-  def link_to_new_file(asset_lock)
-    link_to image('add-child', :alt => 'add child'), new_file_path(:parent_id => @current_asset.id, :v => asset_lock)  if @current_asset.is_a?(DirectoryAsset)
+  def link_to_new_file(asset)
+    link_to image('add-child', :alt => 'add child'), new_file_path(:parent_id => asset.id, :v => asset.lock)  if asset.is_a?(DirectoryAsset)
   end
 
-  def link_to_remove_file(asset_lock)
-    link_to image('remove', :alt => 'remove page'), '/admin/files/remove?id=' + @current_asset.id.to_s + '&v=' + asset_lock.to_s unless (@current_asset.respond_to?(:root?) and @current_asset.root?)
+  def link_to_remove_file(asset)
+    link_to image('remove', :alt => 'remove page'), '/admin/files/remove?id=' + asset.id.to_s + '&v=' + asset.lock.to_s unless (asset.respond_to?(:root?) and asset.root?)
   end
 
-  def link_to_rename_file(asset_lock)
-     (@current_asset.respond_to?(:root?) and @current_asset.root?) ? @current_asset.pathname.basename : "<a href='/admin/files/edit?id=#{@current_asset.id}&v=#{asset_lock}'>#{@current_asset.pathname.basename}</a>"
+  def link_to_rename_file(asset)
+     (asset.respond_to?(:root?) and asset.root?) ? asset.basename : "<a href='/admin/files/edit?id=#{asset.id}&v=#{asset.lock}'>#{asset.basename}</a>"
   end
 
 end

app/models/asset.rb

@@ -37,19 +37,37 @@ def rename(asset)
         return false     
     end
   end
+
+  def size
+    @pathname.size
+  end
+
+  def basename
+    @pathname.basename
+  end
 
   def exists?
     @pathname.nil? ? false : true
   end
 
+  def lock
+    # TODO: Abstract away all lock versiony stuff.
+    # asset.version should probably be the only public interface
+    AssetLock.lock_version
+  end
+
   class << self
 
-   def find(*args)
+    def find(*args)
       case args.first
-        when :root then find_by_pathname(Pathname.new(absolute_path))
+        when :root then root
         else find_from_id(args[0], args[1])
       end
     end
+
+    def root
+      find_by_pathname(Pathname.new(absolute_path))
+    end
 
     def find_from_id(id, version)
       if AssetLock.confirm_lock(version) and !id.blank? 

app/models/directory_asset.rb

@@ -24,6 +24,10 @@ def save
     end
     @id
   end
+
+  def size
+    # Don't report size for directories, it would return bogus size info (the 'directory file' on disk)
+  end
 
   def destroy
       path = id2path(@id)

app/views/admin/file/_children.rhtml

@@ -1,21 +1,21 @@
 <tr class="level-<%= level -%> <%= asset.html_class -%> node" id = "page-<%= asset.id -%>" >   
   <td style="padding-left: <%= padding_left(level)-%>px;">
     <span class="w1">
-      <%= expander unless simple -%>
-      <%= icon_for -%>
-      <span class="title"><%= link_to_rename_file(asset_lock) -%></span>
-      <%= spinner -%>
+      <%= expander_for(asset) unless simple -%>
+      <%= icon_for(asset) -%>
+      <span class="title"><%= link_to_rename_file(asset) -%></span>
+      <%= spinner_for(asset) -%>
     </span>
   </td>
   <td class="type"><%= asset.description -%></td>   
   <% if !simple -%>
-    <td class="size"><%= asset_size -%></td>
-    <td class="embed"><%= link_or_embed_field_for -%></td>
-    <td class="add-child"><%= link_to_new_file(asset_lock) -%></td>
-    <td class="remove"><%= link_to_remove_file(asset_lock) -%></td> 
+    <td class="size"><%= number_to_human_size(asset.size) -%></td>
+    <td class="embed"><%= link_or_embed_field_for(asset) -%></td>
+    <td class="add-child"><%= link_to_new_file(asset) -%></td>
+    <td class="remove"><%= link_to_remove_file(asset) -%></td> 
   <% end %>
 </tr> 
-<% if expanded? and asset.is_a?(DirectoryAsset) -%>
+<% if expanded?(asset) and asset.is_a?(DirectoryAsset) -%>
   <% asset.children.each do |child| -%>
     <%= render_children child, :level => level + 1, :simple => simple -%>
   <% end -%>

app/views/admin/file/index.rhtml

@@ -12,8 +12,8 @@
     </tr>
   </thead>
   <tbody>
-<% if @assets -%>
-  <%= render_children(@assets) -%>
+<% if @root_asset -%>
+  <%= render_children @root_asset -%>
 <% else -%>
   <tr>
     <td colspan="6" class="note">No Files</td>
@@ -25,7 +25,7 @@
 <script>
 
 var auth_token = '<%=form_authenticity_token-%>';
-var asset_lock = '<%=@asset_lock-%>';
+var asset_lock = '<%=@root_asset.lock-%>';
 
 //over-riding the sitemap.js -> getBranch and saveExpandedCookie methods
 SiteMap.addMethods ({

lib/directory_array.rb

@@ -1,7 +1,7 @@
 module DirectoryArray
 
   def get_directory_array(path)
-    asset_array = Array.new
+    asset_array = []
     asset_absolute_path = Pathname.new(FileBrowserExtension.asset_path.to_s)
     path.children.collect do |child|
       unless hidden?(child)

spec/controllers/admin/file_controller_spec.rb

@@ -317,9 +317,26 @@ def remove_asset(assetname, version=current_version)
     end
 
   end
+
+  describe 'list' do
+    MAX_EMPTY_TIME = 0.05
+    it "should take < #{MAX_EMPTY_TIME} seconds to run on empty asset directory" do
+      time_taken_to{ get(:index) }.should <= MAX_EMPTY_TIME
+    end
+    # all this is doing is calling Asset.root , that should be fast
+    MAX_FULL_TIME = 0.1
+    it "should take < #{MAX_FULL_TIME} seconds to run on empty populated directory" do
+      begin
+        create_lots_of_files
+        time_taken_to{ get(:index) }.should <= MAX_FULL_TIME
+      ensure
+        delete_those_lots_of_files
+      end
+    end
+  end
 
   #####
-  #Both the below specs needs to be corrected
+  #TODO: Both the below specs needs to be corrected
   describe "managing AJAX request" do
 
     it "should render children via AJAX" do

spec/spec_helper.rb

@@ -38,3 +38,26 @@
   # If you declare global fixtures, be aware that they will be declared
   # for all of your examples, even those that don't use them.
 end
+
+
+def time_taken_to
+  t = Time.now
+  yield
+  Time.now - t
+end
+
+def create_lots_of_files
+  10.times do |i|
+    Dir.mkdir(File.join(FileBrowserExtension.asset_path,i.to_s))
+    10.times do |j|
+      Dir.mkdir(File.join(FileBrowserExtension.asset_path, i.to_s, j.to_s))
+      10.times do |k|
+        FileUtils.touch(File.join(FileBrowserExtension.asset_path, i.to_s, j.to_s, k.to_s))
+      end
+    end
+  end
+end
+
+def delete_those_lots_of_files
+  10.times {|i| FileUtils.rm_rf(File.join(FileBrowserExtension.asset_path,i.to_s))}
+end

spec/views/admin/file/index.rhtml_spec.rb

@@ -1,13 +1,28 @@
 require File.dirname(__FILE__) + '/../../../spec_helper'
 
 describe "/admin/file/index.rhtml" do
+  before(:each) do
+    assigns[:root_asset] = Asset.root
+    template.stub! :form_authenticity_token
+  end
+
+  def render_index
+    render "/admin/file/index.rhtml"
+  end
 
-    before(:each) do
-       @assets = Pathname.new(FileBrowserExtension.asset_path)
-    end
+  MAX_EMPTY_RENDER_TIME = 0.1
+  it "should render empty directory in less than #{MAX_EMPTY_RENDER_TIME} seconds" do
+    time_taken_to{ render_index }.should <= MAX_EMPTY_RENDER_TIME
+  end
 
-    it "should render list of assets" do
-       render "/admin/file/index.rhtml"
+  # The first level nodes start collapsed, so it shouldn't take long
+  MAX_RENDER_TIME = 1.0
+  it "should render populated directory in less than #{MAX_RENDER_TIME} seconds" do    
+    begin
+      create_lots_of_files
+      time_taken_to{ render_index }.should <= MAX_RENDER_TIME
+    ensure
+      delete_those_lots_of_files
     end
-
+  end
 end