CSC/ECE 517 Fall 2016/E1641. Refactor review mapping controller.rb: Difference between revisions
Jump to navigation
Jump to search
Line 49: | Line 49: | ||
def assign_reviewers_for_team(assignment_id,student_review_num,participants_hash,exact_num_of_review_needed) | def assign_reviewers_for_team(assignment_id,student_review_num,participants_hash,exact_num_of_review_needed) | ||
if ReviewResponseMap.where(["reviewed_object_id = ? and created_at > ? and calibrate_to = ?", | if ReviewResponseMap.where(["reviewed_object_id = ? and created_at > ? and calibrate_to = ?", | ||
assignment_id, \ | assignment_id, \ | ||
@@time_create_last_review_mapping_record, 0]).size < exact_num_of_review_needed | @@time_create_last_review_mapping_record, 0]).size < exact_num_of_review_needed | ||
Line 57: | Line 57: | ||
end | end | ||
unsorted_teams_hash = {} | unsorted_teams_hash = {} | ||
ReviewResponseMap.where(["reviewed_object_id = ? and calibrate_to = ?", | ReviewResponseMap.where(["reviewed_object_id = ? and calibrate_to = ?", | ||
assignment_id, 0]).each do |response_map| | assignment_id, 0]).each do |response_map| | ||
if unsorted_teams_hash.key? response_map.reviewee_id | if unsorted_teams_hash.key? response_map.reviewee_id | ||
Line 68: | Line 68: | ||
participants_with_insufficient_review_num.each do |participant_id| | participants_with_insufficient_review_num.each do |participant_id| | ||
teams_hash.each do |team_id, _num_review_received| | teams_hash.each do |team_id, _num_review_received| | ||
next if TeamsUser.exists?(team_id: team_id, | next if TeamsUser.exists?(team_id: team_id, | ||
user_id: Participant.find(participant_id).user_id) | user_id: Participant.find(participant_id).user_id) | ||
ReviewResponseMap.where(reviewee_id: team_id, reviewer_id: participant_id, | ReviewResponseMap.where(reviewee_id: team_id, reviewer_id: participant_id, | ||
reviewed_object_id: assignment_id).first_or_create | reviewed_object_id: assignment_id).first_or_create | ||
teams_hash[team_id] += 1 | teams_hash[team_id] += 1 | ||
Line 78: | Line 78: | ||
end | end | ||
end | end | ||
@@time_create_last_review_mapping_record = ReviewResponseMap. | @@time_create_last_review_mapping_record = ReviewResponseMap. | ||
where(reviewed_object_id: assignment_id). | where(reviewed_object_id: assignment_id). | ||
last.created_at | last.created_at | ||
end | end | ||
def execute_peer_review_strategy(assignment_id, teams, num_participants, | def execute_peer_review_strategy(assignment_id, teams, num_participants, | ||
student_review_num, num_reviews_per_team, | student_review_num, num_reviews_per_team, | ||
participants, participants_hash) | participants, participants_hash) | ||
# Exception detection: If instructor want to assign too many reviews done | # Exception detection: If instructor want to assign too many reviews done | ||
# by each student, there will be an error msg. | # by each student, there will be an error msg. | ||
if student_review_num >= teams.size | if student_review_num >= teams.size | ||
flash[:error] = 'You cannot set the number of reviews done | flash[:error] = 'You cannot set the number of reviews done | ||
by each student to be greater than or equal to total number of teams | by each student to be greater than or equal to total number of teams | ||
[or "participants" if it is an individual assignment].' | [or "participants" if it is an individual assignment].' | ||
end | end | ||
peer_review_strategy(assignment_id, teams, num_participants, | peer_review_strategy(assignment_id, teams, num_participants, | ||
student_review_num, num_reviews_per_team, | student_review_num, num_reviews_per_team, | ||
participants, participants_hash) | participants, participants_hash) | ||
end | end | ||
==Test== | ==Test== |
Revision as of 04:11, 29 October 2016
Background
Current Implementation and Problems
- Method names for dealing with the 5 different kinds of objects should be as similar as possible, and they should share helper functions when possible.
- Method response_report has some sql - like code. Rewrite with Active Record.
- Test whether method add_user_to_assignment is used. There is no way that this method should be in ReviewMappingController.--plz remove this method and caller.
- There was a self-review feature, the method add_self_reviewr, get_team_from_submission are related to this. Two views calls add_self_reviewer are ç. The names of views are not related to self_review feature. Plus those two views are not called anywhere.
- Method delete_all_reviewers actually only deletes the outstanding review response maps (the ones which has been initiated, but there is no response yet). So it should better be named delete_outstanding_reviewers. You can try to test this method by clicking “Assign reviewers” icon on an assignment.
- Method release_reservation should be renamed as release_mapping. In addition, delete it if you find this method is not called anywhere.
- Method delete_mappings is problematic. It does not looks like a controller method. Please refactor it or delete it if you can validate that this method is not called anywhere.
- Method automatic_review_mapping_strategy is too long. Please refactor and test it.
Changes Implemented
- By searching the whole project and routes, we verify that methods add_self_reviewr and get_team_from_submission in this controller are not called by any other methods except for views add_self_reviewr, get_team_from_submission. And those two views are not linked to any other views. So they are deleted from the project.
- We already renamed method delete_all_reviewers to delete_outstanding_reviewers and changed the corresponding button name at the corresponding view (_list_review_mappings.html.erb).
- By searching the whole project and routes, we verify that methods release_reservation in this controller are not used in anywhere.
- By searching the whole project and routes, we verify that methods delete_mappings in this controller are not used in anywhere.
- This function have many long lines. We already shorten each long line to multiple lines. And also, we split this long function into three different functions.
def automatic_review_mapping_strategy(assignment_id, participants, teams, student_review_num = 0, submission_review_num = 0) participants_hash = {} participants.each {|participant| participants_hash[participant.id] = 0 } # calculate reviewers for each team num_participants = participants.size if student_review_num != 0 and submission_review_num == 0 num_reviews_per_team = (participants.size * student_review_num * 1.0 / teams.size).round student_review_num = student_review_num exact_num_of_review_needed = participants.size * student_review_num elsif student_review_num == 0 and submission_review_num != 0 num_reviews_per_team = submission_review_num student_review_num = (teams.size * submission_review_num * 1.0 / participants.size).round exact_num_of_review_needed = teams.size * submission_review_num end execute_peer_review_strategy(assignment_id, teams, num_participants, student_review_num, num_reviews_per_team, participants, participants_hash) # after assigning peer reviews for each team, # if there are still some peer reviewers not obtain enough peer review, # just assign them to valid teams assign_reviewers_for_team(assignment_id,student_review_num,participants_hash, exact_num_of_review_needed) end
def assign_reviewers_for_team(assignment_id,student_review_num,participants_hash,exact_num_of_review_needed) if ReviewResponseMap.where(["reviewed_object_id = ? and created_at > ? and calibrate_to = ?", assignment_id, \ @@time_create_last_review_mapping_record, 0]).size < exact_num_of_review_needed participants_with_insufficient_review_num = [] participants_hash.each do |participant_id, review_num| participants_with_insufficient_review_num << participant_id if review_num < student_review_num end unsorted_teams_hash = {} ReviewResponseMap.where(["reviewed_object_id = ? and calibrate_to = ?", assignment_id, 0]).each do |response_map| if unsorted_teams_hash.key? response_map.reviewee_id unsorted_teams_hash[response_map.reviewee_id] += 1 else unsorted_teams_hash[response_map.reviewee_id] = 1 end end teams_hash = unsorted_teams_hash.sort_by {|_, v| v }.to_h participants_with_insufficient_review_num.each do |participant_id| teams_hash.each do |team_id, _num_review_received| next if TeamsUser.exists?(team_id: team_id, user_id: Participant.find(participant_id).user_id) ReviewResponseMap.where(reviewee_id: team_id, reviewer_id: participant_id, reviewed_object_id: assignment_id).first_or_create teams_hash[team_id] += 1 teams_hash = teams_hash.sort_by {|_, v| v }.to_h break end end end @@time_create_last_review_mapping_record = ReviewResponseMap. where(reviewed_object_id: assignment_id). last.created_at end
def execute_peer_review_strategy(assignment_id, teams, num_participants, student_review_num, num_reviews_per_team, participants, participants_hash) # Exception detection: If instructor want to assign too many reviews done # by each student, there will be an error msg. if student_review_num >= teams.size flash[:error] = 'You cannot set the number of reviews done by each student to be greater than or equal to total number of teams [or "participants" if it is an individual assignment].' end peer_review_strategy(assignment_id, teams, num_participants, student_review_num, num_reviews_per_team, participants, participants_hash) end