CSC/ECE 517 Fall 2019 - Project E1947. Refactor quiz questionnaire controller.rb

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

• to-do

Drawbacks and Solutions

• to-do

Code improvements

• The method save_changes had a congnitive complexity of _ and

orted

• 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

Automated Testing using RSPEC

Testing from Travis CI

Project Mentors

Edward Gehringer (efg@ncsu.edu)

Akansha Mohan (amohan7@ncsu.edu)

Team Members

Shalin Rathi (sjrathi@ncsu.edu)

Akshay Saxena (asaxena5@ncsu.edu)

Daniel Mendez (dmendez@ncsu.edu)

References

1. Expertiza on GitHub
2. GitHub Project Repository Fork
3. The live Expertiza website
4. Demo link
5. Expertiza project documentation wiki
6. Rspec Documentation
7. Clean Code: A handbook of agile software craftsmanship. Author: Robert C Martin