Computing overall averages in my web application

Question

I'm a senior Comp. Sci. major working on a senior design project for our faculty. The name of this project is "Gradebook", and it is responsible for allowing instructors to record grades for students and for students to check their grades in a class. This project is written in Ruby on Rails, and this feature set is integrated into our current CS website.

One requirement for our project is to constantly keep the course average and each of the student's averages updated, so I've designed a CourseInfo class and a StudentInfo class to help with this process.

The CourseInfo class accepts a Gradebook (an ActiveRecord object) as a parameter and calculates the course average. It creates an Associative Array of StudentInfo objects, with each StudentInfo object containing the student's overall average in the class. It also eagerly loads all of Gradebook's ActiveRecord associations in order to avoid N + 1 queries.

Every time a student's grade is updated, the new student average and course average automatically updates in the UI through AJAX. The problem is, the CourseInfo object has to keep being recreated with each request sent to the server, which is a bottleneck. My plan is to serialize the CourseInfo object and store it in the Gradebook table.

However I also want to make sure the way I'm calculating the overall averages is efficient too. What do you think of my approach to doing this? Do you spot any potential bottlenecks or inefficiencies in my code?

To explain simply how our database tables are setup:

1. A Gradebook has many Categories (and belongs to a Gradebook)

2. A Category has many Assignments (and belongs to a Category)

3. An Assignment has many Grades (and belongs to an Assignment and Gradebook)

And to explain how a Gradebook is setup:

1. A Gradebook can be setup in a point-based system or percentage-based system. In a point-based system, all categories are weighted by points, in a percentage-based system, all categories are weighted by percentages.

2. A Category can be created in a point-based system or a percentage-based system. In a point-based system, all assignments are weighted by points, in a percentage-based system, all assignments are weighted by percentages.

course_info.rb

module Api

# Organizes Gradebook data to retrieve useful information

    class CourseInfo

    # Initializes the CourseInfo object by eagerly loading the all Gradebook data and  initializing student data.
    #
    # ==== Attributes
    #
    # +gradebook+ - A Gradebook ActiveRecord object
    def initialize ( gradebook )

      @percentage_sum       = 0
      @students_with_grades = 0

      refresh_categories(gradebook)
      init_student_hash(gradebook)
    end

    # Returns the overall average in the class
    def get_overall_average
      @percentage_sum / @students_with_grades.to_f unless @students_with_grades == 0
    end

    # Returns all eagerly loaded data
    def get_all_categories
      @all_categories
    end

    # Returns a StudentInfo object when given a User object representing a student in the class. This StudentInfo object can
    # be used to retrieve the student's average in the class.
    #
    # ==== Attributes
    #
    # +student+ - The User ActiveRecord object that represents a student in the class.
    def get_student_info(student)
      @student_hash[student.id]
    end

    private

    # Eagerly loads all categories, assignments, and grades into the CourseInfo object
    #
    # ==== Attributes
    #
    # +gradebook+ - The associated Gradebook ActiveRecord object
    def refresh_categories(gradebook)
      @all_categories = gradebook.categories.all(include: [{assignments: :grades}])
    end

    # Initializes an Associate Array of StudentInfo objects
    #
    # ==== Attributes
    #
    # +gradebook+ - The associated Gradebook ActiveRecord object
    def init_student_hash(gradebook)

      # Initialize the Hash table
      @student_hash = Hash.new

      # For all students in the Gradebook
      gradebook.fetch_all_students.each do |student|

        # Create a new StudentInfo object using the student's ID as a key
        @student_hash[student.id] = StudentInfo.new(gradebook.grading_system, @all_categories, student)

        # Check if the student has an average in the class, if so, we
        # include this in the calculation of the overall average of the class
        my_student_info = @student_hash[student.id]

        if !(my_student_info.get_overall_average.nil?)
          @percentage_sum += my_student_info.get_overall_average
          @students_with_grades += 1
        end

      end

    end
  end
end

student_info.rb

