5
\$\begingroup\$

High Level Explanation of Code's Purpose

I wrote a class to create sectioned reports, where each section has a subtotal. It allows these reports to be nested within each other, to create multiple levels of subtotals. An example will help clarify the idea before getting to the real code....

Consider this sample data:

rows = [
  {person: 'john'  , team: 'red'  , height: 73, age: 30} ,
  {person: 'carl'  , team: 'red'  , height: 65, age: 23} ,
  {person: 'sam'   , team: 'red'  , height: 74, age: 42} ,
  {person: 'joe'   , team: 'red'  , height: 67, age: 29} ,
  {person: 'bob'   , team: 'blue' , height: 77, age: 45} ,
  {person: 'james' , team: 'blue' , height: 71, age: 26} ,
  {person: 'danny' , team: 'blue' , height: 70, age: 26} ,
  {person: 'billy' , team: 'blue' , height: 67, age: 27} ,
]

Here's a report showing subtotals by team, along with a grand total:

subtotals by team

We may want to drill down further, and create subtotals within the team totals based on tall people (> 72 inches) and normal people. It might look like this:

subtotals by height

Example Client Code

To make the first report pictured above, we'd use the class I wrote, ReportWithSections, like this:

report = ReportWithSections.new(
  rows: rows, 
  totals_cols: [:height, :age], 
  total_label_proc: ->(totals, *) {totals.update(:person => 'Grand Total')}
)

by_team = ->(totals) {totals[:team]}

team_label = ->(totals, report) do 
  team = report.last_row[:team] 
  totals.update(:person => "Team #{team} Total")
end

report = report.add_subtotals(subsection_proc: by_team, total_label_proc: team_label)

puts html_table(report.rows_with_totals)

Note that html_table is just a simple "view" function for the purposes of this post, and not part of my question, but for completeness it looks like this:

def html_table(rows)
  row = ->(x) { "<tr>#{x}</tr>\n" }
  header = rows.first.keys.map {|x| "<th>#{x}</th>"}.join
  header = row.(header)
  cells = ->(r) {r.values.map  {|x| "<td>#{x}</td>"}.join}
  all_tds = rows.map {|r| row.(cells.(r))}.join
  "<table border=1>#{header + all_tds}</table>"
end

Finally, to create the 2nd report pictured above, which includes height subtotals, we'd continue as follows:

by_height = ->(totals) {totals[:height] > 72}

height_label = ->(totals, report) do 
  height = report.last_row[:height] > 72 ? 'Tall' : 'Normal'
  totals.update(:person => "#{height} Total")
end

report = report.add_subtotals(subsection_proc: by_height, total_label_proc: height_label)

puts html_table(report.rows_with_totals)

So, we can specify which columns we want to total, and we can dynamically create the labels for the subtotals however we want, and put those labels in whatever column we want, although in this example we choose to place all subtotal labels in the first column.

The Code Itself

The class below is my current implementation. I am fairly happy with it. The methods are short and clearly named I think, indendation is kept to one level for the most part, and the public API is small. There is one thing that's not sitting with well with me, yet I can't figure out a way around it. This is what I'd like suggestions on, although other suggestions are welcome too. I'll put my thoughts after the code, in case you want to form your opinion by reading the code without bias.

class ReportWithSections

  attr_accessor :all_cols, :totals_cols, :totals

  # rows can be [{}] or [ReportWithSections]
  def initialize(rows:, totals_cols:, total_label_proc: nil)
    @rows, @totals_cols, @total_label_proc = rows, totals_cols, total_label_proc
    @all_cols = nested? ? @rows.first.all_cols : @rows.first.keys
    @totals = make_totals_row
    @totals = total_label_proc.(@totals, self) if total_label_proc
  end

  def rows_with_totals
    flatten_with_totals + [@totals]
  end

  def rows_without_totals
    flatten_without_totals
  end

  def last_row
    rows_without_totals.last
  end

  def add_subtotals(subsection_proc:, total_label_proc: nil)
    method = nested? ? :nested_add_subtotals : :simple_add_subtotals
    send(method, subsection_proc: subsection_proc, 
                 total_label_proc: total_label_proc)
  end

  private

  def clone_with(rows:, total_label_proc: nil)
    label_proc = total_label_proc || @total_label_proc
    ReportWithSections.new(rows: rows, totals_cols: @totals_cols, 
                           total_label_proc: label_proc)
  end

  def simple_add_subtotals(subsection_proc:, total_label_proc: nil)
    subsections = @rows.group_by(&subsection_proc)
    return self if subsections.size == 1

    nested_reports = subsections.map do |chunk_elm|
      clone_with(rows: chunk_elm.last, total_label_proc: total_label_proc)
    end
    clone_with(rows: nested_reports)
  end

  def nested_add_subtotals(subsection_proc:, total_label_proc: nil)
    rows = @rows.map {|r| r.add_subtotals(subsection_proc: subsection_proc, total_label_proc: total_label_proc)}
    clone_with(rows: rows)
  end

  def flatten_with_totals
    return @rows unless nested?
    @rows.map {|r| r.rows_with_totals}.flatten
  end

  def flatten_without_totals
    return @rows unless nested?
    @rows.map {|r| r.rows_without_totals}.flatten
  end

  def make_totals_row
    nested? ? sum_rows(nested_totals) : sum_rows(@rows)
  end

  def sum_rows(rows)
    rows.each_with_object(zeroed_totals) do |row, total|
      @totals_cols.each {|c| total[c] += row[c]}
    end
  end

  def nested_totals
    @rows.map {|r| r.totals}
  end

  def totals_col?(col)
    @totals_cols.include? col
  end

  def zeroed_totals
    @all_cols.map {|c| [c, totals_col?(c) ? 0 : nil]}.to_h
  end

  def nested?
    @rows.first.is_a? ReportWithSections
  end

