|
|
| Line 1: |
Line 1: |
| ==E1947. Refactoring quiz_questionnaire_controller.rb==
| |
|
| |
|
| This page provides a description of the Expertiza based OSS project.
| |
|
| |
|
| |
| __TOC__
| |
|
| |
|
| |
| ===About Expertiza===
| |
|
| |
| [http://expertiza.ncsu.edu/ Expertiza] is an open source project based on [http://rubyonrails.org/ Ruby on Rails] framework. Expertiza allows the instructor to create new assignments and customize new or existing assignments. It also allows the instructor to create a list of topics the students can sign up for. Students can form teams in Expertiza to work on various projects and assignments. Students can also peer review other students' submissions. Expertiza supports submission across various document types, including the URLs and wiki pages.
| |
|
| |
| <br>
| |
|
| |
| ===Problem Statement===
| |
| The following tasks were accomplished in this project:
| |
|
| |
| * Rename filename such that it follows the coding standards
| |
| * Remove redundant information from method names
| |
| * Add comments to methods and complex lines of code
| |
| * Remove unsafe reflection method
| |
| * Simplify RSpec
| |
| * Try to fix issues from Code Climate
| |
| <br>
| |
|
| |
| ===About Quiz Questionnaire Controller===
| |
| This is a new controller, having recently been separated from ''questionnaires_controller.rb''. It is used to allow students to take quizzes. The idea is that the author(s) of submitted work can write a quiz that is given to each reviewer before the reviewer is allowed to review the work. If the reviewer does badly on the quiz, then we know not to trust the review. It is also possible to set up an Expertiza assignment so that some participants just take the quiz and don’t review the work.
| |
|
| |
| <br>
| |
|
| |
| ===Current Implementation===
| |
|
| |
|
| |
| =====Functionality=====
| |
| * A student can create a quiz questionnaire for the assignments that allow a quiz to be generated. This quiz will be taken by the reviewers, once they are done reviewing the development/project. Based on their answers the TA's can know whether to consider the reviewer's review or not based , because if the reviewer didn't perform well on the quiz, then it is likely that the review is not done properly.
| |
|
| |
| * The maximum number of questions is decided by the instructor when creating the assignment.
| |
|
| |
| * The student has option to either create True/False, Multiple-Choice Checkbox or Multiple-Choice Radio question.
| |
|
| |
| * The question cannot be blank and correct answer must be selected to create a quiz.
| |
|
| |
| =====Drawbacks and Solutions=====
| |
| * '''Problem 1''': Rename filename such that it follows the coding standards
| |
| ** '''Solution''': Currently the file was named as ''quiz_questionnaire_controller.rb''. This doesn't follow the Ruby naming standards. Hence it was changed to ''quiz_questionnaires_controller.rb''
| |
| * '''Problem 2''': Remove redundant information from method names
| |
| **'''Solution''': Since this controller was separated from the ''questionnaires_controller.rb'', the method names were kept the same from the previous controller which had redundant information. Method names were renamed by removing the succeeding '_quiz'. Also ''valid_quiz'' was renamed to ''validate_quiz'' to make more sense. Also the ''routes.rb'' was changed to accept these changes.
| |
| * '''Problem 3''': Add comments to methods and complex lines of code
| |
| **'''Solution''': Many methods had more than 325 lines of code and performed multiple functionalities. These methods were changed by creating private methods that performed individual tasks, thus reducing the lines of code and the complexity.
| |
| * '''Problem 4''': Remove unsafe reflection method
| |
| **'''Solution''': This part was already implemented, hence no changes were made. Method ''questionnaire_params'' was already defined and takes care of this issue.
| |
| * '''Problem 5''': Fix issues from Code Climate
| |
| **'''Solution''': Refer to Solution for Problem 3
| |
|
| |
| <br>
| |
|
| |
| ===Code improvements===
| |
| * The method '''save_choices''' had a congnitive complexity of 23 and had 37 lines of code. By refactoring, the number of lines was reduced to 20 and private methods '''create_checkbox''', '''create_truefalse''' and '''create_radio''' were implemented to break up functionality and to reduce cognitive complexity.
| |
| '''Before:'''
| |
| <pre>
| |
| def save_choices(questionnaire_id)
| |
| return unless params[:new_question] or params[:new_choices]
| |
| questions = Question.where(questionnaire_id: questionnaire_id)
| |
| question_num = 1
| |
|
| |
| questions.each do |question|
| |
| q_type = params[:question_type][question_num.to_s][:type]
| |
| params[:new_choices][question_num.to_s][q_type].each_key do |choice_key|
| |
| if q_type == "MultipleChoiceCheckbox"
| |
| q = if params[:new_choices][question_num.to_s][q_type][choice_key][:iscorrect] == 1.to_s
| |
| QuizQuestionChoice.new(txt: params[:new_choices][question_num.to_s][q_type][choice_key][:txt], iscorrect: "true", question_id: question.id)
| |
| else
| |
| QuizQuestionChoice.new(txt: params[:new_choices][question_num.to_s][q_type][choice_key][:txt], iscorrect: "false", question_id: question.id)
| |
| end
| |
| q.save
| |
| elsif q_type == "TrueFalse"
| |
| if params[:new_choices][question_num.to_s][q_type][1.to_s][:iscorrect] == choice_key
| |
| q = QuizQuestionChoice.new(txt: "True", iscorrect: "true", question_id: question.id)
| |
| q.save
| |
| q = QuizQuestionChoice.new(txt: "False", iscorrect: "false", question_id: question.id)
| |
| q.save
| |
| else
| |
| q = QuizQuestionChoice.new(txt: "True", iscorrect: "false", question_id: question.id)
| |
| q.save
| |
| q = QuizQuestionChoice.new(txt: "False", iscorrect: "true", question_id: question.id)
| |
| q.save
| |
| end
| |
| else
| |
| q = if params[:new_choices][question_num.to_s][q_type][1.to_s][:iscorrect] == choice_key
| |
| QuizQuestionChoice.new(txt: params[:new_choices][question_num.to_s][q_type][choice_key][:txt], iscorrect: "true", question_id: question.id)
| |
| else
| |
| QuizQuestionChoice.new(txt: params[:new_choices][question_num.to_s][q_type][choice_key][:txt], iscorrect: "false", question_id: question.id)
| |
| end
| |
| q.save
| |
| end
| |
| end
| |
| question_num += 1
| |
| question.weight = 1
| |
| end
| |
| end
| |
| </pre>
| |
| '''After:'''
| |
| <pre>
| |
| def save_choices(questionnaire_id)
| |
| return unless params[:new_question] or params[:new_choices]
| |
| questions = Question.where(questionnaire_id: questionnaire_id)
| |
| question_num = 1
| |
|
| |
| questions.each do |question|
| |
| q_type = params[:question_type][question_num.to_s][:type]
| |
| q_choices = params[:new_choices][question_num.to_s][q_type]
| |
| q_choices.each_key do |choice_key|
| |
| if q_type == "MultipleChoiceCheckbox"
| |
| create_checkbox(question, choice_key, q_choices)
| |
| elsif q_type == "TrueFalse"
| |
| create_truefalse(question, choice_key, q_choices)
| |
| else # MultipleChoiceRadio
| |
| create_radio(question, choice_key, q_choices)
| |
| end
| |
| end
| |
| question_num += 1
| |
| question.weight = 1
| |
| end
| |
| end
| |
| </pre>
| |
|
| |
| <br>
| |
| * The method '''action_allowed?''' has been implemented in quiz_questionnaire_controller.rb to specifically allow Student to edit an existing quiz.
| |
| <br>
| |
| * The quiz_questionnaire_controller_spec.rb has been updated to allow edit tests to work for Student role.
| |
| <pre>
| |
| let(:student) { build(:student, id: 8609) }
| |
| </pre>
| |
| '''Before:'''
| |
| <pre>
| |
| context 'when current questionnaire is not taken by anyone' do
| |
| it 'renders questionnaires#edit page' do
| |
| allow(@questionnaire).to receive(:taken_by_anyone?).and_return(false)
| |
| params = {id: 1}
| |
| get :edit, params
| |
| expect(response).to render_template(:edit)
| |
| end
| |
| end
| |
| </pre>
| |
| '''After:'''
| |
| <pre>
| |
| context 'when current questionnaire is not taken by anyone' do
| |
| it 'renders questionnaires#edit page' do
| |
| stub_current_user(student, student.role.name, student.role) #action only permitted for Student role
| |
| allow(@questionnaire).to receive(:taken_by_anyone?).and_return(false)
| |
| params = {id: 1}
| |
| get :edit, params
| |
| expect(response).to render_template(:edit)
| |
| end
| |
| end
| |
| </pre>
| |
| '''Before:'''
| |
| <pre>
| |
| context 'when current questionnaire has been taken by someone' do
| |
| it 'shows flash[:error] message and redirects to submitted_content#view page' do
| |
| allow(@questionnaire).to receive(:taken_by_anyone?).and_return(true)
| |
| params = {id: 1, pid: 1}
| |
| get :edit, params
| |
| expect(flash[:error]).to eq('Your quiz has been taken by some other students, you cannot edit it anymore.')
| |
| expect(response).to redirect_to('/submitted_content/view?id=1')
| |
| end
| |
| end
| |
| end
| |
| </pre>
| |
| '''After:'''
| |
| <pre>
| |
| context 'when current questionnaire has been taken by someone' do
| |
| it 'shows flash[:error] message and redirects to submitted_content#view page' do
| |
| stub_current_user(student, student.role.name, student.role) #action only permitted for Student role
| |
| allow(@questionnaire).to receive(:taken_by_anyone?).and_return(true)
| |
| params = {id: 1, pid: 1}
| |
| get :edit, params
| |
| expect(flash[:error]).to eq('Your quiz has been taken by some other students, you cannot edit it anymore.')
| |
| expect(response).to redirect_to('/submitted_content/view?id=1')
| |
| end
| |
| end
| |
| end
| |
| </pre>
| |
| <br>
| |
| * The method '''update_quiz''' has been renamed to '''update''' and has been refactored to reduce the line count by breaking functionalities. (The function '''multiple_choice_checkbox''' was renamed to '''update_checkbox''' and '''multiple_choice_radio''' was renamed to '''update_radio''' so as to reduce function name length and bring consistency with methods performing create operations: '''create_radio''', '''create_checkbox''', '''create_truefalse''')
| |
| The Cognitive Complexity of update method has now been reduced to 15 (from 29).<br>
| |
| '''Before:'''
| |
| <pre>
| |
| def update
| |
| @questionnaire = Questionnaire.find(params[:id])
| |
| if @questionnaire.nil?
| |
| redirect_to controller: 'submitted_content', action: 'view', id: params[:pid]
| |
| return
| |
| end
| |
| if params['save'] && params[:question].try(:keys)
| |
| @questionnaire.update_attributes(questionnaire_params)
| |
|
| |
| params[:question].each_key do |qid|
| |
| @question = Question.find(qid)
| |
| @question.txt = params[:question][qid.to_sym][:txt]
| |
| @question.save
| |
|
| |
| @quiz_question_choices = QuizQuestionChoice.where(question_id: qid)
| |
| question_index = 1
| |
| @quiz_question_choices.each do |question_choice|
| |
| # Call to private method to handle Multile Choice Questions
| |
| multiple_choice_checkbox(question_choice, question_index) if @question.type == "MultipleChoiceCheckbox"
| |
| multiple_choice_radio(question_choice, question_index) if @question.type == "MultipleChoiceRadio"
| |
| if @question.type == "TrueFalse"
| |
| if params[:quiz_question_choices][@question.id.to_s][@question.type][1.to_s][:iscorrect] == "True" # the statement is correct
| |
| question_choice.txt == "True" ? question_choice.update_attributes(iscorrect: '1') : question_choice.update_attributes(iscorrect: '0')
| |
| # the statement is correct so "True" is the right answer
| |
| else # the statement is not correct
| |
| question_choice.txt == "True" ? question_choice.update_attributes(iscorrect: '0') : question_choice.update_attributes(iscorrect: '1')
| |
| # the statement is not correct so "False" is the right answer
| |
| end
| |
| end
| |
| question_index += 1
| |
| end
| |
| end
| |
| end
| |
| redirect_to controller: 'submitted_content', action: 'view', id: params[:pid]
| |
| end
| |
| </pre>
| |
|
| |
| '''After:'''
| |
| <pre>
| |
| def update
| |
| @questionnaire = Questionnaire.find(params[:id])
| |
| if @questionnaire.nil?
| |
| redirect_to controller: 'submitted_content', action: 'view', id: params[:pid]
| |
| return
| |
| end
| |
| if params['save'] && params[:question].try(:keys)
| |
| @questionnaire.update_attributes(questionnaire_params)
| |
| params[:question].each_key do |qid|
| |
| @question = Question.find(qid)
| |
| @question.txt = params[:question][qid.to_sym][:txt]
| |
| @question.save
| |
| @quiz_question_choices = QuizQuestionChoice.where(question_id: qid)
| |
| question_index = 1
| |
| @quiz_question_choices.each do |question_choice|
| |
| # Call to private method to handle Multile Choice Questions
| |
| update_checkbox(question_choice, question_index) if @question.type == "MultipleChoiceCheckbox"
| |
| update_radio(question_choice, question_index) if @question.type == "MultipleChoiceRadio"
| |
| update_truefalse(question_choice) if @question.type == "TrueFalse"
| |
| question_index += 1
| |
| end
| |
| end
| |
| end
| |
| redirect_to controller: 'submitted_content', action: 'view', id: params[:pid]
| |
| end
| |
| </pre>
| |
| <pre>
| |
| def update_truefalse(question_choice)
| |
| if params[:quiz_question_choices][@question.id.to_s][@question.type][1.to_s][:iscorrect] == "True" # the statement is correct
| |
| question_choice.txt == "True" ? question_choice.update_attributes(iscorrect: '1') : question_choice.update_attributes(iscorrect: '0')
| |
| # the statement is correct so "True" is the right answer
| |
| else # the statement is not correct
| |
| question_choice.txt == "True" ? question_choice.update_attributes(iscorrect: '0') : question_choice.update_attributes(iscorrect: '1')
| |
| # the statement is not correct so "False" is the right answer
| |
| end
| |
| end
| |
| </pre>
| |
|
| |
|
| |
| <br>
| |
| * The method '''valid_quiz''' has been renamed to '''validate_quiz'''. The function '''validate_question''' was extracted out to only validate the particular question. The part where quiz name was being verified was also taken out of the loop to only verify this once as opposed to verifying in each iteration, which was unnecessary. The Cognitive Complexity of 12 has now been reduced to less than 5.
| |
| '''Before:'''
| |
| <pre>
| |
| def valid_quiz
| |
| num_questions = Assignment.find(params[:aid]).num_quiz_questions
| |
| valid = "valid"
| |
|
| |
| (1..num_questions).each do |i|
| |
| if params[:questionnaire][:name] == "" # questionnaire name is not specified
| |
| valid = "Please specify quiz name (please do not use your name or id)."
| |
| elsif !params.key?(:question_type) || !params[:question_type].key?(i.to_s) || params[:question_type][i.to_s][:type].nil?
| |
| # A type isnt selected for a question
| |
| valid = "Please select a type for each question"
| |
| else
| |
| @new_question = Object.const_get(params[:question_type][i.to_s][:type]).create(txt: '', type: params[:question_type][i.to_s][:type], break_before: true)
| |
| @new_question.update_attributes(txt: params[:new_question][i.to_s])
| |
| type = params[:question_type][i.to_s][:type]
| |
| choice_info = params[:new_choices][i.to_s][type] # choice info for one question of its type
| |
| valid = if choice_info.nil?
| |
| "Please select a correct answer for all questions"
| |
| else
| |
| @new_question.isvalid(choice_info)
| |
| end
| |
| end
| |
| break if valid != "valid"
| |
| end
| |
| valid
| |
| end
| |
| </pre>
| |
| '''After:'''
| |
| <pre>
| |
| def validate_quiz
| |
| num_questions = Assignment.find(params[:aid]).num_quiz_questions
| |
| valid = "valid"
| |
| if params[:questionnaire][:name] == "" # questionnaire name is not specified
| |
| valid = "Please specify quiz name (please do not use your name or id)."
| |
| end
| |
| (1..num_questions).each do |i|
| |
| break if valid != "valid"
| |
| valid = validate_question(i)
| |
| end
| |
| valid
| |
| end
| |
| </pre>
| |
| <pre>
| |
| def validate_question(i)
| |
| if !params.key?(:question_type) || !params[:question_type].key?(i.to_s) || params[:question_type][i.to_s][:type].nil?
| |
| # A type isn't selected for a question
| |
| valid = "Please select a type for each question"
| |
| else
| |
| # The question type is dynamic, so const_get is necessary
| |
| type = params[:question_type][i.to_s][:type]
| |
| @new_question = Object.const_get(type).create(txt: '', type: type, break_before: true)
| |
| @new_question.update_attributes(txt: params[:new_question][i.to_s])
| |
| choice_info = params[:new_choices][i.to_s][type] # choice info for one question of its type
| |
| valid = if choice_info.nil?
| |
| "Please select a correct answer for all questions"
| |
| else
| |
| @new_question.isvalid(choice_info)
| |
| end
| |
| end
| |
| valid
| |
| end
| |
| </pre>
| |
|
| |
| ===Testing===
| |
| Link to [https://youtu.be/dVu0-LZcW1w video] showing the RSpec testing.
| |
| ====Automated Testing using RSPEC====
| |
| [[File:RspecResults.png]]
| |
|
| |
| ====Testing from Travis CI====
| |
| [[File:TravisCIBuild.png]]
| |
| [[File:GitChecksPass.png]]
| |