Dummy Url2

E1947. Refactoring quiz_questionnaire_controller.rb

This page provides a description of the Expertiza based OSS project.

About Expertiza

Expertiza is an open source project based on Ruby on Rails framework. Expertiza allows the instructor to create new assignments and customize new or existing assignments. It also allows the instructor to create a list of topics the students can sign up for. Students can form teams in Expertiza to work on various projects and assignments. Students can also peer review other students' submissions. Expertiza supports submission across various document types, including the URLs and wiki pages.

Problem Statement

The following tasks were accomplished in this project:

• Rename filename such that it follows the coding standards
• Remove redundant information from method names
• Add comments to methods and complex lines of code
• Remove unsafe reflection method
• Simplify RSpec
• Try to fix issues from Code Climate

About Quiz Questionnaire Controller

This is a new controller, having recently been separated from questionnaires_controller.rb. It is used to allow students to take quizzes. The idea is that the author(s) of submitted work can write a quiz that is given to each reviewer before the reviewer is allowed to review the work. If the reviewer does badly on the quiz, then we know not to trust the review. It is also possible to set up an Expertiza assignment so that some participants just take the quiz and don’t review the work.

Current Implementation

Functionality

• A student can create a quiz questionnaire for the assignments that allow a quiz to be generated. This quiz will be taken by the reviewers, once they are done reviewing the development/project. Based on their answers the TA's can know whether to consider the reviewer's review or not based , because if the reviewer didn't perform well on the quiz, then it is likely that the review is not done properly.

• The maximum number of questions is decided by the instructor when creating the assignment.

• The student has option to either create True/False, Multiple-Choice Checkbox or Multiple-Choice Radio question.

• The question cannot be blank and correct answer must be selected to create a quiz.

Drawbacks and Solutions

• Problem 1: Rename filename such that it follows the coding standards
  • Solution: Currently the file was named as quiz_questionnaire_controller.rb. This doesn't follow the Ruby naming standards. Hence it was changed to quiz_questionnaires_controller.rb
• Problem 2: Remove redundant information from method names
  • Solution: Since this controller was separated from the questionnaires_controller.rb, the method names were kept the same from the previous controller which had redundant information. Method names were renamed by removing the succeeding '_quiz'. Also valid_quiz was renamed to validate_quiz to make more sense. Also the routes.rb was changed to accept these changes.
• Problem 3: Add comments to methods and complex lines of code
  • Solution: Many methods had more than 325 lines of code and performed multiple functionalities. These methods were changed by creating private methods that performed individual tasks, thus reducing the lines of code and the complexity.
• Problem 4: Remove unsafe reflection method
  • Solution: This part was already implemented, hence no changes were made. Method questionnaire_params was already defined and takes care of this issue.
• Problem 5: Fix issues from Code Climate
  • Solution: Refer to Solution for Problem 3

Code improvements

• The method save_choices had a congnitive complexity of 23 and had 37 lines of code. By refactoring, the number of lines was reduced to 20 and private methods create_checkbox, create_truefalse and create_radio were implemented to break up functionality and to reduce cognitive complexity.

Before:

def save_choices(questionnaire_id)
    return unless params[:new_question] or params[:new_choices]
    questions = Question.where(questionnaire_id: questionnaire_id)
    question_num = 1

    questions.each do |question|
      q_type = params[:question_type][question_num.to_s][:type]
      params[:new_choices][question_num.to_s][q_type].each_key do |choice_key|
        if q_type == "MultipleChoiceCheckbox"
          q = if params[:new_choices][question_num.to_s][q_type][choice_key][:iscorrect] == 1.to_s
                QuizQuestionChoice.new(txt: params[:new_choices][question_num.to_s][q_type][choice_key][:txt], iscorrect: "true", question_id: question.id)
              else
                QuizQuestionChoice.new(txt: params[:new_choices][question_num.to_s][q_type][choice_key][:txt], iscorrect: "false", question_id: question.id)
              end
          q.save
        elsif q_type == "TrueFalse"
          if params[:new_choices][question_num.to_s][q_type][1.to_s][:iscorrect] == choice_key
            q = QuizQuestionChoice.new(txt: "True", iscorrect: "true", question_id: question.id)
            q.save
            q = QuizQuestionChoice.new(txt: "False", iscorrect: "false", question_id: question.id)
            q.save
          else
            q = QuizQuestionChoice.new(txt: "True", iscorrect: "false", question_id: question.id)
            q.save
            q = QuizQuestionChoice.new(txt: "False", iscorrect: "true", question_id: question.id)
            q.save
          end
        else
          q = if params[:new_choices][question_num.to_s][q_type][1.to_s][:iscorrect] == choice_key
                QuizQuestionChoice.new(txt: params[:new_choices][question_num.to_s][q_type][choice_key][:txt], iscorrect: "true", question_id: question.id)
              else
                QuizQuestionChoice.new(txt: params[:new_choices][question_num.to_s][q_type][choice_key][:txt], iscorrect: "false", question_id: question.id)
              end
          q.save
        end
      end
      question_num += 1
      question.weight = 1
    end
  end

