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:
- Assess if the method called create_questionnaire is used and remove it if not
- The create method is 49 lines long and needs to be broken up into smaller self-documenting methods
- There are three checks for whether a questionnaire is a QuizQuestionnaire, evaluate the necessity of these and remove as needed.
- Several methods have a questionnaire_id as a parameter, however This may not be necessary as params[:id] should be enough. Evaluate options and implement a solution.
- 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.
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
Won't Fix Issues
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