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