CSC/ECE 517 Fall 2022 - E2257: Refactor questionnaires controller.rb

Introduction

In Expertiza, Questionnaire is the superclass for all kinds of questionnaires and rubrics—rubrics for evaluating submissions and teammate contributions, and taking quizzes and surveys. All of these are subclasses of Questionnaire. questionnaires_controller.rb is charged with creating, displaying, and managing Questionnaires. Because it is used so widely, malfunctions could cause bugs in many parts of the system; hence it is especially important that the code be clear.

Issues Fixed

1. Removed create_questionnaire
2. Split create method into smaller self-documenting methods.
3. QuizQuestionnaire check
4. Refactor add_new_questions method
5. Remove redundant variables in QuestionnairesController

Files Changed

• app/controllers/questionnaires_controller.rb
• spec/controllers/questionnaires_controller_spec.rb
• app/models/question.rb

Changes

Fix #1

The create_questionnnaires method was present but its related functionality had been removed. Some kind of link to it is still present in one of the initial migrations which was populating a database to show values for a drop-down that is no longer in use. Thus ended up removing the function and its corresponding test cases.

Fix #2

The create method itself is 49 lines long. This is much too long to be viewable at a glance. Thus the method was broken into adding_question_variables and create_node method so that they are clear enough to be self-documenting.

   def create
    if params[:questionnaire][:name].blank?
      flash[:error] = 'A rubric or survey must have a title.'
      redirect_to controller: 'questionnaires', action: 'new', model: params[:questionnaire][:type], private: params[:questionnaire][:private]
    else
      questionnaire_private = params[:questionnaire][:private] == 'true'
      display_type = params[:questionnaire][:type].split('Questionnaire')[0]
      begin
        @questionnaire = Object.const_get(params[:questionnaire][:type]).new if Questionnaire::QUESTIONNAIRE_TYPES.include? params[:questionnaire][:type]
      rescue StandardError
        flash[:error] = $ERROR_INFO
      end
      begin
        # Zhewei: Right now, the display_type in 'questionnaires' table and name in 'tree_folders' table are not consistent.
        # In the future, we need to write migration files to make them consistency.
        # E1903 : We are not sure of other type of cases, so have added a if statement. If there are only 5 cases, remove the if statement
        if %w[AuthorFeedback CourseSurvey TeammateReview GlobalSurvey AssignmentSurvey BookmarkRating].include?(display_type)
          display_type = display_type.split(/(?=[A-Z])/).join('%')
        end
        # setting the object variables
        adding_question_variables(questionnaire_private, display_type)
        # Create node
        create_node()
      rescue StandardError
        flash[:error] = $ERROR_INFO
      end
      redirect_to controller: 'questionnaires', action: 'edit', id: @questionnaire.id
    end
  end

---

  # Assigns corrresponding variables to questionnaire object.
  def adding_question_variables(prv,display)
    @questionnaire.private = prv
    @questionnaire.name = params[:questionnaire][:name]
    @questionnaire.instructor_id = session[:user].id
    @questionnaire.min_question_score = params[:questionnaire][:min_question_score]
    @questionnaire.max_question_score = params[:questionnaire][:max_question_score]
    @questionnaire.type = params[:questionnaire][:type]
    @questionnaire.display_type = display
    @questionnaire.instruction_loc = Questionnaire::DEFAULT_QUESTIONNAIRE_URL
    @questionnaire.save
  end
 

---

  # Creates tree node
  def create_node()
    tree_folder = TreeFolder.where(['name like ?', @questionnaire.display_type]).first
    parent = FolderNode.find_by(node_object_id: tree_folder.id)
    QuestionnaireNode.create(parent_id: parent.id, node_object_id: @questionnaire.id, type: 'QuestionnaireNode')
    flash[:success] = 'You have successfully created a questionnaire!'
  end
 

Fix #3

There are three checks for whether a questionnaire is a QuizQuestionnaire. Testing the class of an object is always suspect, though, in a controller, which has to control a whole hierarchy of objects, it is not necessarily wrong. However, they probably aren’t necessary. 2 of them seem to have been removed already and the only remaining check was in create_questionnaire, thus it got removed while applying Fix #1.

Fix #4

According to the code climate, it was suggested that usage of each_key is preferable instead of keys.each . Rubocop suggests this due to performance. Using large sets of data is where this becomes noticeable. Chaining methods is going to be slower than using the built-in method (in this case) which accomplishes the task with a single special enumerator.

    params[:question].keys.each do |question_key|
 

    params[:question].each_key do |question_key|
 

Fix #5

According to the code climate, it was suggested that the use of parentheses around a method call should be avoided.

    display_type = (display_type.split(/(?=[A-Z])/)).join('%') 
 

    display_type = display_type.split(/(?=[A-Z])/).join('%') 
 

Fix #6

The add_new_questions method has string literal values like 'Strongly agree', 'Strongly disagree', '50, 3', '0|1|2|3|4|5' for max_label, min_label, size and alternatives. The literals are removed by introducing constants in question.rb.

      if question.is_a? ScoredQuestion
        question.weight = params[:question][:weight]
        question.max_label = 'Strongly agree'
        question.min_label = 'Strongly disagree'
      end

      question.size = '50, 3' if question.is_a? Criterion
      question.size = '50, 3' if question.is_a? Cake
      question.alternatives = '0|1|2|3|4|5' if question.is_a? Dropdown
      question.size = '60, 5' if question.is_a? TextArea
      question.size = '30' if question.is_a? TextField

 

      if question.is_a? ScoredQuestion
        question.weight = params[:question][:weight]
        question.max_label = Question::MAX_LABEL
        question.min_label = Question::MIN_LABEL
      end

      if Question::SIZES.key?(question.class.name)
        question.size = Question::SIZES[question.class.name]
      end
      if Question::ALTERNATIVES.key?(question.class.name)
        question.alternatives = Question::ALTERNATIVES[question.class.name]
      end
 

Fix #7

The add_new_questions and save_all_questions methods have reduntant variable defined called questionnaire_id. The variable declaration is redundant as we can access it using params[:id].

     questionnaire_id = params[:id] unless params[:id].nil?
     num_of_existed_questions = Questionnaire.find(questionnaire_id).questions.size
 

     num_of_existed_questions = Questionnaire.find(params[:id]).questions.size
 

Future scope

• Have a closer look at the following issue: Assignment Branch Condition size for save_all_questions is too high.
  

save_all_questions has params[:view_advice] as another if statement not sure why this is here or what it corresponds to, the application seems to have a ‘view advice’ button on the questionnaire edit page, but clicking it throws the following error: No route matches [GET] "/advice/edit_advice". Hence not sure how to proceed here.

• Bad if statement. Line 190: if question.is_a? ScoredQuestion.

Need to have a closer look at the class higher archy and understand how this can be refactored.

Project Mentor

Edward Gehringer (efg@ncsu.edu)

Team Member

Ashwin Shankar Umasankar (aumasan@ncsu.edu)

Kailash Singaravelu (ksingar2@ncsu.edu)

Pujitha Enamandala (penaman@ncsu.edu)

Reference

1. VCL link
2. Github Repo
3. Github Pull Request
4. Expertiza on GitHub
5. Expertiza Project Documentation Wiki
6. CodeClimate Documentation