CSC/ECE 517 Fall 2019 - E1946. Refactor Questionnaire controller

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.

About Questionnaire Controller

Questionnaire is the superclass for all kinds of questionnaires and rubrics, rubrics is used for evaluating submissions and teammate contributions, and taking quizzes and surveys. All of these are subclasses of Questionnaire. This controller is used for creating, displaying, and managing Questionnaires. Different type of questionnaire can be created in Expertiza like Review, Metareview, Teammate review, Quiz Questionnaire, Global Survey, Course Survey and many more. This Controller is widely used and malfunctions can cause many issues in different parts of the system.

Below is screenshot of questionnaire on expertiza. It shows all the kinds of questionnaire we can create on expertiza.

Below is screenshot of all the review questionnaire we have created till now. We can edit, delete and do many other operations on questionnaire.

Below is screenshot which shows how we can edit any questionnaire.

Why Refactor Questionnaire Controller

In Questionnaire controller below are some of the problems which needs refactoring:

• Renaming the methods breaking Rubys naming conventions.
• Refactoring the methods doing multiple tasks and breaking them in different modules.
• Reducing the length of methods longer than 50 lines of code.
• Removing the string literal values used in controller.
• Separating the business logic from controller and putting it in method.
• Remove variables behaving same as ruby's given functionality.
• Commenting the code properly for easy understanding.

Files Modified

Below are the five file we changed as part of refactoring

Questionnaire Controller

This is the questionnaires_controller.rb which handles all the CRUD operation performed on any kind of questionnaire - review or surveys, add weightage to all questions, selecting type of questions. This controller holds the major part of work done for creating questions. Creating of questionnaire was one method which was extremely complicated and not well commented about what is going on. Multiple things were done in just one method which we broke in modules doing that specific task. This controller was holding the business logic as well, which we can move to model. Variables were created separate to get value from UI. At many places string literals were being hardcoded, which were made as constants.

Questionnaire Model

This is the questionnaire.rb model file and we have added self method in it, which will create questionnaire object for us and we will get that object in controller and process it in controller.

Questionnaire Spec

This is the questionnaire_controller_spec.rb file and we have changed this file in order to test the changes we have made in Questionnaire controller.

Quiz Questionnaire Controller

This file is quiz_questionnaire_controller.rb and was calling multiple methods of quiz questionnaire by passing questionnaire id as parameter which we have removed.

Quiz Questionnaire Spec

This is quiz_questionnaire_spec.rb was changed as we have removed questionnaire ID from methods in Quiz Questionnaire controller.

User Stories

We have used Github project board to keep track of work that needs to be refactored. Below were the user stories that we have created:

• US1 - As a developer i want to analyse create questionnaire so that i can refactor and break it into modules.
• US2 - As a developer i want to refactor the create_questionnaire method by giving it meaningful name and improving code so that it does not conflict with ruby naming standard.
• US3 - As a developer i want to remove questionnaire_id with params[:id] so that we can remove unnecessary variables.
• US4 - As a developer i want to remove all hardcoded values of alternatives, min label and max label and put them in literals, so that code can be aligned to coding standards.
• US5 - As a developer i want to analyse why Questionnaire is being checked as QuizQuestionnaire multiple times and functionality is being changed, so that we can make quiz questionnaire same as rest.
• US6 - As a developer i want to analyse why questionnaire_id has been passed as parameter in many question so that i can refactor the code.
• US7 - As a developer i want to run rspecs in Questionnaire spec to so that i can make sure the changes made are working correct.
• US8 - As a developer i want to run rspecs in Quiz Questionnaire spec to so that i can make sure the changes made are working correct.

Refactoring

Refactor Create Method

This method was in questionnaires_controller.rb Create method was very long in Questionnaire controller and was not self explanatory, number of characters in one line is more than 80 and cant be seen in one window. Before Refactoring

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].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

After Refactoring

In this user story we created self method in questionnaire model which will create questionnaire object and pass it in controller. Self method was divided in to two different modules which we then called into self.

Method create_questionnaire_node(questionnaire) - This method will create a Tree node for new questionnaire in Expertiza

 # This method is used to create Treenode for newly created questionnaire
    def create_questionnaire_node(questionnaire)
      tree_folder = TreeFolder.where(['name like ?', questionnaire.display_type]).first

      if tree_folder.present?
        parent = FolderNode.find_by(node_object_id: tree_folder.id)
        QuestionnaireNode.create(parent_id: parent.id, node_object_id: questionnaire.id, type: 'QuestionnaireNode')
      end
    end

