Background

In Expertiza, Questionnaires are used primarily by instructors to create question forms/ reviews, etc. As a student, you might have seen them while performing reviews for peers. The 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 the 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.
A Questionnaire can have multiple Questions as a part of it. These questionnaires can be used as a part of an assignment. Also, we notice that the Questionnaire controller has a parent-child relationship with the QuizQuestionnaire controller. There are multiple types of questions that can be added to a questionnaire like Criterion, Dropdown, Checkbox, and TextArea to name a few. There are multiple type of Questionnaires like Review, MetaReview, Assignment etc.
To sum it off, we can have many types of Questionnaires and each would have possibly multiple types of Questions. There can be numerous questionnaires of the same kind for a given Questionnaire type.

Project Purpose

In recent years, questionnaires_controller.rb has been refactored repeatedly. It is a lot clearer than it used to be. But rough edges still remain. question.rb contains a large number of constants. It is not clear what they are used for. There is a questionnaires_controller, but no questions_controller which is fully developed. The questionnaires_controller creates Question objects.
This is not elegant, because a controller should only create objects of one type, the type of objects it controls. In the present code we see that the questionnaire_controller.rb has multiple non CRUD methods that should have been in the questions_controller.rb. Our priority is to refactor these functions to the questions_controller.rb so that we maintain the Single responsibility in the SOLID principles. We also notice that there are 2 places where there is code that is repeating itself so we extract the common code into a helper function to make the code more DRY.
The following functions that need refactoring-

def delete_questions(questionnaire_id) Move to questions_controller
def question_params Move to questions_controller
def save_new_questions(questionnaire_id) Move to questions_controller
def save_questions(questionnaire_id) Move to questions_controller

We also noticed that there were many places that used constants like the text file are size or the default min and max scores being hard coded. This makes the code not suitable for extension as any change would need to be refactored in multiple places and break code easily.
Thus we plan to modify the models/questions.rb and add the constants there so that all files related to questions can have access to these constants. This would make the refactoring process very easy.

Design Pattern Used

There are multiple types of questions created based on the parameters passed by the user to the controller. This led to two things, Objects being created dynamically which might not be of the acceptable kind, and code injection, as we are using unsanitized user input.

We have decided to use the Factory design pattern instead where we create a type of Questionnaire based on the type of parameters passed. Hence instead of multiple if/else code blocks to check the kind of input passed, we use a factory method to create and return the Questionnaire/Question object. Additionally, Instead of using if/else checks in the factory method, we opt for using a Map to match the input param to an Object type which further cleaned our code.

We employed SOLID principles, and shifted code that pertained to Questions from Questionnaire controller to Questions controller. This helps us better adhere to the Single Responsibility principle. We also shifted the common code out of the Questionnaire controller to helper classes to make the code more DRY.

All Changes Implemented

#	File/Function	Change Proposed	Rationale	Commit Link
1	method `add_new_questions()`	Store the used default value to some kind of constant in `questions.rb`, so that they can be changed without refactoring code	There are many literals used in the code and can change in future, so it is better to separate them	Commit
2	files`question_helper.rb` and `questionnaire_helper.rb`	Add a factory method that creates respective object based on the parameter that is being passed	We use factory method in a helper file as that removes the code for creating an object from inside a function that just uses those objects	Commit
3	File `questionnaires_controller.rb`	Move `save_new_questions()`	To be consistent with SOLID principle, these methods belong in questions_controller.rb	Commit
4	File `questionnaires_controller.rb`	Move `delete_questions()`	To be consistent with SOLID principle, these methods belong in questions_controller.rb	Commit
5	File `questionnaires_controller.rb`	Move `save_questions()`	To be consistent with SOLID principle, these methods belong in questions_controller.rb	Commit
6	file `questionnaires_controller.rb` and `questions_controller.rb`	Refactor or add more comments to the following vague method names	There are confusing function names like `save_all_questions()` , `save_new_questions()` , `save_questions()` and `save()` and the use of each cannot be easily understood	Commit
7	method `save_questions()`	Change `params[:question].keys.each` to `params[:question].each_key`	This will improve the performance and uses a single enumerator	Commit
8	method `create()`	Remove parenthesis around function call on line 72	Unnecessary parenthesis	Commit
9	method `create()` in `questionnaire_controller.rb`	Move code related to creation of tree node to different function name `create_tree_node()`	The create method is very big and this would help in readability and code re-usability	Commit
10	method `questionnaire_helper()`	Remove the usage of if/else or switch case and use a Hash Map for questionnaire factory object creation	We want to make our code more modular and extendable. Any change can be easily added to Hash Map rather than adding if else statements.	Commit
11	method `question_helper()`	Remove the usage of if/else or switch case and use a Hash Map for questions factory object creation	We want to make our code more modular and extendable. Any change can be easily added to Hash Map rather than adding if else statements.	Commit
12	controller `advice_controller.rb`	There needs to be a `.each` after they keys are enumerated	The Edit/View advice button was not working on the view where we create a questionnaire and new questions	Commit
13	file `routes.rb`	Add some routes so that all the new methods in `questions_controller.rb` can work	Newly added functions don't have a predefined route and it must be specified	Commit
14	file `routes.rb`	Add some routes so that the delete functionality works for questionnaires	The existing code had the functionality implemented but the route was not configured rendered an error	Commit
15	controller `quiz_questionnaire_controller.rb()`	There are multiple redirects to functions in the `questionnaire_controller.rb`	As some functions have been moved to the `questions_controller.rb` they need to be refactored.	Commit
16	method `update_questionnaire_questions`	As per DRY, we removed the code to update the questionnaire's questions from `questionnaire_controller.rb` to the helper class.	This will make the code more DRY and maintainable	Commit

