Refactoring a Single Rails Model with large methods & long join queries trying to do everything

Posted by Kelseydh on Programmers See other posts from Programmers or by Kelseydh
Published on 2014-07-12T16:30:23Z Indexed on 2014/08/19 4:31 UTC
Read the original article Hit count: 395

I have a working Ruby on Rails Model that I suspect is inefficient, hard to maintain, and full of unnecessary SQL join queries. I want to optimize and refactor this Model (Quiz.rb) to comply with Rails best practices, but I'm not sure how I should do it.

The Rails app is a game that has Missions with many Stages. Users complete Stages by answering Questions that have correct or incorrect Answers. When a User tries to complete a stage by answering questions, the User gets a Quiz entry with many Attempts. Each Attempt records an Answer submitted for that Question within the Stage. A user completes a stage or mission by getting every Attempt correct, and their progress is tracked by adding a new entry to the UserMission & UserStage join tables.

All of these features work, but unfortunately the Quiz.rb Model has been twisted to handle almost all of it exclusively. The callbacks began at 'Quiz.rb', and because I wasn't sure how to leave the Quiz Model during a multi-model update, I resorted to using Rails Console to have the @quiz instance variable via self.some_method do all the heavy lifting to retrieve every data value for the game's business logic; resulting in large extended join queries that "dance" all around the Database schema.

The Quiz.rb Model that Smells:

class Quiz < ActiveRecord::Base
  belongs_to :user
  has_many :attempts, dependent: :destroy

  before_save :check_answer
  before_save :update_user_mission_and_stage

  accepts_nested_attributes_for :attempts, :reject_if => lambda { |a| a[:answer_id].blank? }, :allow_destroy => true

  #Checks every answer within each quiz, adding +1 for each correct answer 
  #within a stage quiz, and -1 for each incorrect answer

  def check_answer
    stage_score = 0
    self.attempts.each do |attempt|
      if attempt.answer.correct? == true
        stage_score += 1
      elsif attempt.answer.correct == false
        stage_score - 1
      end
    end
    stage_score
  end

  def winner
   return true
  end

  def update_user_mission_and_stage
    #######
    #Step 1: Checks if UserMission exists, finds or creates one.
    #if no UserMission for the current mission exists, creates a new UserMission
      if self.user_has_mission? == false
        @user_mission = UserMission.new(user_id: self.user.id, 
                          mission_id: self.current_stage.mission_id,
                          available: true)
        @user_mission.save
      else
        @user_mission = self.find_user_mission
      end

    #######
    #Step 2: Checks if current UserStage exists, stops if true to prevent duplicate entry
      if self.user_has_stage?
        @user_mission.save
        return true
      else

    #######
    ##Step 3: if step 2 returns false: 
    ##Initiates UserStage creation instructions
      #checks for winner (winner actions need to be defined) if they complete last stage of last mission for a given orientation
        if self.passed? && self.is_last_stage? && self.is_last_mission?
          create_user_stage_and_update_user_mission
          self.winner

      #NOTE: The rest are the same, but specify conditions that are available to add badges or other actions upon those conditions occurring:
      ##if user completes first stage of a mission
        elsif self.passed? && self.is_first_stage? && self.is_first_mission?
          create_user_stage_and_update_user_mission

          #creates user badge for finishing first stage of first mission
          self.user.add_badge(5)
          self.user.activity_logs.create(description: "granted first-stage badge", type_event: "badge", value: "first-stage")

      #If user completes last stage of a given mission, creates a new UserMission
        elsif self.passed? && self.is_last_stage? && self.is_first_mission?
          create_user_stage_and_update_user_mission

          #creates user badge for finishing first mission
          self.user.add_badge(6)
          self.user.activity_logs.create(description: "granted first-mission badge", type_event: "badge", value: "first-mission")

        elsif self.passed?
          create_user_stage_and_update_user_mission

        else self.passed? == false
          return true
        end
      end
  end

  #Creates a new UserStage record in the database for a successful Quiz question passing
  def create_user_stage_and_update_user_mission
    @nu_stage = @user_mission.user_stages.new(user_id: self.user.id, 
                                              stage_id: self.current_stage.id)
    @nu_stage.save
    @user_mission.save

    self.user.add_points(50)
  end

  #Boolean that defines passing a stage as answering every question in that stage correct
  def passed?
    self.check_answer >= self.number_of_questions
  end


  #Returns the number of questions asked for that stage's quiz
  def number_of_questions
    self.attempts.first.answer.question.stage.questions.count
  end

  #Returns the current_stage for the Quiz, routing through 1st attempt in that Quiz
  def current_stage
    self.attempts.first.answer.question.stage
  end

  #Gives back the position of the stage relative to its mission.
  def stage_position
    self.attempts.first.answer.question.stage.position
  end

  #will find the user_mission for the current user and stage if it exists
  def find_user_mission
    self.user.user_missions.find_by_mission_id(self.current_stage.mission_id)
  end

  #Returns true if quiz was for the last stage within that mission
  #helpful for triggering actions related to a user completing a mission
  def is_last_stage?
    self.stage_position == self.current_stage.mission.stages.last.position
  end

  #Returns true if quiz was for the first stage within that mission
  #helpful for triggering actions related to a user completing a mission
  def is_first_stage?
    self.stage_position == self.current_stage.mission.stages_ordered.first.position
  end

  #Returns true if current user has a UserMission for the current stage
  def user_has_mission?
    self.user.missions.ids.include?(self.current_stage.mission.id)
  end

  #Returns true if current user has a UserStage for the current stage
  def user_has_stage?
    self.user.stages.include?(self.current_stage)
  end

  #Returns true if current user is on the last mission based on position within a given orientation
  def is_first_mission?
     self.user.missions.first.orientation.missions.by_position.first.position == self.current_stage.mission.position
  end


  #Returns true if current user is on the first stage & mission of a given orientation
  def is_last_mission?
     self.user.missions.first.orientation.missions.by_position.last.position == self.current_stage.mission.position
  end

