CSC/ECE 517 Spring 2016/Refactor review mapping controller.rb: Difference between revisions
Line 253: | Line 253: | ||
</pre> | </pre> | ||
==References== | |||
#[https://github.com/expertiza/expertiza Expertiza on GitHub] | #[https://github.com/expertiza/expertiza Expertiza on GitHub] | ||
#[https://github.com/harsha1007/expertiza Forked repository for the project] | #[https://github.com/harsha1007/expertiza Forked repository for the project] | ||
#[http://expertiza.ncsu.edu/ Expertiza Main Page] | #[http://expertiza.ncsu.edu/ Expertiza Main Page] |
Revision as of 03:35, 24 March 2016
E1615. Refactoring the Review Mapping Controller
This page provides details about the OSS project which was based on refactoring one of controllers related to peer reviewing strategies used in Expertiza.
About Review mapping controller
Review mapping controller contains methods related to peer reviewing strategies. It contains methods to add a reviewer, delete a reviewer, selecting a reviewer. Depending on the number of students and number of submissions, the topics to be reviewed are assigned to the students automatically. If a user wants to look for the team for a submission , it returns the team by comparing the submission id's with the team id's. Also, it assigns quizzes dynamically. Generation of review report, feedback report and teammate review is done.
Code Improvements
1. Unused variables and arguments
There are unused variables in the methods which use the stack unnecessarily. So, it is better to remove the unused variables or at the least indicate that a variable is unused.
For suppose when both keys and values are not used in a hash but are given as arguments, then the unused variables can be indicated by adding a "_" infront of the name or replaced with "_" to represent it as unused variable but allow them arguments.
Previous Code: teams_hash = unsorted_teams_hash.sort_by{|k, v| v}.to_h
After Changing the code: teams_hash = unsorted_teams_hash.sort_by{|_, v| v}.to_h
In the above case teams_hash should consist of a hash with both keys and values but the sorting is done based on values. So the key is replaced with a "_" so that the user may deem it unused in the implementation of the process.
2. Use sample instead of shuffle
When sample is used, the elements in an array are chosen by using random and unique indices in the array so that the elements doesn't repeat in the array. This cannot be guaranteed in shuffle. Also by using shuffle[0] we are shuffling all the elements in the array and then picking the first element instead of picking a single element randomly which is more efficient. The following are the couple of places where shuffle[0] was used and is replaced by sample.
Previous Code: assignment_team = assignment_teams.to_a.shuffle[0] rescue nil topic = assignment.candidate_topics_to_review(reviewer).to_a.shuffle[0] rescue nil
After Changing the code: assignment_team = assignment_teams.to_a.sample rescue nil topic = assignment.candidate_topics_to_review(reviewer).to_a.sample rescue nil
3. Cyclomatic complexity of automatic_review_mapping_strategy method
The method automatic_review_mapping_strategy handles the number of reviews assigned to a individual and also the number of reviews that can be done with in a team. The method is very long and has many nested if statements due to which the complexity of the method is very high. Instead of a single method handling all the parts of the strategy, it is divided into several parts due to which the code is more readable and also the complexity of code is shared by each method.
The method first checks the number of participants that are in an assignment and the number of teams that are present. It then sets the values for the maximum number of reviews that are possible and also the minimum number that are required. Then it assigns the reviews to each of the teams randomly when a request for review is made by a participant. At last it checks if the participant has the minimum number of reviews required after allotting the reviews. If not, it assigns more reviews to valid teams so that the minimum requirement is met.
The part of the code that is moved out of automatic_review_mapping_strategy as peer_review_strategy:
private def peer_review_strategy(teams, num_participants, student_review_num, participants, participants_hash) iterator = 0 teams.each do |team| selected_participants = Array.new if !team.equal? teams.last #need to even out the # of reviews for teams while selected_participants.size < num_reviews_per_team num_participants_this_team = TeamsUser.where(team_id: team.id).size #If there are some submitters or reviewers in this team, they are not treated as normal participants. #They should be removed from 'num_participants_this_team' TeamsUser.where(team_id: team.id).each do |team_user| temp_participant = Participant.where(user_id: team_user.user_id, parent_id: assignment_id).first num_participants_this_team -= 1 if temp_participant.can_review == false or temp_participant.can_submit == false end #if all outstanding participants are already in selected_participants, just break the loop. break if selected_participants.size == participants.size - num_participants_this_team # generate random number if iterator == 0 rand_num = rand(0..num_participants-1) else min_value = participants_hash.values.min #get the temp array including indices of participants, each participant has minimum review number in hash table. participants_with_min_assigned_reviews = Array.new participants.each do |participant| participants_with_min_assigned_reviews << participants.index(participant) if participants_hash[participant.id] == min_value end #if participants_with_min_assigned_reviews is blank if_condition_1 = participants_with_min_assigned_reviews.empty? #or only one element in participants_with_min_assigned_reviews, prohibit one student to review his/her own artifact if_condition_2 = (participants_with_min_assigned_reviews.size == 1 and TeamsUser.exists?(team_id: team.id, user_id: participants[participants_with_min_assigned_reviews[0]].user_id)) if if_condition_1 or if_condition_2 #use original method to get random number rand_num = rand(0..num_participants-1) else #rand_num should be the position of this participant in original array rand_num = participants_with_min_assigned_reviews[rand(0..participants_with_min_assigned_reviews.size-1)] end end # prohibit one student to review his/her own artifact next if TeamsUser.exists?(team_id: team.id, user_id: participants[rand_num].user_id) if_condition_1 = (participants_hash[participants[rand_num].id] < student_review_num) if_condition_2 = (!selected_participants.include? participants[rand_num].id) if if_condition_1 and if_condition_2 # selected_participants cannot include duplicate num selected_participants << participants[rand_num].id participants_hash[participants[rand_num].id] += 1 end # remove students who have already been assigned enough num of reviews out of participants array participants.each do |participant| if participants_hash[participant.id] == student_review_num participants.delete_at(rand_num) num_participants -= 1 end end end else #review num for last team can be different from other teams. #prohibit one student to review his/her own artifact and selected_participants cannot include duplicate num participants.each do |participant| # avoid last team receives too many peer reviews if !TeamsUser.exists?(team_id: team.id, user_id: participant.user_id) and selected_participants.size < num_reviews_per_team selected_participants << participant.id participants_hash[participant.id] += 1 end end end begin selected_participants.each {|index| ReviewResponseMap.where(:reviewee_id => team.id, :reviewer_id => index, :reviewed_object_id => assignment_id).first_or_create} rescue flash[:error] = "Automatic assignment of reviewer failed." end iterator += 1 end end
The method is made private so that it can only be called with in the controller and cannot directly be called through a view.
The complexity of the original method reduced after breaking it and can be easily readable now.
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 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 #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(teams, num_participants, student_review_num, 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 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 = Array.new 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| unless unsorted_teams_hash.has_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| unless 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 end @@time_create_last_review_mapping_record = ReviewResponseMap.where(reviewed_object_id: assignment_id).last.created_at end
4. Moving code specific to models to models instead of controller
The method response_report contains code which generates reports based on the input of one of the three of 'ReviewResponseMap', 'FeedbackResponseMap', 'TeammateReviewResponseMap' and 'Calibration'. Calibration is a special case and should be handled in the controller itself. But the other three are models that are subclasses of ResponseMap model. The report is generated for each of the three by calling the ResponseMap model and obtaining the values by querying the database. Moving the code to the models and calling the methods in the model through the controller makes more sense than writing the code in controller.
The following is the code snippet from the original method which contains the calls to ResponseMap model:
case @type when "ReviewResponseMap" if params[:user].nil? # This is not a search, so find all reviewers for this assignment response_maps_with_distinct_participant_id = ResponseMap.select("DISTINCT reviewer_id").where(["reviewed_object_id = ? and type = ? and calibrate_to = ?", @id, @type, 0]) @reviewers = [] response_maps_with_distinct_participant_id.each do |reviewer_id_from_response_map| @reviewers << (AssignmentParticipant.find(reviewer_id_from_response_map.reviewer_id)) end @reviewers = Participant.sort_by_name(@reviewers) else # This is a search, so find reviewers by user's full name user = User.select("DISTINCT id").where(["fullname LIKE ?", '%'+params[:user][:fullname]+'%']) @reviewers = AssignmentParticipant.where(["user_id IN (?) and parent_id = ?", user, @assignment.id]) end # @review_scores[reveiwer_id][reviewee_id] = score for assignments not using vary_rubric_by_rounds feature # @review_scores[reviewer_id][round][reviewee_id] = score for assignments using vary_rubric_by_rounds feature @review_scores = @assignment.compute_reviews_hash @avg_and_ranges= @assignment.compute_avg_and_ranges_hash when "FeedbackResponseMap" #Example query #SELECT distinct reviewer_id FROM response_maps where type = 'FeedbackResponseMap' and #reviewed_object_id in (select id from responses where #map_id in (select id from response_maps where reviewed_object_id = 722 and type = 'ReviewResponseMap')) @review_response_map_ids = ResponseMap.select("id").where(["reviewed_object_id = ? and type = ?", @id, 'ReviewResponseMap']) @response_ids = Response.select("id").where(["map_id IN (?)", @review_response_map_ids]) @reviewers = ResponseMap.select("DISTINCT reviewer_id").where(["reviewed_object_id IN (?) and type = ?", @response_ids, @type]) when "TeammateReviewResponseMap" #Example query #SELECT distinct reviewer_id FROM response_maps where type = 'TeammateReviewResponseMap' and reviewed_object_id = 711 @reviewers = ResponseMap.select("DISTINCT reviewer_id").where(["reviewed_object_id = ? and type = ?", @id, 'TeammateReviewResponseMap']) end end
The same code after moving the methods to their respective models looks as follows:
case @type 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 @reviewers = FeedbackResponseMap.feedback_response_report(@id, @type) 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) end end