0

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
1

I would recommend looking at your SQL log to see what is getting executed.
Also try the free 'newrelic' http://newrelic.com/ruby to help out with a detailed breakdown of performance.

The problem will be that it's difficult to change when/how various queries are being executed when you call ActiveRecord associations so freely as you are now.

Also, you're not actually using joins, but simple "SELECT" loading based on the association.

Separate Data Retrieval and Logic A good strategy to help with this is to try and fetch all the data you need first, then execute the logic on it with simple ruby (not active-record inheriting) classes. This will require some thinking through of the pathways, and you will probably need to fetch data AGAIN throughout the flow, but at least each point is made explicit and can be optimized.

When the required data crosses over more than one ActiveRecord, or is sufficiently complicated, do the data fetch in a separate class - one per usecase. I like to call these 'query' objects.

Data Structure

The code should not duplicate knowledge about data structure, but instead try to isolate each structure access into one place.

For example, you call "self.attempts.first.answer.question.stage" which means the code in quiz knows about attempts, that the first one is special (I think it's current, right?), that attempts have answers, that answers have questions, and the question has a stage.

Not only is it a lot to take in when reading it, it means that if anything around the structure changes you'll have lots of places to change.

It's also doing lots of SQL at each '.', and you can't optimize it without having to change each line. For example, we can probably optimize by using a number of joins via combining 'scopes'.

Other Points

In update_user_mission_and_stage

  • Try and reduce the branching if possible. Don't do if/else within if/else.

One way to fix this is with a 'state' machine.

  • Avoid callbacks like the plague -- it's so easy to lose track of them, and to not realise how much code is running unexpectedly. Limit them to maybe simple lookups for the core model in the controller.

You have 'check_answer' being called in 'before_save' of quiz, but it doesn't save anything, so appears to have no effect.

  • Extract out the entire method to a separate 'service' class. That's just a plain ruby class to do the logic.

Read this for some more details/links/ideas: http://blog.codeclimate.com/blog/2012/10/17/7-ways-to-decompose-fat-activerecord-models/

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.