CSC/ECE 517 Spring 2020 / E2001 Refactor Questionnaires Controller

E2001 Refactor questionnaires_controller.rb

This page provides a description of the Expertiza based OSS project

About Expertiza

Expertiza is an open source software project created using Ruby on Rails. Expertiza allows instructors to craft new assignments and edit existing ones. This flexibility ensures that each round of students gets an experience that is appropriate to the given situation. It also allows the instructor to create a list of project options and have the students bid for their favorite project.

While their are a plethora of benefits for instructors, students also gain some benefits when using Expertiza. They are able to form teams and keep track of the past peers they have worked with, and are also able to manage the progress and submission of their assignments.

About Questionnaires Controller

Questionnaire controller is the controller for the Questionnaire object. A Questionnaire can be of several types including the following:

• Review
• Metareview
• Author feedback
• Teammate Review
• Survey
• Assignment
• Global Survey
• Course Survey
• Quiz

Of these several different types of questionnaire's the questions, that can be added, have a different formats including:

• Criterion
• Drop down (multiple choice)
• Text box (short question)
• Text area (long question)

Within the controller itself the object can be copied (clone the given questionnaire), created, viewed, edited, updated, deleted, questions can be added, saved, or removed, and creating a questionnaire node for assignming questionnaire attributes.

The next section explores the database relationships with questionnaires_controller.rb.

Database Relationships

The following diagram visualizes connections in our database. The relationships between relevant tables that are touching questionnaire.rb are shown.

Problem Statement

Background: In Expertiza, Questionnaire is a super-class utilized by all questionnaires, including rubrics, quizzes, and surveys. Rubrics are used to assess things such as project submissions and project teammates. All of the above-mentioned questionnaires are sub-classes of the Questionnaire super-class. Since this super-class is used in a multitude of locations ensuring that there are no issues in the code is very important since an error can cause malfunctions throughout Expertiza.

Problem: Questionnaires controller has been refactored repeatedly over the past few years, yet some improvements can still be made through refactoring to increase the quality and dryness of the code.

These problems are as follows:

• Unsafe reflection method const_get called with parameter value
• Hardwired variables in add_new_questions and save_new_questions
• Unnecessary checks for QuizQuestionnaire, checks can be removed
• Use guard clause to enclose methods instead of conditional
• Removed unnecessary method 'create_questionnaire'
• Break up create method into new 'create_questionnaire'
• Use each_key instead of keys.each
• Split lines of code to fit within recommended 160 character length
• Removed useless assignment of variable in save method
• Resolved issues involving use of unsafe reflection

Files Modified in Project

1. app/controllers/questionnaires_controller.rb
2. spec/controllers/questionnaires_controller_spec.rb

Issues and Improvements

Use guard clause

Problem

Method(s) save_new_questions, delete, and save_questions used conditional to wrap code instead of guard clause. Using guard clause can reduce complexity and number of lines in code.

Solution

For example, we go from:

def save_new_questions(questionnaire_id)
    if params[:new_question]

To:

 def save_new_questions(questionnaire_id)
    return unless params[:new_question]

QuizQuestionnaire checks

Problem

Method(s) create and save_new_questions have unnecessary checks for if question type is QuizQuestionnaire.

Solution

We removed the check in both methods. We go from:

if @questionnaire.type == "QuizQuestionnaire"
 q.weight = 1 # setting the weight to 1 for quiz questionnaire since the model validates this field
end

To:

q.weight = 1

Hardwired Variables

Problem

Method(s) save_new_questions and add_new_questions used hardwired variables. When applicable all values used should be stored as variables so the user knows the purpose of the variable, if that value is used in multiple areas it can be changed with a single change, and it's just messy. For instance both add_new_questions and save_new_questions used the same scalar value 1 for a similar task. We go from:

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

To our code which incorporates constants.

if question.is_a? ScoredQuestion
 question.weight = WEIGHT
 question.max_label = LABEL_AGREE
 question.min_label = LABEL_DISAGREE
end
question.size = SIZE_CRITERION if question.is_a? Criterion
question.alternatives = SIZE_ALT_DROPDOWN if question.is_a? Dropdown
question.size = SIZE_TXT_AREA if question.is_a? TextArea
question.size = SIZE_TXT_FIELD if question.is_a? TextField

With our constants at the top of the code for easy accessibility:

