From a690e4a4cdd9c02fb548531435c6486b1f486a9f Mon Sep 17 00:00:00 2001
From: Katrina Owen <katrina.owen@gmail.com>
Date: Sun, 17 Mar 2013 20:37:29 -0600
Subject: [PATCH] Extract methods in pie chart data model

---
 app/controllers/stats_controller.rb |  2 -
 app/models/stats/pie_chart_data.rb  | 97 ++++++++++++++++++++++-------
 test/unit/pie_chart_data_test.rb    |  4 --
 3 files changed, 73 insertions(+), 30 deletions(-)

diff --git a/app/controllers/stats_controller.rb b/app/controllers/stats_controller.rb
index 107eab85..b92e7287 100644
--- a/app/controllers/stats_controller.rb
+++ b/app/controllers/stats_controller.rb
@@ -181,7 +181,6 @@ class StatsController < ApplicationController
     actions = Stats::TopContextsQuery.new(current_user).result
 
     @data = Stats::PieChartData.new(actions, t('stats.spread_of_actions_for_all_context'), 70)
-    @data.calculate
 
     render :pie_chart_data, :layout => false
   end
@@ -189,7 +188,6 @@ class StatsController < ApplicationController
   def context_running_actions_data
     actions = Stats::TopContextsQuery.new(current_user, :running => true).result
     @data = Stats::PieChartData.new(actions, t('stats.spread_of_running_actions_for_visible_contexts'), 60)
-    @data.calculate
 
     render :pie_chart_data, :layout => false
   end
diff --git a/app/models/stats/pie_chart_data.rb b/app/models/stats/pie_chart_data.rb
index 10cc1eec..9a5ded53 100644
--- a/app/models/stats/pie_chart_data.rb
+++ b/app/models/stats/pie_chart_data.rb
@@ -1,37 +1,86 @@
 module Stats
   class PieChartData
 
-    attr_reader :all_actions_per_context, :values, :labels, :ids, :alpha, :title
-    def initialize(all_actions_per_context, title, alpha)
-      @all_actions_per_context = all_actions_per_context
+    attr_reader :all_totals, :alpha, :title
+    def initialize(all_totals, title, alpha)
+      @all_totals = all_totals
       @title = title
       @alpha = alpha
     end
 
-    def calculate
-      sum = all_actions_per_context.inject(0){|sum, apc| sum + apc['total']}
-
-      pie_cutoff=10
-      size = [all_actions_per_context.size, pie_cutoff].min
-
-      # explicitly copy contents of hash to avoid ending up with two arrays pointing to same hashes
-      actions_per_context = Array.new(size){|i| {
-        'name' => all_actions_per_context[i]['name'],
-        'total' => all_actions_per_context[i]['total'],
-        'id' => all_actions_per_context[i]['id']
-      } }
-
-      if all_actions_per_context.size > pie_cutoff
-        actions_per_context[-1]['name']=I18n.t('stats.other_actions_label')
-        actions_per_context[-1]['id']=-1
-        size.upto(all_actions_per_context.size-1){ |i| actions_per_context[-1]['total']+=(all_actions_per_context[i]['total']) }
+    def values
+      @values ||= Array.new(slices) do |i|
+        chart_totals[i]['total'] * 100 / sum
       end
-
-      @values = Array.new(size){|i| actions_per_context[i]['total']*100/sum }
-      @labels = Array.new(size){|i| actions_per_context[i]['name'].truncate(15, :omission => '...') }
-      @ids = Array.new(size){|i| actions_per_context[i]['id']}
     end
 
+    def labels
+      @labels ||= Array.new(slices) do |i|
+        chart_totals[i]['name'].truncate(15, :omission => '...')
+      end
+    end
+
+    def ids
+      @ids ||= Array.new(slices) do |i|
+        chart_totals[i]['id']
+      end
+    end
+
+    private
+
+    def sum
+      @sum ||= totals.inject(0) do |sum, amount|
+        sum + amount
+      end
+    end
+
+    def pie_cutoff
+      10
+    end
+
+    def slices
+      @slices ||= [all_totals.size, pie_cutoff].min
+    end
+
+    def subtotal(from, to)
+      totals[from..to].inject(0) do |sum, amount|
+        sum + amount
+      end
+    end
+
+    def chart_totals
+      unless @chart_totals
+        @chart_totals = first_n_totals(10)
+        if all_totals.size > pie_cutoff
+          @chart_totals[-1] = other
+        end
+      end
+      @chart_totals
+    end
+
+    def first_n_totals(n)
+      # create a duplicate so that we don't accidentally
+      # overwrite the original array
+      Array.new(slices) do |i|
+        {
+          'name' => all_totals[i]['name'],
+          'total' => all_totals[i]['total'],
+          'id' => all_totals[i]['id']
+        }
+      end
+    end
+
+    def other
+      {
+        'name' => I18n.t('stats.other_actions_label'),
+        'id' => -1,
+        'total' => subtotal(slices-1, all_totals.size-1)
+      }
+    end
+
+    def totals
+      @totals ||= all_totals.map { |item| item['total'] }
+    end
 
   end
 end
diff --git a/test/unit/pie_chart_data_test.rb b/test/unit/pie_chart_data_test.rb
index bf5a787d..d5933d12 100644
--- a/test/unit/pie_chart_data_test.rb
+++ b/test/unit/pie_chart_data_test.rb
@@ -12,7 +12,6 @@ class Stats::PieChartDataTest < Test::Unit::TestCase
 
   def test_with_0_items
     data = Stats::PieChartData.new([], 'a chart', 50)
-    data.calculate
 
     assert_equal [], data.values
     assert_equal [], data.labels
@@ -33,7 +32,6 @@ class Stats::PieChartDataTest < Test::Unit::TestCase
     ]
 
     data = Stats::PieChartData.new(items, 'a chart', 50)
-    data.calculate
 
     assert_equal [12, 4, 9, 15, 23, 20, 5, 1, 7], data.values
     assert_equal ["one", "two", "three", "four", "five", "six", "seven", "eight", "nine"], data.labels
@@ -55,7 +53,6 @@ class Stats::PieChartDataTest < Test::Unit::TestCase
     ]
 
     data = Stats::PieChartData.new(items, 'a chart', 50)
-    data.calculate
 
     assert_equal [10, 3, 7, 12, 19, 16, 4, 0, 5, 18], data.values
     assert_equal ["one", "two", "three", "four", "five", "six", "seven", "eight", "nine", "ten"], data.labels
@@ -78,7 +75,6 @@ class Stats::PieChartDataTest < Test::Unit::TestCase
     ]
 
     data = Stats::PieChartData.new(items, 'a chart', 50)
-    data.calculate
 
     assert_equal [9, 3, 6, 11, 16, 14, 4, 0, 5, 27], data.values
     assert_equal ["one", "two", "three", "four", "five", "six", "seven", "eight", "nine", "(other)"], data.labels