CSC/ECE 517 Fall 2021 - E2124. Refactor review mapping controller.rb

This wiki page is for the description of changes made under E2124 OSS assignment for Fall 2021, CSC/ECE 517.

About Expertiza

Expertiza is an open source project based on Ruby on Rails framework. Expertiza allows the instructor to create new assignments and customize new or existing assignments. It also allows the instructor to create a list of topics the students can sign up for. Students can form teams in Expertiza to work on various projects and assignments. Students can also peer review other students' submissions. Expertiza supports submission across various document types, including the URLs and wiki pages.

Functionality of review_mapping_controller

The functionality of review_mapping_controller is to provide mapping for reviewer and assignment. Basically, the controller handles assignment of reviews to different teams or single student user, such as the event of peer review and self review. Also, this controller is responsible to respond student user request for extra bonus reviews based on assignment policy.

Problem Statement

The review_mapping_controller is a long and complex file. Most of the methods are sparsely commented on. Some methods are way too long to understand, please break them down into pieces for better understanding. Also, the few instances of code duplication that exist should also be removed.

Tasks

-Refactor the long methods in review_mapping_controller.rb
-Rename variable names such as student_review_num, submission_review_num, calibrated_artifacts_num, participants_hash to convey what they are actually used for
-Replace switch statements with subclasses methods
-Create models for the subclasses
-Remove hardcoded parameters

Implementation

We were tasked to refactor the review_mapping_controller.rb and solve any cascading issues or bugs we could find. We followed the above work plan to complete this task. There were many times when all the Rspec and Cucumber tests passed locally but ran into an issue when we uploaded the changes on GitHub. Prompt feedback from the TRAVIS CI helped us recognize the issue. Then we went on local machine and followed the whole process of refactoring again. In this way, we covered every refactoring we did and ensured that the TRAVIS CI get passed with minimum issues on the code-climate.

Files modified/created in the current project

1. review_mapping_controller.rb
2. review_mapping_controller_spec.rb
3. select_reviewer.html.haml
4. app/views/assignments/edit/_calibration.html.erb
5. app/views/review_mapping/select_reviewer.html.haml
6. app/views/student_quizzes/_set_dynamic_quiz.html.erb
7. app/views/student_review/_set_dynamic_review.html.erb
8. config/routes.rb
9. db/schema.rb
10. spec/controllers/review_mapping_controller_spec.rb
11. spec/features/assignment_creation_spec.rb
12. spec/features/review_assignment_spec.rb
13. spec/features/review_mapping_spec.rb

ReviewMappingController

This controller will map the submissions made by the teams to the students for facilitating peer-reviewing. A couple of long and complex methods such as peer_review_strategy and automatic_review_mapping were refactored from this controller along with the removal of some non-related methods such as add_calibration and assign_quiz_dynamically. Variable names have been changed and code has been modularized and helper methods were separated from the important methods into a module and were included in the class.

Test Cases were created for the newly created controllers such as assign_quiz_controller etc.

review_mapping_controller_spec.rb

Added a test in this file.

_set_dynamic_review.html.erb & review_assignment_spec.rb & review_mapping_spec.rb & routes.rb & assignment_creation_review_strategy_spec.rb & assignment_creation_spec.rb

Modified due to the variable name change.

views/partials

Routes were changed in the views and partials.

Modified View Files:

app\views\review_mapping\select_reviewer.html.haml
app/views/student_quizzes/_set_dynamic_quiz.html.erb

Details of the changes made

1. A couple of long and coplex methods such as peer_review_strategy were refactored from this controller.
       *Removed code redundancy from review_mapping_controller_spec.rb. Two variables were being initialized containing the same value. One was in the before(:each) loop and other was being called in first three test cases.
       *Replaced the one in the before(:each) loop by @instructor = build(:instructor, id: 1) and used @instructor class variable, wherever required.

2. Changed :i_dont_care to :no_particular_topic
       *:i_dont_care was used in the /app/views/student_review/_set_dynamic_review.html.erb as a flag to store if student is interested in any particular topic or doesn't care which topic to review.
       *It was also used in review_mapping_controller.rb to check if student has selected any particular topic.
       *Since, name :i_dont_care was very difficult to understand, we replaced it with something logical such as :no_particular_topic. It gives hint about what the symbol stores.

3. Replace switch statements with subclasses methods
       We used "check_num_reviews_args" function to represent the switch statements in "automatic review mapping" function to simply it.       

4. Create models for the subclasses
       We created 3 modules and put relative subclasses methods in to make the controller more organized.       

5. Refactored Peer_review_strategy by using a helper method gen_random_participant_id
       *A random participant_id is generated from the possible pool of candidates but the code block for that is kind of a query, i.e. it does not change or set anything.
       *And it is equally complex enough to confuse the reader. So this has been put into a helper method with an expressive name to increase readability.

  ## Helper Method for generating a random participant which is to be used in peer_review_strategy method.
  def gen_random_participant_id(iterator, participants_hash, num_participants, participants)
    if iterator.zero?
        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 = []
        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
    no_particpants = 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
    participant_is_owner = (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))
    rand_num = if no_particpants or participant_is_owner
                 # use original method to get random number
                 rand(0..num_participants - 1)
               else
                 # rand_num should be the position of this participant in original array
                 participants_with_min_assigned_reviews[rand(0..participants_with_min_assigned_reviews.size - 1)]
               end
    end
    return rand_num
  end

