# Simple Report Data Class

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
end

def student_terms
end

private

StudentTerm.where(year_term: year_terms)
.includes(:student)
end

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


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.