Simple Report Data Class

Question

I'm trying to write a class to handle the collecting and calculating of some data. My intention is to have the controller instantiate the Report object, and a very simple view display it. I figured this would keep the controller thin and keep logic out of the views.

class Report
 def initialize(year_term_ids = [])
 @year_term_ids = year_term_ids
 end
 def gpas
 {}.tap do |gpas|
 %i(average minimum maximum).each do |method|
 gpas[method] = student_terms.send(method, :term_gpa).to_f.round(2)
 end
 end
 end
 def students
 student_statuses.merge(
 count: student_terms.count,
 dismissed: student_terms.dismissed,
 on_probation: student_terms.on_probation
 )
 end
 def year_terms
 @year_terms ||= load_year_terms
 end
 def student_terms
 @student_terms ||= load_student_terms
 end
 private
 def load_student_terms
 StudentTerm.where(year_term: year_terms)
 .includes(:student)
 end
 def load_year_terms
 year_terms = YearTerm.includes(student_year_terms: [ :student ])
 if @year_term_ids.empty?
 year_terms
 else
 year_terms.where(id: @year_term_ids)
 end
 end
 def student_statuses
 symbolize_hash(student_terms.map(&:student).group_by(&:status))
 end
 def symbolize_hash(hash)
 hash.keys.each do |k|
 hash[k.underscore.to_sym] = hash.delete(k) if k.is_a?(String)
 end
 hash
 end 
end

Answer 1

The Report#gpas method is utilizing the tap method. This feels like too much magic for something so simple (Related: Should I Tap That Hash). It took me a bit to unravel what that method does, which is essentially:

def gpas
 {
 average: student_terms.average :term_gpa,
 minimum: student_terms.minimum :term_gpa,
 maximum: student_terms.maximum :term_gpa
 }
end

This looks much cleaner and immediately easy to understand -- and it's the same number of lines of code.

I haven't dealt with Ruby much in a few years, but prior to Ruby 2.2 the String#to_sym method was a source of memory leaks, because symbol objects never get garbage collected, and the to_sym method always returned a new Symbol object. The symbolize_hash method could be just such a memory leak. I'm actually wondering why you aren't using classes here instead of a Hash. You are creating these Hashes and then manipulating their data. This feels like you need some additional classes to push this behavior into.

There's also no benefit to checking for @year_term_ids.empty? before adding the "where" condition in load_year_terms. Just a simple:

def load_year_terms
 YearTerm.includes(student_year_terms: [ :student ]).where(id: @year_term_ids)
end

will suffice. In fact this feels like it should be a static method on the YearTerm class itself:

class YearTerm < ActiveRecord::Base
 def self.including_student_for(year_term_ids)
 YearTerm.includes(student_year_terms: [ :student ]).where(id: @year_term_ids)
 end
end

Then just call YearTerm.including_student_for(@year_term_ids) in your Report class.