module Api

  # A StudentInfo object stores the student's overall average in the class, and will constantly update the student's
  # overall average automatically.

  class StudentInfo

    # Initializes the StudentInfo object by averaging all of the student's assignments in the class
    #
    # ==== Attributes
    #
    # +grading_system+ - The class's grading system (percentages or points)
    # +all_categories+ - All categories in the class (eagerly loaded from Gradebook ActiveRecord object)
    # +student+ - The User ActiveRecord object represeting the student in the class
    def initialize ( grading_system, all_categories, student )          
      @grading_system = grading_system
      init_student_overall_average(all_categories, student)
    end

    # Returns the student's overall average in the class unless the Gradebook is in a percentage system.
    def get_overall_average
      @overall_average
    end

    # Returns the points earned in the class if the class is in a point-based system
    def get_points_earned
      @points_earned unless @grading_system == "percentages"
    end

    # Returns the max points in the class if the class is in a point-based system
    def get_max_points
      @points_max unless @grading_system == "percentages"
    end

    private

    # Averages all the student's grades in the class
    #
    # ==== Attributes
    #
    # +all_categories+ - All categories in the class (eagerly loaded from Gradebook ActiveRecord object)
    # +student+ - The User ActiveRecord object represeting the student in the class
    def init_student_overall_average(all_categories, student)

      if gradebook.grading_system == "percentages"
        # In a percentage-based class, compute the student's overall average in the class
        computeOverallAverage(all_categories, student) 

      elsif gradebook.grading_system == "points"
        # In a point-based system, compute the student's overall earned and max points in the class
        computeStudentPoints(all_categories, student)
      end
    end

    private

    # Calculate the student's overall average in a percentage-based system

    # ==== Attributes
    #
    # +all_categories+ - All categories in the class (eagerly loaded from Gradebook ActiveRecord object)
    # +student+ - The User ActiveRecord object represeting the student in the class
    def computeOverallAverage(all_categories, student)

      # A flag that sets if the student has grades in the class.
      has_no_grades  = true
      # The sum of all the grade's percentage-earned values - used for calculating average.
      percentage_sum = 0.0
      # The sum of all the grade's percentage-max values - used for calculating average.
      total_weight   = 0.0

      all_categories.each do |category|
        category.assignments.each do |assignment|

          # Fetch the assignment grade
          my_grade = assignment.get_student_grade(student.id)

          # If this grade has been set
          if !(my_grade.points_earned.nil?) || !(my_grade.percentage.nil?)

            # Student has grades in the class            
            has_no_grades     = false
            # Get the assignment's total weight 
            assignment_weight = assignment.get_overall_weight

            # If the Category's Grading System is point-based
            if category.grading_system == "points"

              # Calculate the percentage, multiplied by the assignment's weight in the class 
              percentage = (my_grade.points_earned / assignment.point_total) * assignment_weight

            # If the Category's Grading System is percentage-based
            elsif category.grading_system == "percentages"

              # Multiply percentage-earned by the assignment's weight in the class
              percentage = my_grade.percentage * assignment_weight
            end

            # Sum up the percentages and total assignment weights
            percentage_sum += percentage
            total_weight   += assignment_weight
          end
        end
      end

      # The overall average is the sum of the percentages divided by the grand total of all
      # assignment weights
      @overall_average = percentage_sum / total_weight unless has_no_grades
    end

    # Calculate the student's total points earned in a point-based system. 

    # ==== Attributes
    #
    # +all_categories+ - All categories in the class (eagerly loaded from Gradebook ActiveRecord object)
    # +student+ - The User ActiveRecord object represeting the student in the class
    def computeStudentPoints(all_categories, student)

      # Initialize points-earned and max points
      points_earned = 0.0
      points_max    = 0.0

      all_categories.each do |category|
        category.assignments.each do |assignment|

          # Fetch the assignment grade
          my_grade = assignment.get_student_grade(student.id)

          # If this grade has been set
          if !(my_grade.points_earned.nil?) || !(my_grade.percentage.nil?)

            # If the Grading System of the Category is points
            if category.grading_system == "points"

              # Add up the points earned in the grade and the max points in the assignment
              points_earned += my_grade.points_earned
              points_max    += assignment.point_total

            # If the grading system is percentages
            elsif category.grading_system == "percentages"

              # Add up the points earned and max points in the grade (percentage multiplied 
              # by the assignment's total weight)
              points_earned += my_grade.percentage * (assignment.weight * category.point_total)
              points_max    += assignment.weight * category.point_total

            end
          end

        end
      end

      # Set total points earned and total max points, and compute stuent's overall average based
      # on the information. 
      if points_max != 0
        @points_earned   = points_earned
        @points_max      = points_max
        @overall_average = @points_earned / @points_max
      end
    end
  end
end

assignment.rb