## Constants
# add_new_questions
LABEL_AGREE = 'Strongly agree' # label for scored question if agree
LABEL_DISAGREE = 'Strongly disagree' # label for scored question if disagree
WEIGHT = 1  # question weight
SIZE_CRITERION = '50, 3' # size of the question box if it's a criterion
SIZE_ALT_DROPDOWN = '0|1|2|3|4|5' # alternative to size if question is a dropdown
SIZE_TXT_AREA = '60, 5' # size of question box if text area
SIZE_TXT_FIELD = '30' # size of question box if text field

Unsafe Reflection

Problem

Unsafe reflection method const_get called with parameter value

Solution

Remove the unsafe reflection through using a variable that checks for null values. We go from:

question = Object.const_get(params[:question][:type]).create(txt: '', questionnaire_id: questionnaire_id,seq: i, type: params[:question][:type], break_before: true)

To:

question_type = params[:question][:type] unless params[:question][:type].nil?
question = Object.const_get(question_type).create(txt: '', questionnaire_id: questionnaire_id,seq: i, type: question_type, break_before: true)

Removed unnecessary method 'create_questionnaire'

Problem

Method create_questionnaire, had no apparent calls to it. Aside from assigning a creator ID, it is similar in functionality to create, so we assume that the method was at some point created as a duplicate. We remove the method to make the code dry.

Solution

For example, we remove:

  def create_questionnaire
    @questionnaire = Object.const_get(params[:questionnaire][:type]).new(questionnaire_params)
    # Create Quiz content has been moved to Quiz Questionnaire Controller
    if @questionnaire.type != "QuizQuestionnaire" # checking if it is a quiz questionnaire
      @questionnaire.instructor_id = Ta.get_my_instructor(session[:user].id) 
                                       if session[:user].role.name == "Teaching Assistant"
      save

      redirect_to controller: 'tree_display', action: 'list'
    end
  end

Break up create method into new 'create_questionnaire'

Problem

The create method itself was 49 lines long. This is too long to be viewable at a glance. Breaking it up into a second private method 'create_questionnaire' to handle attribute assigning and node creation.

Solution

For example, we go from:

 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

To:

  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
        create_questionnaire questionnaire_private, display_type
        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

  def create_questions questionnaire_private, display_type
    @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
    tree_folder = TreeFolder.where(['name like ?', @questionnaire.display_type]).first
    parent = FolderNode.find_by(node_object_id: tree_folder.id)
    # Create node
    QuestionnaireNode.create(parent_id: parent.id, node_object_id: @questionnaire.id, type: 'QuestionnaireNode')
  end

Use each_key instead of keys.each

Problem

The existing code uses keys.each to iterate through the hash. Keys.each is useful for modifying a hash, but in this implementation this is not necessary. So, each_key is used to improve performance timing.

Solution

For example, we go from:

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

To:

  params[:new_question].each_key do |question_key|

Split lines of code to fit within recommended 160 character length

Problem

Some of the lines of code exceed the recommended 160 characters per line. To remediate, we simply split code across multiple lines and indent accordingly to maintain readability.

Solution

For example, we go from:

  question = Object.const_get(params[:question][:type]).create(txt: '', questionnaire_id: questionnaire_id, seq: i, type: params[:question][:type], break_before: true)

To:

 question = Object.const_get(params[:question][:type]).create(txt: '', questionnaire_id: questionnaire_id,
                                                                   seq: i, type: params[:question][:type], break_before: true)

Removed useless assignment of variable in save method

Problem

The existing code included assignments of variables that were not used throughout the file. We assume that these variables were created to implement functionality that has since been removed through previous refactor attempts. In any case, we remove the variable keep the code dry.

Solution

For example, in the following method:

  def save
    @questionnaire.save!

    save_questions @questionnaire.id if !@questionnaire.id.nil? and @questionnaire.id > 0
    # We do not create node for quiz questionnaires
    if @questionnaire.type != "QuizQuestionnaire"
      p_folder = TreeFolder.find_by(name: @questionnaire.display_type)
      parent = FolderNode.find_by(node_object_id: p_folder.id)
      # create_new_node_if_necessary(parent)
    end
    undo_link("Questionnaire \"#{@questionnaire.name}\" has been updated successfully. ")
  end

We remove the unnecessary variables and comments, resulting in the following method:

   def save
    @questionnaire.save!
    save_questions @questionnaire.id if !@questionnaire.id.nil? and @questionnaire.id > 0
    undo_link("Questionnaire \"#{@questionnaire.name}\" has been updated successfully. ")
  end

Created new method 'save_question_hash'

Problem

The same do loop for hash iteration to update questions for a questionnaire existed in both the public methods 'update' and 'save_all_questions' methods. In order to remove repetition and make the code dry, this do loop was refactored into its own method 'save_question_hash'.

Solution

For example, in the following method:

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

