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

Background

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. We can see that a Questionnaire can have multiple Questions. These questionnaires can be used as a part of an assignment. Also we notice that Questionnaire controler 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.

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 functions that need refactoring-

• def delete_questions(questionnaire_id) Move to questions_controller
• def question_params Move to questions_controller?
• def save_all_questions 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.

Proposed Design Changes

#	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	Will be added later
2	File questionnaires_controller.rb	Move save_all_questions(), delete_questions(), save_new_questions(), save_questions()	To be consistent with SOLID principle, these methods belong in questions_controller.rb	Will be added later
3	file questionnaires_controller.rb	Refactor following vague method names	There are confusing function names like save_all_questions() , save_new_questions() , save_questions() and save()	Will be added later
4	method save_questions()	Change params[:question].keys.each to params[:question].each_key	This will improve the performance and uses a single enumerator	Will be added late
5	method create()	Remove parenthesis around function call on line 72	Unnecessary parenthesis	Will be added later
5	File questionnaires_controller.rb	Refactor large methods	Create, update, and delete are noticeably larger than most methods	Will be added later
6	method save_all_questions()	Find a way to refactor the code by using more variables or Procs that can be reused in questions_controller.rb	The process of saving all questions with 2 nested do blocks is difficult to understand	Will be added later
6	method create	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 reusability	Will be added later
7	method questionnaire_helper()	Change if/else to switch case and add default error checking to helper	Code will be cleaner and there will needs to be catch all because there are no default else conditions/ error checks in factories as of now.	Will be added later
8	method question_helper()	Change if/else to switch case and add default error checking to helper	Code will be cleaner and there will needs to be catch all because there are no default else conditions/ error checks in factories as of now.	Will be added later
9	method question_helper()	Change if/else to switch case and add default error checking to helper	Code will be cleaner and there will needs to be catch all because there are no default else conditions/ error checks in factories as of now.	Will be added later
10	method question_helper()	Change if/else to switch case and add default error checking to helper	Code will be cleaner and there will needs to be catch all because there are no default else conditions/ error checks in factories as of now.	Will be added later
11	method question_helper()	Change if/else to switch case and add default error checking to helper	Code will be cleaner and there will needs to be catch all because there are no default else conditions/ error checks in factories as of now.	Will be added later
12	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 link https://github.com/expertiza/expertiza/commit/7b875fbcbc0bc23db2d14280945f09c4cc968223 ]

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

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