1
\$\begingroup\$

I have a situation where I need to predict the input values for a form used to create a new model. The model is a "class report" that a teacher submits for a student after their class finishes and the values of the report I need to predict include the time the class started and finished. These are to be predicted based on previously submitted reports and other model data. When a teacher opens the form to submit a class report, they should see the "class start time" and other values filled in for them automatically with the predictions made.

Currently, this is being handled in the model as one single method and I am looking for a way to refactor it into its own class:

class ClassReport
    ...

    # Get predicted values for report form for a given student, teacher and weekday.
    # e.g. if a student and teacher had a class on the same day as the current day
    # last week, use the same start and end times as the defaults.
    #
    # Values predicted are:
    # - class start time
    # - class finish time
    # - date of next class
    # - start time of next class
    def self.predict_fields(student, teacher, day)
        last_report = ClassReport.most_recent(student, teacher)
        last_report_for_today = ClassReport.most_recent_for_wday(student, teacher, day.wday)

        if Matching.has_class_today?(student, teacher)
            # Get schedule from calendar
            class_start, class_finish = Matching.get_class_time(student, teacher)
        elsif last_report.present?
            if last_report_for_today.present?
                # Use start time from last report for this weekday
                class_start = last_report_for_today.class_start
            else
                # Use start time from last report
                class_start = last_report.class_start
            end

            if last_report_for_today.present?
                # Use duration from last report for current weekday
                class_finish = class_start + last_report_for_today.class_minutes.minutes
            else
                # Default duration of 1 hour
                class_finish = class_start + 1.hour
            end
        else
            # Set time to 1 hour before now
            # Default duration of 1 hour
            class_start = Time.now-1.hour
            class_finish = Time.now
        end

        # Get next class day and try and get last report for that weekday

        next_class_wday = Matching.next_class_wday(student, teacher)

        if next_class_wday.present?
            next_report = ClassReport.most_recent_for_wday(student, teacher, next_class_wday)
        end

        if next_report.present?
            # Use next class day and time from previous report for this weekday
            next_report_day = next_report.class_day
            next_report_time = next_report.class_start
        else
            if next_class_wday.present?
                # Use next matching day as date
                date_diff = (day.wday - next_class_wday).abs
                next_report_day = day + date_diff.days
            else
                # One week from now
                next_report_day = day + 1.week
            end

            if last_report.present?
                # Set time to same as previous report
                next_report_time = last_report.class_start
            else
                # Set time to now-1h
                next_report_time = Time.now-1.hour
            end
        end

        [class_start, class_finish, next_report_day, next_report_time]
    end

You can see that this uses multiple models and is doing far too much for a single method. Ideally I would want this as a class that takes student, teacher and day in the constructor, does some initialisation, and has methods for ReportPrediction#class_start, ReportPrediction#class_finish, etc.

I'm not sure if this is the best approach though, or where such a class should be placed. I had a read through these refactoring patterns but couldn't see any that I thought fitted this situation. The closest seemed to be a "policy object" but that appears to be more commonly used for returning boolean values based on model data. Service objects also seem slightly different as the way the author describes them implies they are for write operations.

How would you refactor this in a Rails-like manner? Or in more general terms, where would you have logic that looks at the attributes of multiple models and gives you back some values based on those?

\$\endgroup\$
1
\$\begingroup\$

For any complex piece of logic that involves multiple models, I'd rely on a service. Here's how I'd refactor your method into services:

#result= PredictFieldService.build.call(student, teacher, day)

class PredictFieldService

   def self.build
      new(ClassStartFinishService.build, .....)
   end

   def initialize(class_start_finish_service, .......)
      @class_start_finish_service = class_start_finish_service
      ........
   end

   def call(student, teacher, day)
      last_report = get_last_report
      last_report_for_today = get_last_report_for_today

      class_start, class_finish = @class_start_finish_service.call(student, teacher, last_report, last_report_for_today)

      next_report_day = anyservice.call
      next_report_time = whateverservice.call

      [class_start, class_finish, next_report_day, next_report_time]
   end

   private

   def get_last_report
        ClassReport.most_recent(student, teacher)
   end

   def get_last_report_for_today
        ClassReport.most_recent_for_wday(student, teacher, day.wday)
   end
end


class PredictFieldService::ClassStartFinishService

  def self.build
      new
  end

  def call(student, teacher, last_report, last_report_for_today)
      if Matching.has_class_today?(student, teacher)
          class_start, class_finish = get_matching_class 
      else
          class_start, class_finish = get_start_finish(last_report, last_report_for_today)
      end

      class_start, class_finish
  end

  private

  def get_matching_class
      Matching.get_class_time(student, teacher)
  end

  def get_start_finish(last_report, last_report_for_today)
      if last_report.present?
          if last_report_for_today.present?
              class_start = last_report_for_today.class_start
              class_finish = class_start + last_report_for_today.class_minutes.minutes
          else
              class_start = last_report.class_start
              class_finish = class_start + 1.hour
          end
      else
          class_start = Time.now-1.hour
          class_finish = Time.now
      end
      [class_start, class_finish]
   end

end

This way you can test each piece of logic encapsulated in a service in isolation, and expand the functionality through dependency injection.

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