Refactoring a Single Rails Model with large methods & long join queries trying to do everything
- by Kelseydh
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:
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?
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
