Nsryjdtyk

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

• 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(...)

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