3
\$\begingroup\$

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
\$\endgroup\$
1
\$\begingroup\$
  • 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(...)

| improve this answer | |
\$\endgroup\$
1
\$\begingroup\$

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
| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.