3
\$\begingroup\$

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
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

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.

\$\endgroup\$

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.