CSC/ECE 517 Fall 2015/oss E1552 NRR
E1552 Refactoring dynamic_review_assignment_helper.rb and dynamic_quiz_assignment_helper.rb
Overview
Refactoring is the process of changing a software system in such a way that it does not alter the external behavior of the code yet improves its internal structure <ref>Refactoring: Improving the Design of Existing Code - by Martin Fowler, Kent Beck, John Brant, William Opdyke, Don Robert [1]</ref>. It is a disciplined way to clean up code that minimizes the chances of introducing bugs. In essence when you refactor you are improving the design of the code after it has been written. There are many benefits of refactoring as follows: <ref>Refactoring Software using Design Patterns - by Masatomo Noborikawa [2]</ref>
- Refactoring improves the design of software. Refactoring often cleans up codes by deleting duplicates, divides a big chunk of codes into several methods, and makes the program more understandable.
- Because refactoring makes a design cleaner, it helps the programmers understand codes better and see things that may have not been seen before.
- Refactoring helps spot bugs since it makes the software more comprehensible.
- Refactoring turns an adverse design into a good design, which allows for rapid software development.
Peer Reviewing
It is the process of evaluating work done by an individual or a team by another team or an individual with expertise in the concerned area. Types of Reviewing Strategies in Expertiza -
- Instructor Selected Reviewing - Using this strategy the instructor assigns reviews to all participants of the assignment. After selecting this strategy the instructor has the following two options -
- Set number of reviews done by each student.
- Set minimum number of reviews done for each submission.
Then the instructor can click on assign reviewers and map the students to the topics which should be reviewed by them.
- Auto Selected Reviewing - If the instructor selects this strategy, students need to select reviews themselves. Either the student is given a list of topics to choose from and he selects which topic he wants to review, or he checks “I don’t care which topic I review” and he/she will be randomly assigned a topic to review. The instructor can also specify two parameters after selecting this strategy -
- Review topic threshold: - Let us assume Topic 1 has a submission than has not been reviewed. Topic 2 has submissions that has been reviewed atleast 2 times and k is set 2. Then the student will not be able to select topic 2 until all of topic 1’s submission has been reviewed atleast 2 times.
- Maximum number of reviews per submission: - This specifies the maximum number of times a particular top can be reviewed.
- Student Selected Reviewing -
In this strategy the student selects a particular submission to review instead of a particular topic. This feature had never been used and did not work correctly. Hence it has been removed from the system. However there are a lot of places where the code for student selected reviewing exists however it not being called anywhere.
Project Resources
- GitHub Repository
- GitHub Pull Request Link
- VCL Deployment Link (For Instructor - username: instructor6, password: password. For student - username: student5700, password: password)
Problem Statement
Classes involved
dynamic_quiz_assignment_helper.rb dynamic_review_assignment_helper.rb review_mapping_controller.rb
What it does
dynamic_review_assignment_helper.rb allows the system choose a team to review when a student chooses a topic to review. It helps the code choose topics that have submissions with fewer reviews than other topics. A threshold k is used to decide whether a particular topic can be chosen. If, e.g., Topic 1 has a submission that has not yet been reviewed, and Topic 2 has no submission that has been reviewed by fewer than 3 reviewers, then the incoming reviewer can only choose Topic 2 if k ≥ 3. This class does not actually decide what is reviewable, but it does make up a sorted list of the submissions by the number of the reviews they have. dynamic_quiz_assignment_helper.rb is similar, but for quizzes.
What needs to be done
- Remove the code for “student-selected” reviewing
- The find_submissions_in_current_cycle method returns a list of participants. It should return a list of teams.
- Making sure all the conditions mentioned at the beginning of the class are implemented in both the helper classes
- The two classes have a lot of duplicated code. Remove all DRY problems.
Modification of find_submissions_in_current_cycle method
This function returns a list of participants for the particular assignment. This leads to redundant entries in the hash since most teams are made up of more than one member. Earlier the submissions belonged to participants rather than teams, however currently there is no need for this since if an instructor wants individual assignments he/she simply selects maximum number of size of a team as one. Correspondingly changes need to be made to the call sites.
- Before Code Snippet -
def self.find_submissions_in_current_cycle()
if @topic_id.blank?
submissions_in_current_cycle = AssignmentParticipant.where(parent_id: @assignment_id)
else
#using topic_id to find participant.id(s).
signUps = SignedUpTeam.where(topic_id: @topic_id)
signUps.each do |signUp|
users = TeamsUser.where(team_id: signUp.team_id)
users.each do |user|
participant = Participant.where(user_id: user_id, parent_id: @assignment_id)
if participant
submissions_in_current_cycle << participant
end
end
end
end
submissions_in_current_cycle = submissions_in_current_cycle.reject { |submission| !submission.has_submissions? }
# Create a new Hash to store the number of reviews that have already been done (or are in progress) for
# each submission.
@submission_review_count = Hash.new
submissions_in_current_cycle.each do |submission|
# Each 'ResponseMap' entry indicates a review has been performed or is in progress.
existing_maps = ResponseMap.where(reviewee_id: submission.id, reviewed_object_id: @assignment_id )
if existing_maps.nil?
@submission_review_count[submission.id] = 0 # There are no reviews in progress (potential or completed).
else
@submission_review_count[submission.id] = existing_maps.size
end
end
# Sort and return the list of submissions by the number of reviews that they have.
sorted_review_count = @submission_review_count.sort {|a, b| a[1]<=>b[1]}
return sorted_review_count
end
- After Code Snippet -
def self.find_submissions_in_current_cycle()
if @topic_id.blank?
submissions_in_current_cycle = AssignmentTeam.where(parent_id: @assignment_id)
else
#using topic_id to find participant.id(s).
signUps = SignedUpTeam.where(topic_id: @topic_id)
signUps.each do |signUp|
team=Team.where(team_id: signUp.team_id)
if team
submissions_in_current_cycle << team
end
end
end
submissions_in_current_cycle = submissions_in_current_cycle.reject { |submission| !submission.has_submissions? }
# Create a new Hash to store the number of reviews that have already been done (or are in progress) for
# each submission.
@submission_review_count = Hash.new
submissions_in_current_cycle.each do |submission|
# Each 'ResponseMap' entry indicates a review has been performed or is in progress.
existing_maps = ResponseMap.where(reviewee_id: submission.id, reviewed_object_id: @assignment_id )
if existing_maps.nil?
@submission_review_count[submission.id] = 0 # There are no reviews in progress (potential or completed).
else
@submission_review_count[submission.id] = existing_maps.size
end
end
# Sort and return the list of submissions by the number of reviews that they have.
sorted_review_count = @submission_review_count.sort {|a, b| a[1]<=>b[1]}
return sorted_review_count
end
As seen from the above code we have replaced AssignmentParticipant with AssignmentTeam. Now submissions_in_current_cycle will contain list of participants rather than a list of teams. Also note the model for AssignmentTeam contains the same methods which are present in the model for AssignmentParticipant. In the same helper class we had to modify code to handle the aspect that submissions_in_current_cycle now contains teams rather than participants. Hence we made changes in build_submissions_availability().
Implementation of Criteria
Making sure all the conditions mentioned at the beginning of the class are implemented in both the helper classes.
The helper classes while returning a list of teams should check the below set of conditions. Each of the below conditions need to be checked at one place to ensure better code readability and maintainability.
- The article was not written by the potential reviewer.
The potential reviewer should not be able to review its own submission and hence a check for this condition is required. However, it was already implemented in the code beforehand.
- The article was not already reviewed by the potential reviewer.
The potential reviewer should not be able to review a topic which is already reviewed by him/her. The same was also implemented along with the above condition.
- The article is not on the same topic as the potential reviewer has previously written about.
The reviewer should not be able to review on the same topic that he/she submitted as the reviewer could give biased reviews to make his/her own submission seem better. This was not implemented previously and was implemented in the code changes by checking the topic id.
Implementation :
def self.build_submissions_availability()
least_review_count = -1;
max_no_reviews = 5
reviewer_teams=TeamsUser.where(user_id: @reviewer_id)
@submissions_availability = Hash.new
unless @submissions_in_current_cycle.nil?
@submissions_in_current_cycle.each { |submission|
team_check=false
if( least_review_count == -1)
least_review_count = submission[1]
end
reviewer_teams.each do |reviewer_team|
if submission[0] == reviewer_team.team_id
team_check=true
end
end
if !team_check
@submissions_availability[submission[0]] = get_state(least_review_count,submission[1],max_no_reviews)
end
}
end
return @submissions_availability
end
- The article does not already have the maximum number of potential reviews in progress.
The reviewer should only be able to review topics that have not reached the maximum number of reviews. The check for the same is done in get_state method where the current_review_count is compared with max_review_count and make it unavailable.
- The article has the minimum number of reviews for that assignment.
The article is made available if it has the minimum number of reviews for the assignment and the same is checked in get_state method where the current_review_count is compared with least_review_count and make it available.
Implementation:
def self.get_state(least_review_count,current_review_count,max_review_count)
if(current_review_count != -1 && current_review_count == max_review_count)
return -1
elsif(current_review_count != -1 && current_review_count > least_review_count)
return 1
elsif(current_review_count != -1 && current_review_count == least_review_count)
return 0
end
end
Deletion of "student-selected" reviewing
The following files had code for “student-selected” reviewing –
- dynamic_quiz_assignment_helper.rb
- dynamic_review_assignment_helper.rb
- review_mapping_controller.rb
In student-selected reviewing, a student was allowed to review a particular submission and not just a particular topic.The functionality for “student-selected” reviewing was never implemented successfully. Hence, the methods corresponding to the aforementioned functionality were removed from respective files.
Changes were made at the following places -
- dynamic_quiz_assignment_helper.rb
# TODO: Remove this helper, this code is not well-designed.
# Look at Assignment.contributor_to_review() as an example of a better approach
module DynamicQuizAssignmentHelper
# * The article was not written by the potential reviewer.
# * The article was not already reviewed by the potential reviewer.
# * The article is not on the same topic as the potential reviewer has previously written about.
# * The article does not already have the maximum number of potential reviews in progress.
# * The article has the minimum number of reviews for that assignment.
def self.quiz_assignment(assignment_id, reviewer_id, questionnaire_id, quiz_type )
@assignment_id = assignment_id
@current_assignment = Assignment.find(assignment_id)
@reviewer_id = reviewer_id
@questionnaire_id = questionnaire_id
if (quiz_type == Assignment::RS_STUDENT_SELECTED)
@questionnaires = Array.new
@questionnaires << Questionnaire.find(@questionnaire_id)#student_selected_quiz_assignment( )
return @questionnaires
else
return nil
end
end
- dynamic_review_assignment_helper.rb
# TODO: Remove this helper, this code is not well-designed.
# Look at Assignment.contributor_to_review() as an example of a better approach
module DynamicReviewAssignmentHelper
# * The article was not written by the potential reviewer.
# * The article was not already reviewed by the potential reviewer.
# * The article is not on the same topic as the potential reviewer has previously written about.
# * The article does not already have the maximum number of potential reviews in progress.
# * The article has the minimum number of reviews for that assignment.
def self.review_assignment(assignment_id, reviewer_id, topic_id, review_type )
@assignment_id = assignment_id
@current_assignment = Assignment.find(assignment_id)
@reviewer_id = reviewer_id
@topic_id = topic_id
if (review_type == Assignment::RS_STUDENT_SELECTED)
return student_selected_review_assignment( )
else
return nil
end
end
- review_mapping_controller.rb
# Get all the available submissions
def show_available_submissions
assignment = Assignment.find(params[:assignment_id])
reviewer = AssignmentParticipant.where(user_id: params[:reviewer_id], parent_id: assignment.id).first
requested_topic_id = params[:topic_id]
@available_submissions = Hash.new
@available_submissions = DynamicReviewAssignmentHelper::review_assignment(assignment.id, reviewer.id, requested_topic_id, Assignment::RS_STUDENT_SELECTED)
end
References
<references/>