After:

def save_choices(questionnaire_id)
    return unless params[:new_question] or params[:new_choices]
    questions = Question.where(questionnaire_id: questionnaire_id)
    question_num = 1

    questions.each do |question|
      q_type = params[:question_type][question_num.to_s][:type]
      q_choices = params[:new_choices][question_num.to_s][q_type]
      q_choices.each_key do |choice_key|
        if q_type == "MultipleChoiceCheckbox"
          create_checkbox(question, choice_key, q_choices)
        elsif q_type == "TrueFalse"
          create_truefalse(question, choice_key, q_choices)
        else # MultipleChoiceRadio
          create_radio(question, choice_key, q_choices)
        end
      end
      question_num += 1
      question.weight = 1
    end
  end

• The method action_allowed? has been implemented in quiz_questionnaire_controller.rb to specifically allow Student to edit an existing quiz.

• The quiz_questionnaire_controller_spec.rb has been updated to allow edit tests to work for Student role.

let(:student) { build(:student, id: 8609) }

Before:

    context 'when current questionnaire is not taken by anyone' do
      it 'renders questionnaires#edit page' do
        allow(@questionnaire).to receive(:taken_by_anyone?).and_return(false)
        params = {id: 1}
        get :edit, params
        expect(response).to render_template(:edit)
      end
    end

After:

    context 'when current questionnaire is not taken by anyone' do
      it 'renders questionnaires#edit page' do
        stub_current_user(student, student.role.name, student.role) #action only permitted for Student role
        allow(@questionnaire).to receive(:taken_by_anyone?).and_return(false)
        params = {id: 1}
        get :edit, params
        expect(response).to render_template(:edit)
      end
    end

Before:

    context 'when current questionnaire has been taken by someone' do
      it 'shows flash[:error] message and redirects to submitted_content#view page' do
        allow(@questionnaire).to receive(:taken_by_anyone?).and_return(true)
        params = {id: 1, pid: 1}
        get :edit, params
        expect(flash[:error]).to eq('Your quiz has been taken by some other students, you cannot edit it anymore.')
        expect(response).to redirect_to('/submitted_content/view?id=1')
      end
    end
  end

After:

    context 'when current questionnaire has been taken by someone' do
      it 'shows flash[:error] message and redirects to submitted_content#view page' do
        stub_current_user(student, student.role.name, student.role)  #action only permitted for Student role
        allow(@questionnaire).to receive(:taken_by_anyone?).and_return(true)
        params = {id: 1, pid: 1}
        get :edit, params
        expect(flash[:error]).to eq('Your quiz has been taken by some other students, you cannot edit it anymore.')
        expect(response).to redirect_to('/submitted_content/view?id=1')
      end
    end
  end

• The method update_quiz has been renamed to update and has been refactored to reduce the line count by breaking functionalities. (The function multiple_choice_checkbox was renamed to update_checkbox and multiple_choice_radio was renamed to update_radio so as to reduce function name length and bring consistency with methods performing create operations: create_radio, create_checkbox, create_truefalse)

The Cognitive Complexity of update method has now been reduced to 15 (from 29).
Before:

 def update
    @questionnaire = Questionnaire.find(params[:id])
    if @questionnaire.nil?
      redirect_to controller: 'submitted_content', action: 'view', id: params[:pid]
      return
    end
    if params['save'] && params[:question].try(:keys)
      @questionnaire.update_attributes(questionnaire_params)

      params[:question].each_key do |qid|
        @question = Question.find(qid)
        @question.txt = params[:question][qid.to_sym][:txt]
        @question.save

        @quiz_question_choices = QuizQuestionChoice.where(question_id: qid)
        question_index = 1
        @quiz_question_choices.each do |question_choice|
          # Call to private method to handle  Multile Choice Questions
          multiple_choice_checkbox(question_choice, question_index) if @question.type == "MultipleChoiceCheckbox"
          multiple_choice_radio(question_choice, question_index) if @question.type == "MultipleChoiceRadio"
          if @question.type == "TrueFalse"
            if params[:quiz_question_choices][@question.id.to_s][@question.type][1.to_s][:iscorrect] == "True" # the statement is correct
              question_choice.txt == "True" ? question_choice.update_attributes(iscorrect: '1') : question_choice.update_attributes(iscorrect: '0')
              # the statement is correct so "True" is the right answer
            else # the statement is not correct
              question_choice.txt == "True" ? question_choice.update_attributes(iscorrect: '0') : question_choice.update_attributes(iscorrect: '1')
              # the statement is not correct so "False" is the right answer
            end
          end
          question_index += 1
        end
      end
    end
    redirect_to controller: 'submitted_content', action: 'view', id: params[:pid]
  end

