CSC/ECE 517 Spring 2020 / E2001 Refactor Questionnaires Controller
E2000 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.
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:
- Unprotected mass assignment in 3 different areas
- Unsafe reflection method const_get called with parameter value in 4 locations
- 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
- app/controllers/questionnaires_controller.rb
- 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
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)
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
Manual testing was performed by running Expertiza on the local machine and creating the various types of questionnaires (i.e. rubrics, quizzes, and surveys) in the various locations they occurred albeit non-exhaustively . If a questionnaire was created and could be updated without throwing an error it was determined that the refactoring on the questionnaires controller had succeeded. This process was performed and confirmed by all team members in order to ensure accuracy.