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.

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

• The find_submissions_in_current_cycle method returns a list of participants. It should return a list of teams. Returning a list of participants leads to redundant entries in the hash, if any are done by multi-member teams. There used to be a need for dealing with participants rather than teams, but in the current Expertiza, submissions always belong to teams rather than individuals. (This may also require changing the call site for some calls to methods of this class.)
• The comment at the beginning of the class lists a number of criteria that have to hold for a submission to be reviewable (e.g., it was not written by the potential reviewer, it has not already been reviewed by the potential reviewer). It’s not clear where these conditions are checked for. Probably the different conditions are checked for in different places. Sometimes they are checked for late (e.g., if a user has written on Topic 2, (s)he is allowed to select Topic 2 as a topic to review, but after (s)he selects it, (s)he is told that there are no more submissions to review on that topic). It would be better to check for all of these conditions in dynamic_review_assignment_helper; then the user would not even be able to select topics that have no available reviews.
• There is code for “student-selected” reviewing. This would have allowed a student to choose a particular submission, not just a particular topic. This feature had never been used, and did not work correctly, so we removed it from the system. Thus, the code for student-selected reviewing should be removed.
• 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 want an individual project we simply select 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 present in the model for AssignmentTeam. 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. However, the reviewer could have got his own submission if it had been the one with least reviews. This was an undesirable aspect of student-selected reviewing. 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/>