6. Refactored automatic_review_mapping by using a helper method check_num_reviews_args
       *Parameters such as num_reviews_per_student, num_calibrated_artifacts etc passed are first verified to check if they are in acceptable range or pattern
       *To increase readability, the not-so good looking sets of if-else statements have been moved into check_num_reviews_args method.

  # Helper Method to check num_reviews_per_student and num_reviews_per_submission arguments passed in by params hash.
  def check_num_reviews_args(num_reviews_per_student, num_reviews_per_submission, teams)
    has_error_not_raised = true
    # check for exit paths first
    if num_reviews_per_student == 0 and num_reviews_per_submission == 0
      flash[:error] = "Please choose either the number of reviews per student or the number of reviewers per team (student)."
      has_error_not_raised = false
    elsif num_reviews_per_student != 0 and num_reviews_per_submission != 0
      flash[:error] = "Please choose either the number of reviews per student or the number of reviewers per team (student), not both."
      has_error_not_raised = false
    elsif num_reviews_per_student >= teams.size
      # Exception detection: If instructor want to assign too many reviews done
      # by each student, there will be an error msg.
      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].'
      has_error_not_raised = false
    end
  end

7. Modularized helper methods into a module and was mixed in the ReviewMappingController Class.
       *Only some of the methods written in the class have external usage i.e called by another controllers, views etc.
       *The other methods are just helpers and as such moved into a Helper method module and mixed in the class

Before Modularization

  class ReviewMappingController < ApplicationController
    ...................
    ...................
    512 Lines & 25 Methods Defined (After moving a few methods into separate controllers)
    ...................
    ...................
  end  

After Modularization

  module Helper_methods
  ...................
  5 Methods and 170 lines
  ...................
  end

  class ReviewMappingController < ApplicationController
    include Helper_methods
    ...................
    340 Lines & 20 Methods (All of those are used elsewhere directly in the application)
    ...................
  end  

8. Abided to the principles of Magic Tricks of testing and did not test any internally used methods, The other tests are written were already following this principle.
       *Internally used methods were not tested
       *Tests for newly created controllers have been moved into a separate spec files.

9. Isolated AssignQuizDynamically method into a separate controller as the functionality was not related to ReviewMappingController.
       *This method assigns a quiz(Stored as an assignment object) to the participant.
       *This method is not related to ReviewMapping functionality, so it was made into a new controller.

10. Associated Specs/routes/views/partials have been modified to adapt the change in controllers.
       *The controller paths present in the views/partials have to be changed to not to break the functionality
       *The controller paths in views/partials/routes have been changed for the newly created controller of AssignQuizDynamically

View File Affected by the creation of AssinQuizController

11. student_review_num sounds like a number or an id associated with a student review, while it actually stores the number of reviews that a student can perform. So it is renamed num_reviews_per_student.
12. submission_review_num sounds like a number or an id associated with a submission review, while it actually stores the total number of reviews that can be performed on a single submission. So it is renamed num_reviews_per_submission.
13. calibrated_artifacts_num sounds like a number or an id associated with the calibrated artifacts, while it actually stores the number of calibrated artifacts. So it is renamed num_calibrated_artifacts. Similarly, uncalibrated_artifacts_num is renamed num_uncalibrated_artifacts.
14. participants_hash is not an appropriate name for a hash whose keys are participant ids and values are number of reviews performed by corresponding participants. So it is renamed num_reviews_by_participant_hash.
15. Extracted a method make_review_strategy (from automatic_review_mapping_strategy) that returns a review_strategy based on the values of num_reviews_per_submission and num_reviews_per_submission.
16. add_calibration is a method that changes the attribute of a ReviewResponseMap and has little to do with Review Mapping. So it is now put in a separate controller named ReviewResponseMapController.
17. Added one test case and modified "select_metaviewver" to check if a mapping can be found correctly.

Test Plan

Rspec Unit Tests

Since this is a Refactoring Project, We made sure that the changes made did not break any functionality.

Capybara Integration and Functional Tests

As the controller routes have been modified in the routes.rb and the other view files, there are potential chances of failures in Integration tests.

However, no such failure has been reported by Travis build.

Code Coverage

Code Coverage for Controllers section climbed up.

[1] # Link for the COVERALLS stats of our pull request.

Project Mentor

Ramya Vijayakumar (rvijaya4@ncsu.edu)

Team Members

Yaswanth Soodini (ysoodin@ncsu.edu)

Saurabh Shingte (svshingt@ncsu.edu)

Vivek Karri (vkarri@ncsu.edu)

Links for Demo Videos

Assign Reviewer Manually Demo[2]

Regression and Unit Testing of Controllers Demo[3]

Automatic Assignment of Reviewer Demo [4]