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. Refactor create method ( usage of local variable instead of instance variables)
3. QuizQuestionnaire check

Files Changed

• app/controllers/questionnaires_controller.rb
• spec/controllers/questionnaires_controller_spec.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 instance variable @questionnaire is converted to local questionnaire object to reduce its scope. Since in any other method @questionnaire object is initialized again.

  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
        questionnaire.private = questionnaire_private
        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]
        # 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
        questionnaire.display_type = display_type
        questionnaire.instruction_loc = Questionnaire::DEFAULT_QUESTIONNAIRE_URL
        questionnaire.save
        # 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!'
      rescue StandardError
        flash[:error] = $ERROR_INFO
      end
      redirect_to controller: 'questionnaires', action: 'edit', id: questionnaire.id
    end
  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('%') 
 

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