I'm new to refactoring and I have just refactored an old Rails project that had bloated controller methods.
I spun off the bloated methods into its own separate controller following this logic, since the quiz functionality can be considered a separate resource compared to the vocab controller's responsibilities (Single Responsibility Principle).
Please critique my refactored version:
- Would I have been better off using a design pattern (Service object, Concerns) instead of splitting it into a separate controller?
- Should this be refactored further? Smaller methods?
This was the original code app/controllers/vocabs_controller.rb:
class VocabsController < ApplicationController
before_action :require_login, except: [:index, :quiz, :answer, :result]
def index
@vocabs = Vocab.all.order("word")
session[:score] = nil
session[:already_asked] = nil
end
...
def quiz
initiate_score
end
def answer
#Keep score and question id's already asked
if params[:answer] == params[:orig]
session[:score] += 1
session[:already_asked] << params[:answer].to_i
flash[:notice] = "You got it right!"
redirect_to quiz_path
else
session[:already_asked] << params[:orig].to_i
flash[:notice] = "Sorry, wrong answer!"
redirect_to quiz_path
end
end
def initiate_score
#Initiate score session
session[:score] ||= 0
#Initiate session to hold questions already asked
session[:already_asked] ||= []
#Total score
session[:amount_questions] = Vocab.all.length - 4
#Get list of words that hasn't been asked before
@left_words = Vocab.all.where.not(id: session[:already_asked])
#Questions remaining
@questions_remaining = @left_words.length - 4
#Pick four words from leftover words list
@four = @left_words.shuffle.take(4)
#Create question variable if there are enough words left in list
if @left_words.length >= 4
@question = @four.first.word
else
redirect_to result_path
end
#save score to user database if all questions done and logged in
if @questions_remaining == 0
high_score = Score.new
high_score.user_id = session[:user_id]
high_score.score = session[:score] / session[:amount_questions].to_f
high_score.save
redirect_to result_path
end
end
private
def vocab_params
params.require(:vocab).permit(:word, :definition)
end
end
The refactored version app/controllers/quizzes_controller.rb:
class QuizzesController < ApplicationController
def start_quiz
clear_session
redirect_to action: "quiz"
end
def quiz
initiate_quiz
remaining_words
if @questions_remaining.zero?
save_score_to_db
redirect_to result_path
end
end
def answer
# Keep track of score and questions already asked
if params[:answer] == params[:orig]
right_answer
else
wrong_answer
end
redirect_to quiz_path
end
private
def initiate_quiz
session[:score] ||= 0
session[:vocab_already_asked] ||= []
session[:number_questions_remaining] = Vocab.all.length - 4
end
def remaining_words
@remaining_words = Vocab.all.where.not(id: session[:vocab_already_asked])
@questions_remaining = @remaining_words.length - 4
@quiz_words = @remaining_words.shuffle.take(4)
if @remaining_words.length >= 4
@question = @quiz_words.first.word
else
redirect_to result_path
end
end
def save_score_to_db
high_score = Score.new
high_score.user_id = session[:user_id]
high_score.score = session[:score] / session[:number_questions_remaining].to_f
high_score.save
end
def right_answer
session[:score] += 1
session[:vocab_already_asked] << params[:answer].to_i
flash[:notice] = 'You got it right!'
end
def wrong_answer
session[:vocab_already_asked] << params[:orig].to_i
flash[:notice] = 'Sorry, wrong answer!'
end
def clear_session
session[:score] = 0
session[:vocab_already_asked] = []
end
end