Method display_type_for_questionnaire(params) - This method will tell us about display type for new questionnaire in Expertiza

# Displaying the newly created questionnaire
    def display_type_for_questionnaire(params)
      display_type = params[:questionnaire][:type].split('Questionnaire')[0]

      if %w[AuthorFeedback CourseSurvey TeammateReview GlobalSurvey AssignmentSurvey].include?(display_type)
        display_type = (display_type.split /(?=[A-Z])/).join("%")
      end

      display_type
    end

This method below will be the main method for creating the questionnaire object and will call the two method we have written above. Self - Assigning all variable values from UI into questionnaire object + create_questionnaire_node(questionnaire) + display_type_for_questionnaire(params)

# This method will create a questionnaire object and will be called from controller. 
class << self
    def create_new_questionnaire_obj(params, session)
      # Assigning values passed from UI in params[:id] to questionnaire object
      if Questionnaire::QUESTIONNAIRE_TYPES.include? params[:questionnaire][:type]
        questionnaire = Object.const_get(params[:questionnaire][:type]).new 
        questionnaire.private = params[:questionnaire][:private] == 'true'
        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.
        questionnaire.display_type = display_type_for_questionnaire(params)
        questionnaire.instruction_loc = Questionnaire::DEFAULT_QUESTIONNAIRE_URL

        if questionnaire.save
          create_questionnaire_node(questionnaire)
        end
        # returning the questionnaire object to calling method create in controller
        questionnaire
      end
    end

Refactor Create Questionnaire Method

This method was in questionnaires_controller.rb and in this user story after deep analysis of create_questionnaire method, we found that this method has not been called anywhere so we have removed this method from the questionnaire controller.

Remove Unnecessary Variables

As a part of this user story we have removed unnecessary variables in questionnaires_controller.rb file. In this file methods like add_new_questions, save_all_questions, save_questions, save_new_questions, delete_questions, local variable questionnaire_id was passed as a method argument which is not actually required since this value can be fetched from class variable @questionnaire.id or from params[:id]. So, this variable was unnecessary. Below is the one of the methods named save_all_questions which was using questionnaire_id as method parameter and we refactored.

Before Refactoring

 # Zhewei: This method is used to save all questions in current questionnaire.
  def save_all_questions
    questionnaire_id = params[:id]
    begin
      if params[:save]
        params[:question].each_pair do |k, v|
          @question = Question.find(k)
          # example of 'v' value
          # {"seq"=>"1.0", "txt"=>"WOW", "weight"=>"1", "size"=>"50,3", "max_label"=>"Strong agree", "min_label"=>"Not agree"}
          v.each_pair do |key, value|
            @question.send(key + '=', value) if @question.send(key) != value
          end

          @question.save
          flash[:success] = 'All questions have been successfully saved!'
        end
      end
    rescue StandardError
      flash[:error] = $ERROR_INFO
    end

    if params[:view_advice]
      redirect_to controller: 'advice', action: 'edit_advice', id: params[:id]
    elsif !questionnaire_id.nil?
      redirect_to edit_questionnaire_path(questionnaire_id.to_sym)
    end
  end

After Refactoring

def save_all_questions
    begin
      if params[:save]
        params[:question].each_pair do |k, v|
          @question = Question.find(k)
          # example of 'v' value
          # {"seq"=>"1.0", "txt"=>"WOW", "weight"=>"1", "size"=>"50,3", "max_label"=>"Strong agree", "min_label"=>"Not agree"}
          v.each_pair do |key, value|
            @question.send(key + '=', value) if @question.send(key) != value
          end

          @question.save
          flash[:success] = 'All questions have been successfully saved!'
        end
      end
    rescue StandardError
      flash[:error] = $ERROR_INFO
    end

    if params[:view_advice]
      redirect_to controller: 'advice', action: 'edit_advice', id: params[:id]
    elsif params[:id].present?
      redirect_to edit_questionnaire_path(params[:id])
    end
  end

Remove Hardcoded String Literals

As a part of this user story we have made changes in questionnaires_controller.rb file. Few methods have values directly hardcoded in code, which does not justify good coding standard. To clean up this code we used constant variables which also clearly mention what value is being set by this variable. Refactoring was done in method like add_new_questions.

