CSC/ECE 517 Fall 2015/oss E1552 NFR

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.

Instructor selected Reviewing

• 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.

Auto selected Reviewing

• 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

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 needed to be done

• Keep only "auto selected" and "instructor 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.

Change in problem statement

While making changes to the helper classes to remove student selected reviewing, a commit was made to the expertiza repository which deleted the page for student selected reviewing. https://github.com/expertiza/expertiza/commit/b62182b4bb42f563ba3cee88917119be378db439

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 was needed to be removed.

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 which needs to be removed.

• dynamic_quiz_assignment_helper.rb (Entire file deleted)
• dynamic_review_assignment_helper.rb (Entire file deleted)
• review_mapping_controller.rb (Only specific method mentioned below deleted)

The method show_available_submissions() in review_mapping_controller.rb was calling student selected reviewing. Since this was the only place the method was being called we decided to delete the below code snippet.

 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)

Also, this was the only call being made to dynamic_review_assignment_helper.rb and this helper is not used in instructor selected and auto selected reviewing strategies. For these strategies another view is being rendered specifically _set_dynamic_review.html.erb. Similarly dynamic_quiz_assignment_helper.rb is also not being used by any controller. Hence the two helper files have been deleted after confirmation from the developer.

Design Patterns

The project involved refactoring code which was used for student selected reviewing. However since this feature was removed from Expertiza during the refactoring, we had to delete the code and hence no design pattern is applicable.

Testing

Since we have deleted the entire helper classes and there has been no code addition, we cannot add any new test cases for deleted code. However, we can manually test to ensure that the code deletion did not cause any broken links by following the steps mentioned below.

Creating an Assignment:

1. Login using instructor.
2. Go to Manage-->Assignments-->New Public Assignment
3. Add couple of topics to the assignments from the topics tab.
4. Check “Allow reviewer to choose which topic to review” and “Enable authors to review others working on same topic” in the same tab.
5. Choose the review strategy as Auto-Selected in Review Strategy tab.
6. Choose appropriate deadlines in the due dates tab.
7. Go to Manage-->Assignments--><Your Assignment Name>
8. Add participants to the assignment.

Signing up and submitting the Assignment:

1. Login using participant1. Select the Assignment from the list of assignments.
2. Sign up for a topic.
3. Submit your work before the deadline.
4. Repeat the same for all participants.

Reviewing work of other Participants:

1. Login using participants. Select the Assignment from the list of assignments.
2. Click on Other’s Work. You will see a list of topics which are eligible for reviewing.

Validation Criteria:

1. You should not be able to review your own submission
2. You can review other submissions of the topic if you have checked “Enable authors to review others working on the same topic” while creating the assignment.
3. You should not be able to review a submission which has already been reviewed by you.
4. You should not be able to review a submission which has reached maximum number of potential reviews.

References

<references/>