We refactor the do loop into its own method:

  def save_question_hash(params)
    params[:question].each_pair do |k, v|
      @question = Question.find(k)
      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

Testing Plan

In order to evaluate the changes to Expertiza throughout the OSS project two different methods were used. The first being automatic testing using RSpec and the second being manual testing through accessing the Expertiza project on one's local machine. More in depth discussion of these tests can be found below.

RSpec Testing

In many cases the issues were resolved by editing a few lines of code within various methods without the need for additional methods. Thus, adding more test was not needed in these cases. However, in order to ensure the code edits didn't cause any previously crafted test to fail an RSpec test was ran before each commit. If and only if all test passed could the commit be pushed.

The command utilized to test the questionnaires controller is as follows:

rspec spec/controllers/questionnaires_controller_spec.rb

However, if a VCL was utilized for development this following command was used instead.

rspec /home/[UNITYID]/expertiza/spec/controllers/questionnaires_controller_spec.rb

In one case the create_questionnaire method was removed, thus in order to keep the set of tests clean the following test block was removed from the testing file:

describe '#create_questionnaire and #save' do
    context 'when quiz is valid' do
      before(:each) do
        # create_quiz_questionnaire
        allow_any_instance_of(QuestionnairesController).to receive(:valid_quiz).and_return('valid')
      end

      context 'when questionnaire type is not QuizQuestionnaire' do
        it 'redirects to submitted_content#edit page' do
          params = {aid: 1,
                    pid: 1,
                    questionnaire: {name: 'Test questionnaire',
                                    type: 'ReviewQuestionnaire'}}
          # create_questionnaire
          allow(ReviewQuestionnaire).to receive(:new).with(any_args).and_return(review_questionnaire)
          session = {user: build(:teaching_assistant, id: 1)}
          allow(Ta).to receive(:get_my_instructor).with(1).and_return(6)
          # save
          allow(TreeFolder).to receive(:find_by).with(name: 'Review').and_return(double('TreeFolder', id: 1))
          allow(FolderNode).to receive(:find_by).with(node_object_id: 1).and_return(double('FolderNode'))
          allow_any_instance_of(QuestionnairesController).to receive(:undo_link).with(any_args).and_return('')
          post :create_questionnaire, params, session
          expect(flash[:note]).to be nil
          expect(response).to redirect_to('/tree_display/list')
          expect(controller.instance_variable_get(:@questionnaire).private).to eq false
          expect(controller.instance_variable_get(:@questionnaire).name).to eq 'Test questionnaire'
          expect(controller.instance_variable_get(:@questionnaire).min_question_score).to eq 0
          expect(controller.instance_variable_get(:@questionnaire).max_question_score).to eq 5
          expect(controller.instance_variable_get(:@questionnaire).type).to eq 'ReviewQuestionnaire'
          expect(controller.instance_variable_get(:@questionnaire).instructor_id).to eq 6
        end
      end
    end
  end

Manual Testing

Test Log in

Website url: http://152.46.18.99:8080/

Log in: instructor6

Password: password

Test Create questionnaire

1. After logging in click the Questionnaires tab.

2. Select the plus sign for Review

3. Enter "test1" for name (or something similiar), 0 for min, 10 for max, and no for review private.

4. Select Create

Test edit/add questions:

5. Change dropbox of question type to TextArea, select Add "1" more. Press the add button.

6. Change dropbox of question type to Criterion, select Add "2" more. Press the add button.

7. Edit question content: "Test question Textarea", "Test question Criterion1", and "Test question Criterion2" in that order.

8. Press Save review questionnaire.

9. Refresh page. All entries should still be available.

10. Select edit or view advice.

11. From descending order fill the boxes with the associated number. i.e. the first box has a 5 above it. Insert 5 in the box. Do this for both criterion.

12. Select save and redisplay advice, then press Back to Questionnaire

13. Going back to Edit/view advice shows the same numbers inputted earlier.

Test View Questionnaire

14. Select back

15. Click on the name Review: This should show a drop down showing different reviews made.

16. Find the review that was created.

17. Select the view icon to the right. The information should match what was created earlier.

Test Delete Questionnaire

18. Select Back.

19. Repeat steps 15-16

20. Select the big red X for delete. When prompted, select YES

21. Click on the name Review. The Questionnaire created previously will not be present anymore

These manual tests demonstrate some of the functionality of the questionnaire controller. Feel free to try your own test cases.

References

• Code Refactoring Best Practices[1]
• Expertiza on Github[2]
• Expertiza Project Fork[3]
• Expertiza Website[4]
• RSpec Documentation[5]