end

My Question

Currently my Rails server takes roughly 500ms to 1 sec to process single @quiz.save action. I am confident that the slowness here is due to sloppy code, not bad Database ERD design.

What does a better solution look like? And specifically:

  1. Should I use join queries to retrieve values like I did here, or is it better to instantiate new objects within the model instead? Or am I missing a better solution?

  2. How should update_user_mission_and_stage be refactored to follow best practices?


Relevant Code for Reference:

quizzes_controller.rb w/ Controller Route Initiating Callback:

  class QuizzesController < ApplicationController
    before_action :find_stage_and_mission
    before_action :find_orientation
    before_action :find_question


  def show
  end

  def create
    @user = current_user
    @quiz = current_user.quizzes.new(quiz_params)
    if @quiz.save
      if @quiz.passed?
         if @mission.next_mission.nil? && @stage.next_stage.nil?
           redirect_to root_path, notice: "Congratulations, you have finished the last mission!"
         elsif @stage.next_stage.nil?
           redirect_to [@mission.next_mission, @mission.first_stage], notice: "Correct! Time for Mission #{@mission.next_mission.position}", info: "Starting next mission"
         else
           redirect_to [@mission, @stage.next_stage], notice: "Answer Correct! You passed the stage!"
         end
      else
       redirect_to [@mission, @stage], alert: "You didn't get every question right, please try again."
      end

    else
      redirect_to [@mission, @stage], alert: "Sorry.  We were unable to save your answer.  Please contact the admministrator."
    end
    @questions = @stage.questions.all

  end

  private


  def find_stage_and_mission
    @stage = Stage.find(params[:stage_id])
    @mission = @stage.mission
  end

  def find_question
    @question = @stage.questions.find_by_id params[:id]
  end

  def quiz_params
    params.require(:quiz).permit(:user_id, :attempt_id, {attempts_attributes: [:id, :quiz_id, :answer_id]}) 

  end

  def find_orientation
  @orientation = @mission.orientation
  @missions = @orientation.missions.by_position
  end

end

Overview of Relevant ERD Database Relationships:

Mission -> Stage -> Question -> Answer -> Attempt <- Quiz <- User

Mission -> UserMission <- User

Stage -> UserStage <- User


Other Models:

Mission.rb

class Mission < ActiveRecord::Base
  belongs_to :orientation


  has_many :stages

  has_many :user_missions, dependent: :destroy
  has_many :users, through: :user_missions

  #SCOPES
  scope :by_position, -> {order(position: :asc)}

  def stages_ordered
    stages.order(:position)
  end


  def next_mission
    self.orientation.missions.find_by_position(self.position.next)
  end

  def first_stage
    next_mission.stages_ordered.first
  end

end

Stage.rb:

class Stage < ActiveRecord::Base
  belongs_to :mission

  has_many :questions, dependent: :destroy

  has_many :user_stages, dependent: :destroy
  has_many :users, through: :user_stages


  accepts_nested_attributes_for :questions, reject_if: :all_blank, allow_destroy: true

  def next_stage
    self.mission.stages.find_by_position(self.position.next)
  end
end

Question.rb

    class Question < ActiveRecord::Base
      belongs_to :stage
      has_many :answers, dependent: :destroy

      accepts_nested_attributes_for :answers, :reject_if => lambda { |a| a[:body].blank? }, :allow_destroy => true
    end

Answer.rb:

class Answer < ActiveRecord::Base
  belongs_to :question

  has_many :attempts, dependent: :destroy
end

Attempt.rb:

class Attempt < ActiveRecord::Base
  belongs_to :answer
  belongs_to :quiz
end

User.rb:

class User < ActiveRecord::Base


  belongs_to :school
  has_many :activity_logs

  has_many :user_missions, dependent: :destroy
  has_many :missions, through: :user_missions

  has_many :user_stages, dependent: :destroy
  has_many :stages, through: :user_stages

  has_many :orientations, through: :school

  has_many :quizzes, dependent: :destroy
  has_many :attempts, through: :quizzes



  def latest_stage_position
    self.user_missions.last.user_stages.last.stage.position
  end

end

UserMission.rb

class UserMission < ActiveRecord::Base

  belongs_to :user
  belongs_to :mission

  has_many :user_stages, dependent: :destroy

end

UserStage.rb

class UserStage < ActiveRecord::Base

  belongs_to :user
  belongs_to :stage
  belongs_to :user_mission
end

© Programmers or respective owner

Related posts about refactoring

Related posts about sql