CSC/ECE 517 Spring 2018- Project E1808: Refactor review mapping controller.rb: Difference between revisions
(Created page with "== Introduction == === Expertiza === [http://expertiza.ncsu.edu/ Expertiza] is a platform through which students are able to view and manage their assignments, form teams for a g...") |
No edit summary |
||
Line 20: | Line 20: | ||
=== Refactoring === | === 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 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: | |||
<pre> | |||
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 | |||
</pre> | |||
After Refactoring: | |||
<pre> | |||
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(flash,params) | |||
helper.assign_reviews_for_artifacts_num_zero(flash,params) {automatic_review_mapping_strategy(helper.assignment_id, helper.participants, helper.teams, helper.student_review_num, helper.submission_review_num)} | |||
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 | |||
</pre> |
Revision as of 04:55, 26 March 2018
Introduction
Expertiza
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 replace the long switch statements
- Move switch conditions to corresponding subclasses with same method name response_report
- Create corresponding helper classes for different report types if they do not exist
- Replace the conditional with the relevant method calls
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(flash,params) helper.assign_reviews_for_artifacts_num_zero(flash,params) {automatic_review_mapping_strategy(helper.assignment_id, helper.participants, helper.teams, helper.student_review_num, helper.submission_review_num)} 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