Sample of Code Changes

Questionnaire Factory where the object is created with switch case using a HashMap in questionnaire_helper.rb

 #Map type to questionnaire
 QUESTIONNAIRE_MAP = {
   'ReviewQuestionnaire' => ReviewQuestionnaire,
   'MetareviewQuestionnaire' => MetareviewQuestionnaire,
   'AuthorFeedbackQuestionnaire' => AuthorFeedbackQuestionnaire,
   'TeammateReviewQuestionnaire' => TeammateReviewQuestionnaire,
   'AssignmentSurveyQuestionnaire' => AssignmentSurveyQuestionnaire,
   'SurveyQuestionnaire' => SurveyQuestionnaire,
   'GlobalSurveyQuestionnaire' => GlobalSurveyQuestionnaire,
   'CourseSurveyQuestionnaire' => CourseSurveyQuestionnaire,
   'BookmarkRatingQuestionnaire' => BookmarkRatingQuestionnaire,
   'QuizQuestionnaire' => QuizQuestionnaire
 }.freeze
 
 # factory method to create the appropriate questionnaire based on the type 
 def questionnaire_factory(type)
   questionnaire = QUESTIONNAIRE_MAP[type]
   if questionnaire.nil?
     flash[:error] = 'Error: Undefined Questionnaire'
   else
     questionnaire.new
   end
 end

Question Factory where the questions object is created using a HashMap in question_helper.rb

module QuestionHelper
  # Maps type to question
  QUESTION_MAP = {
    'Criterion' => Criterion,
    'Scale' => Scale,
    'Cake' => Cake,
    'Dropdown' => Dropdown,
    'Checkbox' => Checkbox,
    'TextArea' => TextArea,
    'TextField' => TextField,
    'UploadFile' => UploadFile,
    'SectionHeader' => SectionHeader,
    'TableHeader' => TableHeader,
    'ColumnHeader' => ColumnHeader
  }.freeze
  
  # factory method to create the appropriate question based on the type
  def question_factory(type, questionnaire_id, seq)
    question_class = QUESTION_MAP[type]
    if question_class.nil?
      flash[:error] = 'Error: Undefined Question'
    else
      question_class.create(txt: , questionnaire_id: questionnaire_id, seq: seq, type: type, break_before: true)
    end
  end
end

models/question.rb that shows how the constants are used to store the parameters that would be used for object creation

 # Class variables - used questionnaires_controller.rb to set the parameters for a question.
 MAX_LABEL = 'Strongly agree'.freeze
 MIN_LABEL = 'Strongly disagree'.freeze
 SIZES = { 'Criterion' => '50, 3', 'Cake' => '50, 3', 'TextArea' => '60, 5', 'TextField' => '30' }.freeze
 ALTERNATIVES = { 'Dropdown' => '0|1|2|3|4|5' }.freeze

def set_questionnaire_parameters in questionnaires_controller.rb

 # Assigns corrresponding variables to questionnaire object.
 def set_questionnaire_parameters(private_flag, display)
   @questionnaire.private = private_flag
   @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

advice_controller.rb lines 57-64: keys && each_key methods return incorrectly, keys.each is proper syntax for this functionality

before:

 unless params[:advice].nil?
   	 params[:advice].keys do |advice_key|
     	 # Updates the advice corresponding to the key
     	 QuestionAdvice.update(advice_key, advice: params[:advice] [advice_key.to_sym] [:advice])
   end

after:

 unless params[:advice].nil?
   	 params[:advice].keys.each do |advice_key|
     	 # Updates the advice corresponding to the key
        QuestionAdvice.update(advice_key, advice: params[:advice] [advice_key.to_sym] [:advice])
   end

Questionnaires_controller.rb lines 89-99: removal of create_questionnaires method due to it having identical functionality to create

The following method was deleted

 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

Important links

Github: https://github.com/Soultrekker21/expertiza
Pull Request: https://github.com/expertiza/expertiza/pull/2576
Current VCL: http://152.7.178.100:3000/

Test Plan

Make sure that current proposed changes don't break existing test cases.
Add unit test for newly proposed methods.
Shift existing test cases for questionnaire_controller_spec.rb to question_controller.rb corresponding to methods that are being shifted

Testing Implementation

Removal of create_questionnaire method test: Testing a non-existent method no longer necessary
Aside from this we found that the questionnaire controller is already well tested
Below is questionnaires_controller_spec.rb tests run with bundle exec rspec

Below is questions_controller_spec.rb tests run with bundle exec rspec

Below is quiz_questionnaires_controller_spec.rb tests run with bundle exec rspec

CSC/ECE 517 Spring 2023 -E2326 Refactor questionnaires controller.rb

Contents

Background

Project Purpose

Design Pattern Used

All Changes Implemented

Sample of Code Changes

Important links

Test Plan

Testing Implementation

Navigation menu

CSC/ECE 517 Spring 2023 -E2326 Refactor questionnaires controller.rb

Background

Project Purpose

Design Pattern Used

All Changes Implemented

Sample of Code Changes

Important links

Test Plan

Testing Implementation

Navigation menu

Search