class Assignment < ActiveRecord::Base
  include ActionView::Helpers

  after_initialize :set_unique_id
  after_create :init_assignment_grades

  belongs_to :category
  has_many :grades, dependent: :destroy
  has_many :topics, dependent: :destroy

  def get_gradebook
    return Category.find(self.category_id).gradebook
  end

  def create_assignment_grades_for_user(gradebook, user)
    if user.student? || user.user?
      new_grade = Grade.create(student_name: user.email_short, dropped: false)
      self.grades << new_grade
      user.grades << new_grade
      gradebook.grades << new_grade
    end
  end

  def get_overall_weight
    # Get the category this assignment belongs to
    category = self.category
    # Get the Gradebook
    gradebook = get_gradebook

    # Is this category setup percentage-based?
    if category.grading_system == "percentages"
      # Then simply fetch the weight from the table
      assignment_weight = self.weight
    # Is this category point-based?  
    elsif category.grading_system == "points"
      # Then divide the point total of the assignment by the category's point total
      assignment_weight =  self.point_total / category.point_total
    end

    # Is the Gradebook setup percentage-based?
    if gradebook.grading_system == "percentages"
      # Then simply fetch the category's weight from the table
      category_weight = category.weight
    # Is the Gradebook setup point-based?  
    elsif gradebook.grading_system == "points"
      # Then divide the category's point total by the Gradebook's point total
      category_weight = category.point_total / gradebook.point_total
    end

    # The assignment's total weight is it's weight multiplied by its category's weight 
    return assignment_weight * category_weight
  end

  def get_student_grade(student_id)
    myGrade = self.grades.index_by(&:user_id)[student_id]
    return myGrade
  end

  def get_percentage_average
    return self.grades.average(:percentage)
  end

  def get_points_average
    return self.grades.average(:points_earned)
  end

  def get_percentage_min
    return self.grades.minimum(:percentage)
  end

  def get_points_min
    return self.grades.minimum(:points_earned)
  end

  def get_percentage_max
    return self.grades.maximum(:percentage)
  end

  def get_points_max
    return self.grades.maximum(:points_earned)
  end

  private

  def set_unique_id
    self.key ||= SecureRandom.hex if self.new_record?
  end

  def init_assignment_grades
    my_gradebook = get_gradebook
    my_gradebook.users.each do |user|
      create_assignment_grades_for_user(my_gradebook, user)
    end
  end
end

Answer 1

Lots of excellent stuff here! Some points I would mention:

class CourseInfo
  def get_all_categories
    @all_categories
  end
end

Use an attr_reader instead of this trivial accessor; this code becomes:

class CourseInfo
  attr_reader :all_categories
end

If you didn't know, attr_reader creates a method called <attribute> (in this case, all_categories) that simply returns the instance variable (@all_categories, exactly what you already have). Remember to change the places you call it from get_all_categories to all_categories.

This use of attr_reader wont' suffice for some of your other getters, as they have a bit of logic behind them that you need to state.

This leads into the next point - rename all your get_some_attribute methods to some_attribute. This will keep in line with the attr_reader standard.

---

if !(my_student_info.get_overall_average.nil?)

Ordinarily, you would just use unless instead of if !(condition), but in this case you can exploit the fact the ruby treats EVERYTHING that isn't nil or false as true; change this line to:

if my_student_info.overall_average

(Note that I dropped the .nil?, the parentheses, and the get_... as per my previous point). You typically don't use parentheses on conditionals unless it would be prohibitively confusing without; see the style guide

Make a similar fix for this and other lines:

 if !(my_grade.points_earned.nil?) || !(my_grade.percentage.nil?)

---

@student_hash = Hash.new

Don't do that (see style guide); do this:

@student_hash = {}

---

In class StudentInfo, you say that all_categories and student are attributes but you are not treating them like attributes. You pass them to the function init_student_overall_average when that function could just access them as instance variables using @all_categories and @student syntax.

This will first require you to set those variables, however. Either pass them in to the initializer and set them explicitly, or you could use attr_writer in much the same way as attr_reader above to create the all_categories= and student= methods. Then you can set the variables whenever you want by calling, for example,

this_student_info.student = Student.new("Steve") # Or however students are made

---

You have way too many comments, delete pretty much all of them except for the ones that are REQUIRED to understand what the code does;

# Get the Gradebook
gradebook = get_gradebook

REALLY not needed. Now get rid of the rest of the comments too, and reword your code so that you can understand what it does without needing the comment.

---

In class Assignment in your vast amount of getters, you don't need to be specifying the self keyword or the return keyword. Methods are automatically invoked on self first if they exist on self, and everything in ruby automatically returns the final line it computes before exiting. You use the return keyword in a few other places as well and for your cases it's not needed. You DO use it when you want to return early from a method, but you're not doing that here.

(Edit: I'm not entirely sure about not needing to use the self keyword, because your grades are not instance variables - see below`)

---

Finally, in class Assignment your grades and other things should arguably be instance variables (@grades) but I'm not entirely familiar with how has_many sets things up so this might be more trouble than it's worth.