After:

  def update
    @questionnaire = Questionnaire.find(params[:id])
    if @questionnaire.nil?
      redirect_to controller: 'submitted_content', action: 'view', id: params[:pid]
      return
    end
    if params['save'] && params[:question].try(:keys)
      @questionnaire.update_attributes(questionnaire_params)
      params[:question].each_key do |qid|
        @question = Question.find(qid)
        @question.txt = params[:question][qid.to_sym][:txt]
        @question.save
        @quiz_question_choices = QuizQuestionChoice.where(question_id: qid)
        question_index = 1
        @quiz_question_choices.each do |question_choice|
          # Call to private method to handle  Multile Choice Questions
          update_checkbox(question_choice, question_index) if @question.type == "MultipleChoiceCheckbox"
          update_radio(question_choice, question_index) if @question.type == "MultipleChoiceRadio"
          update_truefalse(question_choice) if @question.type == "TrueFalse"
          question_index += 1
        end
      end
    end
    redirect_to controller: 'submitted_content', action: 'view', id: params[:pid]
  end

  def update_truefalse(question_choice)
    if params[:quiz_question_choices][@question.id.to_s][@question.type][1.to_s][:iscorrect] == "True" # the statement is correct
      question_choice.txt == "True" ? question_choice.update_attributes(iscorrect: '1') : question_choice.update_attributes(iscorrect: '0')
      # the statement is correct so "True" is the right answer
    else # the statement is not correct
      question_choice.txt == "True" ? question_choice.update_attributes(iscorrect: '0') : question_choice.update_attributes(iscorrect: '1')
      # the statement is not correct so "False" is the right answer
    end
  end

• The method valid_quiz has been renamed to validate_quiz. The function validate_question was extracted out to only validate the particular question. The part where quiz name was being verified was also taken out of the loop to only verify this once as opposed to verifying in each iteration, which was unnecessary. The Cognitive Complexity of 12 has now been reduced to less than 5.

Before:

  def valid_quiz
    num_questions = Assignment.find(params[:aid]).num_quiz_questions
    valid = "valid"

    (1..num_questions).each do |i|
      if params[:questionnaire][:name] == "" # questionnaire name is not specified
        valid = "Please specify quiz name (please do not use your name or id)."
      elsif !params.key?(:question_type) || !params[:question_type].key?(i.to_s) || params[:question_type][i.to_s][:type].nil?
        # A type isnt selected for a question
        valid = "Please select a type for each question"
      else
        @new_question = Object.const_get(params[:question_type][i.to_s][:type]).create(txt: '', type: params[:question_type][i.to_s][:type], break_before: true)
        @new_question.update_attributes(txt: params[:new_question][i.to_s])
        type = params[:question_type][i.to_s][:type]
        choice_info = params[:new_choices][i.to_s][type] # choice info for one question of its type
        valid = if choice_info.nil?
                  "Please select a correct answer for all questions"
                else
                  @new_question.isvalid(choice_info)
                end
      end
      break if valid != "valid"
    end
    valid
  end

After:

  def validate_quiz
    num_questions = Assignment.find(params[:aid]).num_quiz_questions
    valid = "valid"
    if params[:questionnaire][:name] == "" # questionnaire name is not specified
      valid = "Please specify quiz name (please do not use your name or id)."
    end
    (1..num_questions).each do |i|
      break if valid != "valid"
      valid = validate_question(i)
    end
    valid
  end

  def validate_question(i)
    if !params.key?(:question_type) || !params[:question_type].key?(i.to_s) || params[:question_type][i.to_s][:type].nil?
      # A type isn't selected for a question
      valid = "Please select a type for each question"
    else
      # The question type is dynamic, so const_get is necessary
      type = params[:question_type][i.to_s][:type]
      @new_question = Object.const_get(type).create(txt: '', type: type, break_before: true)
      @new_question.update_attributes(txt: params[:new_question][i.to_s])
      choice_info = params[:new_choices][i.to_s][type] # choice info for one question of its type
      valid = if choice_info.nil?
                "Please select a correct answer for all questions"
              else
                @new_question.isvalid(choice_info)
              end
    end
    valid
  end

Testing

Link to video showing the RSpec testing.

Automated Testing using RSPEC

Testing from Travis CI