CSC/ECE 517 Fall 2017/E1749 Test First Development Refactor questionnaire controller.rb
About
Expertiza is a website that is used and developed by NCSU students and faculty. Coded using Ruby on Rails the source code is readily available on Github -> https://github.com/expertiza/expertiza. The website is used by students for organizing teams, signing up for topics, reviewing other teams and another bunch of tasks. The faculty i.e. Instructors and TAs use this website to set new tasks, add new questions and a few other tasks.
Team Members
- Shreyas Zagade
- Mohit Satarkar
- Darshan Bhandari
Setting Up The Working Environment
One of several ways to set up the environment and the one we adopted is:-
Ubuntu-Expertiza image (.OVA) [Recommended]
- This is the link for the image. (https://drive.google.com/a/ncsu.edu/file/d/0B2vDvVjH76uEUmNKVncxRUhUVVE/view?usp=sharing)
- And you can install VirtualBox (free) and import this image into VirtualBox.
Some machine may require you to enable virtualization and then run the following commands.
cd expertiza
bash ./setup.sh
bundle install
rake db:migrate
- For logging in as an instructor:-
- Username: instructor6
- Password: password
Files involved
- questionnaire_controller.rb
- questionnaire_controller_spec.rb
Issues
- questionnaires_controller.rb is a fairly complex file.
- It contains a lot of methods that are long and hard to understand, these methods need to be broken down into simpler and more specific methods that are easier to read/understand.
- Also, the few instances of code duplication that exist should be removed.
- questionnaires_controller.rb is a fairly complex file.
Tasks assigned
- Complete pending tests in questionnaires_controller_spec.rb, and write integration tests for newly-created methods. Please finish one set of pending tests first before refactoring corresponding methods.
- Refactor create method
- Write failing tests first
- Split into several simpler methods and assign reasonable names
- Extract duplicated code into separate methods
- Refactor update_quiz, save_choices method
- Write failing tests first.
- Use polymorphism to replace the long switch statements
- Move if conditions to corresponding subclasses (eg. MultipleChoiceCheckbox.rb, MultipleChoiceRadio.rb) with same method name
- Replace the conditional with the relevant method calls
- Remove duplicated code
- Use find_by instead of dynamic method
- Write failing tests first
- L68, L385, L386, L559, L560
Tasks Completed
- Testing
- Refactoring
Testing
1) Completed test cases for create
- When test case was pending
describe '#create' do it 'redirects to questionnaires#edit page after create a new questionnaire' end
- After writing test case
describe '#create' do it 'redirects to questionnaires#edit page after create a new questionnaire' do tree_folder1 = double('TreeFolder') allow(tree_folder1).to receive(:id).and_return(1) allow(tree_folder1).to receive(:node_object_id).and_return(1) tree_folder2 = double('TreeFolder') allow(tree_folder2).to receive(:id).and_return(1) allow(tree_folder2).to receive(:node_object_id).and_return(1) allow(TreeFolder).to receive(:where).with(["name like ?", "Review"]).and_return([tree_folder1, tree_folder2]) folder_node2 = double('FolderNode') allow(folder_node2).to receive(:id).and_return(1) allow(FolderNode).to receive(:find_by_node_object_id).and_return(folder_node2) user = double("User") allow(user).to receive(:id).and_return(1) params = { questionnaire: { private: "true", type: "ReviewQuestionnaire", name: "Random Name", min_question_score: "0", max_question_score: "5" } } session = {user: user} get :create, params, session expect(flash[:success]).to eq("You have successfully created a questionnaire!") response.should redirect_to(edit_questionnaire_path(id: 1)) end end
2) Completed test cases for update_quiz
- When test case was pending
describe '#update_quiz' do context 'when @questionnaire is nil' do it 'redirects to submitted_content#view page' end
context 'when @questionnaire is not nil' do it 'updates all quiz questions and redirects to submitted_content#view page' # params = { # id: 1, # pid: 1, # save: true, # questionnaire: { # name: 'test questionnaire', # instructor_id: 6, # private: 0, # min_question_score: 0, # max_question_score: 5, # type: 'ReviewQuestionnaire', # display_type: 'Review', # instructor_loc: # }, # question: { # '1' => {txt: 'Q1'}, # '2' => {txt: 'Q2'}, # '3' => {txt: 'Q3'} # }, # quiz_question_choices: { # '1' => {MultipleChoiceRadio: # {:correctindex => 1, '1' => {txt: 'a11'}, '2' => {txt: 'a12'}, '3' => {txt: 'a13'}, '4' => {txt: 'a14'}}}, # '2' => {TrueFalse: {'1' => {iscorrect: 'True'}}}, # '3' => {MultipleChoiceCheckbox: # {'1' => {iscorrect: '1', txt: 'a31'}, '2' => {iscorrect: '0', txt: 'a32'}, # '3' => {iscorrect: '1', txt: 'a33'}, '4' => {iscorrect: '0', txt: 'a34'}}} # } # } end end
- After writing test case
describe '#update_quiz' do context 'when @questionnaire is nil' do it 'redirects to submitted_content#view page' do params = { id: 1, pid: 1 } get :update_quiz, params response.should redirect_to(view_submitted_content_index_path(id: params[:pid])) end end
context 'when @questionnaire is not nil' do it 'updates all quiz questions and redirects to submitted_content#view page' do params = { id: 3, pid: 1, save: true, questionnaire: { name: 'test questionnaire', instructor_id: 6, private: 0, min_question_score: 0, max_question_score: 5, type: 'ReviewQuestionnaire', display_type: 'Review', instructor_loc: }, question: { '1' => {txt: 'Q1'}, '2' => {txt: 'Q2'}, '3' => {txt: 'Q3'} }, quiz_question_choices: { '1' => {MultipleChoiceRadio: {:correctindex => 1, '1' => {txt: 'a11'}, '2' => {txt: 'a12'}, '3' => {txt: 'a13'}, '4' => {txt: 'a14'}}}, '2' => {TrueFalse: {'1' => {iscorrect: 'True'}}}, '3' => {MultipleChoiceCheckbox: {'1' => {iscorrect: '1', txt: 'a31'}, '2' => {iscorrect: '0', txt: 'a32'}, '3' => {iscorrect: '1', txt: 'a33'}, '4' => {iscorrect: '0', txt: 'a34'}}} } } get :update_quiz, params response.should redirect_to(view_submitted_content_index_path(id: params[:pid])) end end end
Refactoring
- Create method has been split into two methods create and save_questionnaire_create
Before Modification:
def create questionnaire_private = params[:questionnaire][:private] == "true" ? true : false display_type = params[:questionnaire][:type].split('Questionnaire')[0] if Questionnaire::QUESTIONNAIRE_TYPES.include? params[:questionnaire][:type] @questionnaire = Object.const_get(params[:questionnaire][:type]).new 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. case display_type when 'AuthorFeedback' display_type = 'Author%Feedback' when 'CourseSurvey' display_type = 'Course%Survey' when 'TeammateReview' display_type = 'Teammate%Review' when 'GlobalSurvey' display_type = 'Global%Survey' when 'AssignmentSurvey' display_type = 'Assignment%Survey' 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 flash[:error] = $ERROR_INFO end redirect_to controller: 'questionnaires', action: 'edit', id: @questionnaire.id end
After Modification:
def create begin save_questionnaire_create # 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 flash[:error] = $ERROR_INFO end redirect_to controller: 'questionnaires', action: 'edit', id: @questionnaire.id end
def save_questionnaire_create questionnaire_private = params[:questionnaire][:private] == "true" ? true : false display_type = params[:questionnaire][:type].split('Questionnaire')[0] if Questionnaire::QUESTIONNAIRE_TYPES.include? params[:questionnaire][:type] @questionnaire = Object.const_get(params[:questionnaire][:type]).new end @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. case display_type when 'AuthorFeedback' display_type = 'Author%Feedback' when 'CourseSurvey' display_type = 'Course%Survey' when 'TeammateReview' display_type = 'Teammate%Review' when 'GlobalSurvey' display_type = 'Global%Survey' when 'AssignmentSurvey' display_type = 'Assignment%Survey' end @questionnaire.display_type = display_type @questionnaire.instruction_loc = Questionnaire::DEFAULT_QUESTIONNAIRE_URL @questionnaire.save end