Rails controller for a vocabulary quiz

Question

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

Answer 1

• What is the point of clear_session it is never called without also calling initiate_quiz yet repeats the same code.

• I would define a constant for the number of questions (or pass it as a parameter) rather than repeating it.

• Usually I expect a method with a noun name like remaining_words to return the remaining words. Since this method does other things I would call it something else maybe get_words or initialize_words

• I also avoid doing redirects inside methods that are doing calculations. i.e. I would move the redirect out of remaining_words

• save_score_to_db could be turned into a single line using Score.create(...)

Answer 2

1) Single Responsibility Principle is very important but checkout other principles too: SOLID

2) Not a fan of concerns, so wouldn't recommend them. There are very good posts about their downsides (just Google it).

3) Design patterns are useful. However, I wouldn't recommend starting with them. If you have duplications in your controllers/models (or high complexity), then you can think about implementing them. Check out 7 Patterns to Refactor Fat ActiveRecord Models' Form Objects section.

4) If most part of your application requires signing-in, move require_login before hook to ApplicationController and skip it when necessary.

5) Most importantly, make your controllers CRUD. Example:

class AnswersController < ApplicationController
 def create
 # Keep track of score and questions already asked
 if params[:answer] == params[:orig]
 right_answer
 else
 wrong_answer
 end
 redirect_to quiz_path
 end
end