Before Refactoring

 # Zhewei: This method is used to add new questions when editing questionnaire.
  def add_new_questions
    questionnaire_id = params[:id] unless params[:id].nil?
    num_of_existed_questions = Questionnaire.find(questionnaire_id).questions.size
    ((num_of_existed_questions + 1)..(num_of_existed_questions + params[:question][:total_num].to_i)).each do |i|
      question = Object.const_get(params[:question][:type]).create(txt: '', questionnaire_id: questionnaire_id, seq: i, type: params[:question][:type], break_before: true)
      if question.is_a? ScoredQuestion
        question.weight = 1
        question.max_label = 'Strongly agree'
        question.min_label = 'Strongly disagree'
      end
      question.size = '50, 3' if question.is_a? Criterion
      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
      begin
        question.save
      rescue StandardError
        flash[:error] = $ERROR_INFO
      end
    end
    redirect_to edit_questionnaire_path(questionnaire_id.to_sym)
  end

After Refactoring

Constants created were:

QUESTION_MIN_SCORE = 0
QUESTION_MAX_SCORE = 1
QUESTION_MAX_LABEL = 'Strongly agree'
QUESTION_MIN_LABEL = 'Strongly disagree'
QUESTION_CRITERION_SIZE = '50, 3'
DROPDOWN_SCALE = '0|1|2|3|4|5'
TEXT_AREA_SIZE = '60, 5'
TEXT_FIELD_SIZE = '30'

We refactored the above method by making the constants and using them instead of just passing literals. We have also improved the logic by using the better conditions and variables in method.

 # Zhewei: This method is used to add new questions when editing questionnaire.
  def add_new_questions
    num_of_existed_questions = Questionnaire.find(params[:id]).questions.size
    question_nums =params[:question][:total_num].to_i
    total_question_nums = ((num_of_existed_questions + 1)..(num_of_existed_questions + question_nums))

    total_question_nums.each do |i|
      question = Object.const_get(params[:question][:type]).create(
          txt: '',
          questionnaire_id: params[:id],
          seq: i,
          type: params[:question][:type],
          break_before: true
      )
      if question.is_a? ScoredQuestion
        question.weight = MAXIMUM_QUESTION_SCORE
        question.min_label = QUESTION_MIN_LABEL
        question.max_label = QUESTION_MAX_LABEL
      end
      
      question.alternatives = DROPDOWN_SCALE if question.is_a? Dropdown
      question.size = question_size(question)
      begin
        question.save
      rescue StandardError
        flash[:error] = $ERROR_INFO
      end
    end
    redirect_to edit_questionnaire_path(params[:id].to_sym)
  end

Remove Unnecessary Parameters

This change was done in Quiz_questionnaire.rb file. Questionnaire_id is the variable which has value of params[:id], this variable was passed as method parameter in many calls when call quiz questionnaire controller.

Remove Unnecessary Checks

In few methods like save_questions checks like “@questionnaire.id if !@questionnaire.id.nil? and @questionnaire.id > 0” are added. These kind of checks are unnecessary because since value of questionnaire.id will be present is @questionnaire class variable. To clear up such code we have removed unnecessary check.

Update Questionnaire Spec

As mentioned in US2, we have removed create_questionnaire method and we have also removed corresponding test of that method in spec file. Rest no changes were done in spec file. We had run the tests before and after refactoring and they have passed.

Update Quiz Questionnaire Spec

As mentioned in US3 we have removed unnecessary variables, one of the variable was questionnaire_id which was just params[:id]. This variable was passed as parameter in many of quiz questionnaire methods. we have removed this variable and method calls are changed. We have changed test cases also of all the corresponding method.

Testing

Our task was refactoring the controller so we have mainly focused on to pass the tests which are already written. In the refactoring we have added the methods that are private so according the Messages sent to self should not be tested behaviour we have not added extra tests.

LoopHoles

• In several places begin and rescue statement are used without properly understanding the functionality. Due to this critical error messages are lost and debugging those error would be difficult. To fix this proper error log is added and begin rescue is removed.
• On editing any questionnaire, they are not getting updated. Values are being hardcoded at backend and questionnaire never gets updated. Issue#1550 has been created to fix this bug.

Team

Mentor

Prof. Edward Gehringer

Members

• Garima Garima, Ggarima
• Bharat Bhardwaj, Bbhardw
• Piyush Tiwari, Ptiwari

References

1. Expertiza
2. Expertiza GitHub
3. Pull Request to Merge
4. Deployed Changes Userid - instructor6 and Password - password
5. GitHub Project Board