How to refactor this Ruby (controller) code?

Question

This is the code in my reports controller, it just looks so bad, can anyone give me some suggestions on how to tidy it up?

# app\controller\reports_controller.rb

 @report_lines  = []
   @sum_wp, @sum_projcted_wp, @sum_il, @sum_projcted_il, @sum_li,@sum_gross_profit ,@sum_opportunities = [0,0,0,0,0,0,0]    
 date = @start_date

 num_of_months.times do
    wp,projected_wp, invoice_line,projected_il,line_item, opp = Report.data_of_invoicing_and_delivery_report(@part_or_service,date)
    @sum_wp += wp
    @sum_projcted_wp +=projected_wp
    @sum_il=invoice_line
    @sum_projcted_il +=projected_il
    @sum_li += line_item
    gross_profit = invoice_line - line_item
    @sum_gross_profit += gross_profit
    @sum_opportunities += opp
    @report_lines << [date.strftime("%m/%Y"),wp,projected_wp ,invoice_line,projected_il,line_item,gross_profit,opp]
    date = date.next_month
 end

I'm looking to use some method like

@sum_a,@sum_b,@sum_c += [1,2,3] 

Answer 1

My instant thought is: move the code to a model.

The objective should be "Thin Controllers", so they should not contain business logic.

Second, I like to present my report lines to my Views as OpenStruct() objects, which seems cleaner to me.

So I'd consider moving this accumulation logic into (most likely) a class method on Report and returning an array of "report line" OpenStructs and a single totals OpenStruct to pass to my View.

My controller code would become something like this:

@report_lines, @report_totals = Report.summarised_data_of_inv_and_dlvry_rpt(@part_or_service, @start_date, num_of_months)

EDIT: (A day later)

Looking at that adding accumulating-into-an-array thing, I came up with this:

require 'test/unit'

class Array
  def add_corresponding(other)
    each_index { |i| self[i] += other[i] }
  end
end

class TestProblem < Test::Unit::TestCase
  def test_add_corresponding
    a = [1,2,3,4,5]
    assert_equal [3,5,8,11,16], a.add_corresponding([2,3,5,7,11])
    assert_equal [2,3,6,8,10], a.add_corresponding([-1,-2,-2,-3,-6])
  end
end

Look: a test! It seems to work OK. There are no checks for differences in size between the two arrays, so there's lots of ways it could go wrong, but the concept seems sound enough. I'm considering trying something similar that would let me take an ActiveRecord resultset and accumulate it into an OpenStruct, which is what I tend to use in my reports...

Our new Array method might reduce the original code to something like this:

totals = [0,0,0,0,0,0,0]    
date = @start_date

num_of_months.times do
  wp, projected_wp, invoice_line, projected_il, line_item, opp = Report.data_of_invoicing_and_delivery_report(@part_or_service,date)
  totals.add_corresponding [wp, projected_wp, invoice_line, projected_il, line_item, opp, invoice_line - line_item]
  @report_lines << [date.strftime("%m/%Y"),wp,projected_wp ,invoice_line,projected_il,line_item,gross_profit,opp]
  date = date.next_month
end

@sum_wp, @sum_projcted_wp, @sum_il, @sum_projcted_il, @sum_li, @sum_opportunities, @sum_gross_profit = totals 

...which if Report#data_of_invoicing_and_delivery_report could also calculate gross_profit would reduce even further to:

num_of_months.times do
  totals.add_corresponding(Report.data_of_invoicing_and_delivery_report(@part_or_service,date))
end

Completely un-tested, but that's a hell of a reduction for the addition of a one-line method to Array and performing a single extra subtraction in a model.