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

Question

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

Answer 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/