CSC/ECE 517 Spring 2018- Project E1808: Refactor review mapping controller.rb: Difference between revisions
Line 542: | Line 542: | ||
#Install VirtualBox and import this image to the VirtualBox. | #Install VirtualBox and import this image to the VirtualBox. | ||
#clone the repo (steps 2 to 5 are executed from the terminal in the image.) | #clone the repo (steps 2 to 5 are executed from the terminal in the image.) | ||
git clone | :''git clone https://github.com/XEllis/expertiza.git'' | ||
#. go to expertiza folder | #. go to expertiza folder | ||
''cd expertiza'' | :''cd expertiza'' | ||
#run bash setup.sh | #run bash setup.sh | ||
''bash setup.sh'' | :''bash setup.sh'' | ||
#run bundle install | #run bundle install | ||
''bundle install'' | :''bundle install'' | ||
#run rails server | #run rails server | ||
''rails server'' | :''rails server'' | ||
#Use firefox in the image and test response_report method | #Use firefox in the image and test response_report method | ||
##Type in http://0.0.0.0:3000/review_mapping/response_report?id=772 | ##Type in http://0.0.0.0:3000/review_mapping/response_report?id=772 |
Revision as of 23:24, 2 April 2018
Introduction
Expertiza Overview
Expertiza is a platform through which students are able to view and manage their assignments, form teams for a group project and review other team's work for improvement. It uses Ruby on Rails Framework
Objectives of the Project
- Refactor automatic_review_mapping method
- Write failing tests first
- Split into several simpler methods and assign reasonable names
- Extract duplicated code into separate methods
- Refactor response_report method
- Write failing tests first
- Use factory pattern to create a corresponding report according to the switch condition
- Create corresponding helper classes for different report types
Implementation
Files editted
- app/views/review_mapping/
- _answer_tagging_report.html.erb
- _calibration_report.html.erb
- _feedback_report.html.erb
- _plagiarism_checker_report.html.erb
- _review_report.html.erb
- _self_review_report.html.erb
- _summary_report.html.haml
- _summary_reviewee_report.html.haml
- _team_score.html.erb
- _teammate_review_report.html.erb
- app/views/user_pastebins/
- _save_text_macros.html.erb
- app/controllers/
- review_mapping_controller.rb
- app/helpers/
- response_report_helper.rb
- review_mapping_helper.rb
- automatic_review_mapping_helper.rb
About Review Mapping Controller
Review mapping controller handles the review responses that are done on every assignment. It assigns reviews to different teams or individual student depending on the type of assignment(team project or individual project). It keeps track that all the teams are reviewed and assigned teams to review too.
Refactoring
Refactoring is used to improve a design code by changing the structure of the code such that the functionality remains the same. The changes made are quite small, but the overall effect becomes significant.
Refactoring automatic_review_mapping method
As this method was long and complex it needed to be broken down into smaller methods with specific functionality.
Original:
def automatic_review_mapping assignment_id = params[:id].to_i participants = AssignmentParticipant.where(parent_id: params[:id].to_i).to_a.reject {|p| p.can_review == false }.shuffle! teams = AssignmentTeam.where(parent_id: params[:id].to_i).to_a.shuffle! max_team_size = Integer(params[:max_team_size]) # Assignment.find(assignment_id).max_team_size # Create teams if its an individual assignment. if teams.empty? and max_team_size == 1 participants.each do |participant| user = participant.user next if TeamsUser.team_id(assignment_id, user.id) team = AssignmentTeam.create_team_and_node(assignment_id) ApplicationController.helpers.create_team_users(participant.user, team.id) teams << team end end student_review_num = params[:num_reviews_per_student].to_i submission_review_num = params[:num_reviews_per_submission].to_i calibrated_artifacts_num = params[:num_calibrated_artifacts].to_i uncalibrated_artifacts_num = params[:num_uncalibrated_artifacts].to_i if calibrated_artifacts_num == 0 and uncalibrated_artifacts_num == 0 if student_review_num == 0 and submission_review_num == 0 flash[:error] = "Please choose either the number of reviews per student or the number of reviewers per team (student)." elsif (student_review_num != 0 and submission_review_num == 0) or (student_review_num == 0 and submission_review_num != 0) # REVIEW: mapping strategy automatic_review_mapping_strategy(assignment_id, participants, teams, student_review_num, submission_review_num) else flash[:error] = "Please choose either the number of reviews per student or the number of reviewers per team (student), not both." end else teams_with_calibrated_artifacts = [] teams_with_uncalibrated_artifacts = [] ReviewResponseMap.where(reviewed_object_id: assignment_id, calibrate_to: 1).each do |response_map| teams_with_calibrated_artifacts << AssignmentTeam.find(response_map.reviewee_id) end teams_with_uncalibrated_artifacts = teams - teams_with_calibrated_artifacts # REVIEW: mapping strategy automatic_review_mapping_strategy(assignment_id, participants, teams_with_calibrated_artifacts.shuffle!, calibrated_artifacts_num, 0) # REVIEW: mapping strategy # since after first mapping, participants (delete_at) will be nil participants = AssignmentParticipant.where(parent_id: params[:id].to_i).to_a.reject {|p| p.can_review == false }.shuffle! automatic_review_mapping_strategy(assignment_id, participants, teams_with_uncalibrated_artifacts.shuffle!, uncalibrated_artifacts_num, 0) end redirect_to action: 'list_mappings', id: assignment_id end
After Refactoring:
def automatic_review_mapping count = 0 # this variable is used to control whether the calibrated or the uncalibrated automatic_review_map_controller_strategy will be called on yield. helper = AutomaticReviewMappingHelper::AutomaticReviewMappingHelper.new(params) # Create teams if its an individual assignment. helper.create_teams_if_individual_assignment if helper.check_if_all_artifacts_num_are_zero(params) helper.assign_reviews_for_artifacts_num_zero(params) do |error| if error flash[:error] = error.message else automatic_review_mapping_strategy(helper.assignment_id, helper.participants, helper.teams, helper.student_review_num, helper.submission_review_num) end end else helper.assign_reviews_for_artifacts_num_not_zero(params) do automatic_review_mapping_strategy(helper.assignment_id, helper.participants, helper.teams_with_calibrated_artifacts.shuffle!, helper.calibrated_artifacts_num, 0) if count == 0 count = count+1 if count == 2 automatic_review_mapping_strategy(helper.assignment_id, helper.participants, helper.teams_with_uncalibrated_artifacts.shuffle!, helper.uncalibrated_artifacts_num, 0) count == 0 end end end redirect_to action: 'list_mappings', id: helper.assignment_id end
Approach
As seen, the method originally was very long and complex. So, the method was broken down into smaller methods. These smaller methods perform various tasks in the review mapping controller. As tey show similar behavior they were grouped in a helper class: AutomaticReviewMappingHelper.
Methods
- initialize
- create_teams_if_individual_assignment
- check_if_all_artifacts_num_are_zero
- assign_reviews_for_artifacts_num_zero
- assign_reviews_for_artifacts_num_not_zero
The common parameters that are used in the automatic review mapping method are defined here so that they can be used as instance variables throughout for better flow of the code.
def initialize(params) @assignment_id = params[:id].to_i @participants = AssignmentParticipant.where(parent_id: params[:id].to_i).to_a.reject {|p| p.can_review == false }.shuffle! @teams = AssignmentTeam.where(parent_id: params[:id].to_i).to_a.shuffle! @max_team_size = Integer(params[:max_team_size]) # Assignment.find(assignment_id).max_team_size @student_review_num = params[:num_reviews_per_student].to_i @submission_review_num = params[:num_reviews_per_submission].to_i @calibrated_artifacts_num = params[:num_calibrated_artifacts].to_i @uncalibrated_artifacts_num = params[:num_uncalibrated_artifacts].to_i @teams_with_calibrated_artifacts = [] @teams_with_uncalibrated_artifacts = [] end
def create_teams_if_individual_assignment if @teams.empty? and @max_team_size == 1 @participants.each do |participant| next if TeamsUser.team_id(@assignment_id, participant.user.id) team = AssignmentTeam.create_team_and_node(@assignment_id) ApplicationController.helpers.create_team_users(participant.user, team.id) @teams << team end end end
This method checks for the value of num_calibrated_artifacts and uncalibrated_artifacts. If both are zero, it returns true to the controller and the controller then calls the assign_reviews_for_artifacts_num_zero method. If both the values are not zero, 'false' is returned to the controller and controller calls assign_reviews_for_artifacts_num_not_zero method
def check_if_all_artifacts_num_are_zero(params) if @calibrated_artifacts_num == 0 and @uncalibrated_artifacts_num == 0 true else false end end
Use of 'Yield'
The keyword yield is used in the two helper class methods( assign_reviews_for_artifacts_num_zero and assign_reviews_for_artifacts_num_not_zero) as the functionality of these two methods require to call a controller method(automatic_review_mapping_strategy) inside them. As it is not a good practice to call controller methods in helper class, we transfer the control from the helper class method back to the controller so that we don't have to create controller object in the helper class. The variable count in the controller is used to differentiate the call of automatic_review_mapping_strategy method. The value of the count decides which method call is associated with the yield in the helper method.
Exceptions in Helper Methods
As it not a good practice to have flash errors in helper methods(as flash needs to be passed as a parameter) exceptions are raised in helper class methods. So when these exceptions are raised in the helper class, they are picked up by the controller and controller displays the corresponding error message associated with it. This reduces the complexity of the code.
def assign_reviews_for_artifacts_num_zero(params) if @student_review_num == 0 and @submission_review_num == 0 begin raise "Please choose either the number of reviews per student or the number of reviewers per team (student)." rescue Exception => e yield e end elsif (@student_review_num != 0 and @submission_review_num == 0) or (@tudent_review_num == 0 and @submission_review_num != 0) # REVIEW: mapping strategy yield else begin raise "Please choose either the number of reviews per student or the number of reviewers per team (student), not both." rescue Exception => e yield e end end end
def assign_reviews_for_artifacts_num_not_zero(params) ReviewResponseMap.where(reviewed_object_id: @assignment_id, calibrate_to: 1).each do |response_map| @teams_with_calibrated_artifacts << AssignmentTeam.find(response_map.reviewee_id) end @teams_with_uncalibrated_artifacts = @teams - @teams_with_calibrated_artifacts # REVIEW: mapping strategy yield # REVIEW: mapping strategy # since after first mapping, participants (delete_at) will be nil @participants = AssignmentParticipant.where(parent_id: params[:id].to_i).to_a.reject {|p| p.can_review == false }.shuffle! yield end
Controller Method:
if helper.check_if_all_artifacts_num_are_zero(params) helper.assign_reviews_for_artifacts_num_zero(params) do |error| if error flash[:error] = error.message else automatic_review_mapping_strategy(helper.assignment_id, helper.participants, helper.teams, helper.student_review_num, helper.submission_review_num) end end else helper.assign_reviews_for_artifacts_num_not_zero(params) do automatic_review_mapping_strategy(helper.assignment_id, helper.participants, helper.teams_with_calibrated_artifacts.shuffle!, helper.calibrated_artifacts_num, 0) if count == 0 count = count+1 if count == 2 automatic_review_mapping_strategy(helper.assignment_id, helper.participants, helper.teams_with_uncalibrated_artifacts.shuffle!, helper.uncalibrated_artifacts_num, 0) count == 0 end end
Refactoring response_report method
This method consisted of long case statements making it a long and complex to understand. So this method was refactored using Factory Design Pattern.
Original
def response_report # Get the assignment id and set it in an instance variable which will be used in view @id = params[:id] @assignment = Assignment.find(@id) # ACS Removed the if condition(and corressponding else) which differentiate assignments as team and individual assignments # to treat all assignments as team assignments @type = params.key?(:report) ? params[:report][:type] : "ReviewResponseMap" summary_ws_url = WEBSERVICE_CONFIG["summary_webservice_url"] case @type # this summarizes the reviews of each reviewee by each rubric criterion when "SummaryByRevieweeAndCriteria" sum = SummaryHelper::Summary.new.summarize_reviews_by_reviewees(@assignment, summary_ws_url) # list of variables used in the view and the parameters (should have been done as objects instead of hash maps) # @summary[reviewee][round][question] # @reviewers[team][reviewer] # @avg_scores_by_reviewee[team] # @avg_score_round[reviewee][round] # @avg_scores_by_criterion[reviewee][round][criterion] @summary = sum.summary @reviewers = sum.reviewers @avg_scores_by_reviewee = sum.avg_scores_by_reviewee @avg_scores_by_round = sum.avg_scores_by_round @avg_scores_by_criterion = sum.avg_scores_by_criterion # this summarizes all reviews by each rubric criterion when "SummaryByCriteria" sum = SummaryHelper::Summary.new.summarize_reviews_by_criterion(@assignment, summary_ws_url) @summary = sum.summary @avg_scores_by_round = sum.avg_scores_by_round @avg_scores_by_criterion = sum.avg_scores_by_criterion when "ReviewResponseMap" @review_user = params[:user] # If review response is required call review_response_report method in review_response_map model @reviewers = ReviewResponseMap.review_response_report(@id, @assignment, @type, @review_user) @review_scores = @assignment.compute_reviews_hash @avg_and_ranges = @assignment.compute_avg_and_ranges_hash when "FeedbackResponseMap" # If review report for feedback is required call feedback_response_report method in feedback_review_response_map model if @assignment.varying_rubrics_by_round? @authors, @all_review_response_ids_round_one, @all_review_response_ids_round_two, @all_review_response_ids_round_three = FeedbackResponseMap.feedback_response_report(@id, @type) else @authors, @all_review_response_ids = FeedbackResponseMap.feedback_response_report(@id, @type) end when "TeammateReviewResponseMap" # If review report for teammate is required call teammate_response_report method in teammate_review_response_map model @reviewers = TeammateReviewResponseMap.teammate_response_report(@id) when "Calibration" participant = AssignmentParticipant.where(parent_id: params[:id], user_id: session[:user].id).first rescue nil if participant.nil? participant = AssignmentParticipant.create(parent_id: params[:id], user_id: session[:user].id, can_submit: 1, can_review: 1, can_take_quiz: 1, handle: 'handle') end @review_questionnaire_ids = ReviewQuestionnaire.select("id") @assignment_questionnaire = AssignmentQuestionnaire.where(assignment_id: params[:id], questionnaire_id: @review_questionnaire_ids).first @questions = @assignment_questionnaire.questionnaire.questions.select {|q| q.type == 'Criterion' or q.type == 'Scale' } @calibration_response_maps = ReviewResponseMap.where(reviewed_object_id: params[:id], calibrate_to: 1) @review_response_map_ids = ReviewResponseMap.select('id').where(reviewed_object_id: params[:id], calibrate_to: 0) @responses = Response.where(map_id: @review_response_map_ids) when "PlagiarismCheckerReport" @plagiarism_checker_comparisons = PlagiarismCheckerComparison.where(plagiarism_checker_assignment_submission_id: PlagiarismCheckerAssignmentSubmission.where(assignment_id: params[:id]).pluck(:id)) when "AnswerTaggingReport" tag_prompt_deployments = TagPromptDeployment.where(assignment_id: params[:id]) @questionnaire_tagging_report = {} tag_prompt_deployments.each do |tag_dep| @questionnaire_tagging_report[tag_dep] = tag_dep.assignment_tagging_progress end when "SelfReview" @self_review_response_maps = SelfReviewResponseMap.where(reviewed_object_id: @id) end @user_pastebins = UserPastebin.get_current_user_pastebin current_user end
After Refactoring
def response_report # Get the assignment id and set it in an instance variable which will be used in view @id = params[:id] @assignment = Assignment.find(@id) # ACS Removed the if condition(and corressponding else) which differentiate assignments as team and individual assignments # to treat all assignments as team assignments @type = params.key?(:report) ? params[:report][:type] : "ReviewResponseMap" review_user = params[:user] @response_report_result = ResponseReportHelper::ResponseReportFactory.new.create_response_report(@id, @assignment, @type, review_user) @user_pastebins = UserPastebin.get_current_user_pastebin current_user end
Approach
As seen, the controller method originally was sharing a lot of instance variables with the corresponding views, and it has the long switch statements. Each switch condition is corresponding to a different type of report. Thus, different report helper classes are created for different types of report. These helper classes are grouped in the helper module: ResponseReportHelper.
# SummaryByRevieweeAndCriteria class SummaryRevieweeReport def initialize(assignment, summary_ws_url) sum = SummaryHelper::Summary.new.summarize_reviews_by_reviewees(assignment, summary_ws_url) @summary = sum.summary @reviewers = sum.reviewers @avg_scores_by_reviewee = sum.avg_scores_by_reviewee @avg_scores_by_round = sum.avg_scores_by_round @avg_scores_by_criterion = sum.avg_scores_by_criterion end attr_reader :summary attr_reader :reviewers attr_reader :avg_scores_by_reviewee attr_reader :avg_scores_by_round attr_reader :avg_scores_by_criterion end
# SummaryByCriteria class SummaryReport def initialize(assignment, summary_ws_url) sum = SummaryHelper::Summary.new.summarize_reviews_by_criterion(assignment, summary_ws_url) @summary = sum.summary @avg_scores_by_round = sum.avg_scores_by_round @avg_scores_by_criterion = sum.avg_scores_by_criterion end attr_reader :summary attr_reader :avg_scores_by_round attr_reader :avg_scores_by_criterion end
# ReviewResponseMap class ReviewReport def initialize(id, assignment, type, review_user) @reviewers = ReviewResponseMap.review_response_report(id, assignment, type, review_user) @review_scores = assignment.compute_reviews_hash @avg_and_ranges = assignment.compute_avg_and_ranges_hash end attr_reader :reviewers attr_reader :review_scores attr_reader :avg_and_ranges end
# FeedbackResponseMap class FeedbackReport def initialize(id, assignment, type) if assignment.varying_rubrics_by_round? @authors, @all_review_response_ids_round_one, @all_review_response_ids_round_two, @all_review_response_ids_round_three = FeedbackResponseMap.feedback_response_report(id, type) else @authors, @all_review_response_ids = FeedbackResponseMap.feedback_response_report(id, type) end end attr_reader :authors attr_reader :all_review_response_ids_round_one attr_reader :all_review_response_ids_round_two attr_reader :all_review_response_ids_round_three attr_reader :all_review_response_ids end
# TeammateReviewResponseMap class TeammateReviewReport def initialize(id) @reviewers = TeammateReviewResponseMap.teammate_response_report(id) end attr_reader :reviewers end
# Calibration class CalibrationReport def initialize(id) review_questionnaire_ids = ReviewQuestionnaire.select('id') @assignment_questionnaire = AssignmentQuestionnaire.where(assignment_id: id, questionnaire_id: review_questionnaire_ids).first @questions = @assignment_questionnaire.questionnaire.questions.select {|q| q.type == 'Criterion' or q.type == 'Scale' } @calibration_response_maps = ReviewResponseMap.where(reviewed_object_id: id, calibrate_to: 1) review_response_map_ids = ReviewResponseMap.select('id').where(reviewed_object_id: id, calibrate_to: 0) @responses = Response.where(map_id: review_response_map_ids) end attr_reader :assignment_questionnaire attr_reader :questions attr_reader :calibration_response_maps attr_reader :responses end
# PlagiarismCheckerReport class PlagiarismCheckerReport def initialize(id) @plagiarism_checker_comparisons = PlagiarismCheckerComparison.where(plagiarism_checker_assignment_submission_id: PlagiarismCheckerAssignmentSubmission.where(assignment_id: id).pluck(:id)) end attr_reader :plagiarism_checker_comparisons end
# AnswerTaggingReport class AnswerTaggingReport def initialize(id) tag_prompt_deployments = TagPromptDeployment.where(assignment_id: id) @questionnaire_tagging_report = {} tag_prompt_deployments.each do |tag_dep| @questionnaire_tagging_report[tag_dep] = tag_dep.assignment_tagging_progress end end attr_reader :questionnaire_tagging_report end
# SelfReview class SelfReviewReport def initialize(id) @self_review_response_maps = SelfReviewResponseMap.where(reviewed_object_id: id) end attr_reader :self_review_response_maps end
The Factory Pattern is used to create a corresponding type of report, according to the switch condition.
# ResponseReportFactory class ResponseReportFactory def create_response_report (id, assignment, type, review_user) summary_ws_url = WEBSERVICE_CONFIG["summary_webservice_url"] case type when "SummaryByRevieweeAndCriteria" SummaryRevieweeReport.new(assignment, summary_ws_url) when "SummaryByCriteria" SummaryReport.new(assignment, summary_ws_url) when "ReviewResponseMap" ReviewReport.new(id, assignment, type, review_user) when "FeedbackResponseMap" FeedbackReport.new(id, assignment, type) when "TeammateReviewResponseMap" TeammateReviewReport.new(id) when "Calibration" CalibrationReport.new(id) when "PlagiarismCheckerReport" PlagiarismCheckerReport.new(id) when "AnswerTaggingReport" AnswerTaggingReport.new(id) when "SelfReview" SelfReviewReport.new(id) end end end
Results
Tests for the two methods were already present in review_mapping_controller_spec.rb So after the refactoring was done all the tests pass reflecting that the functionality of the code remains same after refactoring.
Tests passed for Revised Code:
Steps for Manual Testing
- Download /Ubuntu-Expertiza image (.OVA)
(Download an Ubuntu-Expertiza image (.OVA) including latest DB.)
- Install VirtualBox and import this image to the VirtualBox.
- clone the repo (steps 2 to 5 are executed from the terminal in the image.)
- git clone https://github.com/XEllis/expertiza.git
- . go to expertiza folder
- cd expertiza
- run bash setup.sh
- bash setup.sh
- run bundle install
- bundle install
- run rails server
- rails server
- Use firefox in the image and test response_report method
- If you need to log in, use account name “instructor6”, and password “password”.
- 2 In order to view different reports, select different types of report and click “Submit” button.
Image:
- Use firefox browser in the image and test automatic_review_mapping method
- Type in http://0.0.0.0:3000/assignments/720/edit#tabs-4
- If you need to log in, use account name “instructor6”, and password “password”.
- select Insturctor-Selected from Review Strategy dropdown
Image:
- Here are three different strategies to assign reviewers. In order to assign reviewers, enter a number in the text field and click “Assign reviewers” or “Assign both calibrated and uncalibrated artifacts” button.
Image: Image: Image:
References
Team Members
- Sanika Sabnis
- Saurabh Labde
- Xiaohui Ellis