use methods with memoization instead of temporary instance vars

Sergey Nartimov
1 parent c00f1ebe
Showing 5 changed files with 13 additions and 25 deletions Show diff stats
app/controllers/apps_controller.rb
app/models/app.rb
app/views/apps/index.html.haml
spec/controllers/apps_controller_spec.rb
spec/views/apps/index.html.haml_spec.rb
@@ -52,20 +52,13 @@ class AppsController &lt; InheritedResources::Base
  
   protected
     def collection
-      @unresolved_counts, @problem_counts = {}, {}
       @apps ||= begin
         apps = end_of_association_chain.all
  
-        # Cache counts for unresolved errs and problems
-        apps.each do |app|
-          @unresolved_counts[app.id] ||= app.problems.unresolved.count
-          @problem_counts[app.id]    ||= app.problems.count
-        end
-
         # Sort apps by number of unresolved errs, then problem counts.
-        apps.sort do |a,b|
-          (@unresolved_counts[b.id] <=> @unresolved_counts[a.id]).nonzero? ||
-          (@problem_counts[b.id] <=> @problem_counts[a.id]).nonzero? ||
+        apps.sort do |a, b|
+          (b.unresolved_count <=> a.unresolved_count).nonzero? ||
+          (b.problem_count <=> a.problem_count).nonzero? ||
           a.name <=> b.name
         end
       end
@@ -169,6 +169,14 @@ class App
     end
   end
  
+  def unresolved_count
+    @unresolved_count ||= problems.unresolved.count
+  end
+
+  def problem_count
+    @problem_count ||= problems.count
+  end
+
   protected
  
     def store_cached_attributes_on_problems
@@ -47,8 +47,8 @@
               - revision = app.deploys.last.short_revision
               = link_to( app.last_deploy_at.to_s(:micro) << (revision.present? ? " (#{revision})" : ""), app_deploys_path(app))
         %td.count
-          - if @problem_counts[app.id] > 0
-            - unresolved = @unresolved_counts[app.id]
+          - if app.problem_count > 0
+            - unresolved = app.unresolved_count
             = link_to unresolved, app_path(app), :class => (unresolved == 0 ? "resolved" : nil)
     - if @apps.none?
       %tr
@@ -31,17 +31,6 @@ describe AppsController do
         assigns(:apps).should_not include(unwatched_app)
       end
     end
-
-    context 'when there is only one app' do
-      it 'sets unresolved_counts and problem_counts variables' do
-        sign_in Fabricate(:admin)
-        app = Fabricate(:app)
-        get :index
-
-        assigns(:unresolved_counts).should == {app.id => 0}
-        assigns(:problem_counts).should    == {app.id => 0}
-      end
-    end
   end
  
   describe "GET /apps/:id" do
@@ -4,8 +4,6 @@ describe &quot;apps/index.html.haml&quot; do
   before do
     app = stub_model(App, :deploys => [stub_model(Deploy, :created_at => Time.now, :revision => "123456789abcdef")])
     assign :apps, [app]
-    assign :problem_counts, {app.id => 0}
-    assign :unresolved_counts, {app.id => 0}
     controller.stub(:current_user) { stub_model(User) }
   end
...	...	@@ -52,20 +52,13 @@ class AppsController < InheritedResources::Base
52	52
53	53	protected
54	54	def collection
55		- @unresolved_counts, @problem_counts = {}, {}
56	55	@apps \|\|= begin
57	56	apps = end_of_association_chain.all
58	57
59		- # Cache counts for unresolved errs and problems
60		- apps.each do \|app\|
61		- @unresolved_counts[app.id] \|\|= app.problems.unresolved.count
62		- @problem_counts[app.id] \|\|= app.problems.count
63		- end
64		-
65	58	# Sort apps by number of unresolved errs, then problem counts.
66		- apps.sort do \|a,b\|
67		- (@unresolved_counts[b.id] <=> @unresolved_counts[a.id]).nonzero? \|\|
68		- (@problem_counts[b.id] <=> @problem_counts[a.id]).nonzero? \|\|
	59	+ apps.sort do \|a, b\|
	60	+ (b.unresolved_count <=> a.unresolved_count).nonzero? \|\|
	61	+ (b.problem_count <=> a.problem_count).nonzero? \|\|
69	62	a.name <=> b.name
70	63	end
71	64	end
...	...
...	...	@@ -169,6 +169,14 @@ class App
169	169	end
170	170	end
171	171
	172	+ def unresolved_count
	173	+ @unresolved_count \|\|= problems.unresolved.count
	174	+ end
	175	+
	176	+ def problem_count
	177	+ @problem_count \|\|= problems.count
	178	+ end
	179	+
172	180	protected
173	181
174	182	def store_cached_attributes_on_problems
...	...
...	...	@@ -47,8 +47,8 @@
47	47	- revision = app.deploys.last.short_revision
48	48	= link_to( app.last_deploy_at.to_s(:micro) << (revision.present? ? " (#{revision})" : ""), app_deploys_path(app))
49	49	%td.count
50		- - if @problem_counts[app.id] > 0
51		- - unresolved = @unresolved_counts[app.id]
	50	+ - if app.problem_count > 0
	51	+ - unresolved = app.unresolved_count
52	52	= link_to unresolved, app_path(app), :class => (unresolved == 0 ? "resolved" : nil)
53	53	- if @apps.none?
54	54	%tr
...	...
...	...	@@ -31,17 +31,6 @@ describe AppsController do
31	31	assigns(:apps).should_not include(unwatched_app)
32	32	end
33	33	end
34		-
35		- context 'when there is only one app' do
36		- it 'sets unresolved_counts and problem_counts variables' do
37		- sign_in Fabricate(:admin)
38		- app = Fabricate(:app)
39		- get :index
40		-
41		- assigns(:unresolved_counts).should == {app.id => 0}
42		- assigns(:problem_counts).should == {app.id => 0}
43		- end
44		- end
45	34	end
46	35
47	36	describe "GET /apps/:id" do
...	...
...	...	@@ -4,8 +4,6 @@ describe "apps/index.html.haml" do
4	4	before do
5	5	app = stub_model(App, :deploys => [stub_model(Deploy, :created_at => Time.now, :revision => "123456789abcdef")])
6	6	assign :apps, [app]
7		- assign :problem_counts, {app.id => 0}
8		- assign :unresolved_counts, {app.id => 0}
9	7	controller.stub(:current_user) { stub_model(User) }
10	8	end
11	9
...	...