end

What is not sitting with well with me is that is_a? ReportWithSections inside of nested? -- those calls to nested? are really switch statements on type. And switching on type smells of an incorrect, or missing, abstraction. And yet, I see no way around it, since the rows of hashes are the base case of this recursive structure, and seem to need different treatment. What's worse, if I decided to provide access to the "rows" (of hashes or of ReportWithSections) and totals separately, to support views that, eg, wanted to put the totals row above the rows it sums, then this type switching would leak out into the view.

Is there a better approach to this kind of nested structure that avoids these problems?

\$\endgroup\$
  • \$\begingroup\$ As you said, there seems to be an improper abstraction, you probably should have separate classes to generate report and to hold data, what would allow you to convert those hashes to something more usefull before further operation. Instead of using polimorphism you pass behaviour around in weird ways, what makes your code pretty hard to follow, even if it looks quite nice aprat from it. I'll add more detailed answer later. \$\endgroup\$ – Borsunho Oct 19 '15 at 9:33
  • \$\begingroup\$ Thanks, just FYI using dependency injection rather than inheritance is a very conscious choice. I pretty much think inheritance is an anti-pattern but that is a whole other long subject not appropriate for here. however if you believe you have a good solution which uses it I am open to seeing it \$\endgroup\$ – Jonah Oct 19 '15 at 13:09
1
\$\begingroup\$

Code smell you noticed is called fake polimorphism, and - as name indicates - it's easily removed by introducing real polimorphism, thus making code compliant to tell, don't ask rule.

In your case, you could create an "unnested" report class, convert your rows of data to it, and split implementation of each method that needs to check for nested between it and ReportWithSection:

def SimpleReport
  def initialize data # or 'rows'
    @data = data
  end

  def flatten_with_totals
    self # note that we do not return @data hash - this class abstracts the hash!
  end

  def add_subtotals
    # basicly your simple_add_subtotals
  end

  def totals
    sum_rows(@rows)
  end
  # ... and so on ...
end

If you end up with inheritance that bothers you that way, refactor it into composition, but I don't see any sane reason to do so - Ruby modules are flexible enough to suffice in most cases, and, after all, those classes were one in your code, so adding SimpleReport loosens coupling anyways ;)


Passing @totals to total_label proc smells with exposing implementation details. Passing self smells with Python. It would look much nicer if you passed result of grouping proc (key that groups record) and returned appropiate label label:

by_height = ->(record) {record[:height] > 72}

height_label = ->(are_tall) do 
  are_tall ? 'Tall Total' : 'Normal Total'
  # no update, your class does that
end

Now this is less flexible, but easier to refactor and produces nicer interface.


Finally, because your class holds both data and lambdas describing the report, it forces user to manually handle each step of generating the report, so interface is tiresome to use and - unless I missed something - there is no easy way to generate the same report for different sets of data. This doesn't look like client code at all. Consider adding a 'facade' class that would gather all those lambdas, and than use your class to actually generate report, i.e.

gen = ReportGenerator total_cols: [:height, :age],
                      total_label_proc: ->{ ... }

gen.add_subsection subdivision_proc: ->(record){|record| record[:team]}
                   total_label_proc: ->{ ... }

gen.add_subsection subdivision_proc: ->(record){|record| record[:height] > 72}
                   total_label_proc: ->{ ... }

raport_1 = gen.generate(row_set_1)

raport_2 = gen.generate(row_set_2)
\$\endgroup\$
  • \$\begingroup\$ Funny, I had a class SimpleReport at first, then did away with it, but I think you're right -- it will fix this. I also agree I should have a facade class to make client usage more friendly, but that's more of a cosmetic problem, easily fixable. Your suggestion for removing @totals from the label proc, while agree with it in spirit, damages key functionality. You can only use static labels that way. See "Team red total" and "Team blue total" in screens in my OP. Can you think of a cleaner solution that preserves this ability? \$\endgroup\$ – Jonah Oct 21 '15 at 21:33
  • \$\begingroup\$ My solution preserves this ability, unless I misunderstood you, i.e. ->(team){"Team #{team} Total"}. \$\endgroup\$ – Borsunho Oct 22 '15 at 3:12
  • \$\begingroup\$ I think I misunderstood you. You were suggesting that we simply always use the first column to hold the total, and thus that logic can live inside ReportWithSections and doesn't have to be passed in. Likewise we always pass in the "group by" parameter, and that's the only thing the label proc can use. I think it's a good idea that will work for 90% of cases, even thought it's a bit less flexible. \$\endgroup\$ – Jonah Oct 22 '15 at 3:26

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.