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

Greg Burghardt · Accepted Answer · 2015-05-20 19:50:12Z

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.

Stack Exchange Network

Simple Report Data Class

1 Answer 1

You must log in to answer this question.

Hot Network Questions