CSC/ECE 517 Fall 2019 - E1944. Refactor review mapping controller

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

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.

Description of the project

The focus of the project is on a controller named ReviewMappingController and the primary goal is to make changes to the internal structure of the controller to make it easier to read and cheaper to maintain without changing its observable behavior. This can be achieved through refactoring some of the more complex methods, modifying some of the language to make it more Ruby friendly, removing redundant code, etc.

The number of routes directed to review_mapping_controller is illustrated here and since it is intertwined with a lot of other parts in the code base, it is quite imperative that this code needs to be refactored.

Link to the Pull Request Submitted: [1]

Link to the Repository [2]

Work-Plan Followed

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. assign_quiz_controller.rb
4. assign_quiz_controller_spec.rb
5. review_response_map_controller.rb
6. review_response_map_controller_spec.rb
7. routes.rb
8. Other views & partials associated affected by these changes

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

After refactoring the Review_Mapping_Controller.rb, there were some tests still present in the spec file of this controller. So, we removed such tests from the review_mapping_controller_spec.rb to the appropriate spec file.

AssignQuizController

assign_quiz_dynamically (Quizzes are also stored in the Assignment table) is not a seemingly/semantically related task to review mapping. Hence this was moved into a separate controller.

assign_quiz_controller_spec.rb

Tests related to assign_quiz_controller were moved into this file.

ReviewResponseMapController.rb

Add_Calibration is a nuanced method and has seemingly different functionality than Review Mapping Controller. Methods having a different purpose than review_mapping or helping review_mapping should not be present in this controller. So we moved this method into a separate controller.

review_response_map_controller_spec.rb

Tests related to review_response_map were moved into this file.

routes.rb

New routes were added to newly created controllers.

assign_quiz_dynamically_assign_quiz_index POST   /assign_quiz/assign_quiz_dynamically(.:format)  assign_quiz#assign_quiz_dynamically

views/partials

Routes were changed in the views and partials.

Modified View Files:

app/views/student_quizzes/_set_dynamic_quiz.html.erb
app/views/assignments/edit/_calibration.html.erb

Details of the changes made

1. Changed 'instructor = build(:instructor)' to ‘@instructor = build(:instructor, id: 1)’.
       *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. Removed cascading effects of above change from features spec
       *Above changes caused ./spec/features/review_assignment_spec.rb this feature test to fail.
       *This spec has used the above symbol to check if the list of available topics collapse or not after selecting the I don't care option.
before:

after:

4. Created a variable named ‘allowed_actions’ in method choose_case(action_in_params)
       *Switch case in above method from review_mapping_controller.rb contained all the actions having the same output for around 70% of cases.
       *Hence, replaced the switch statements and initialized a list with those switch cases.
       *Even the space complexity increased, the tradeoff got balanced because if someone has to change some actions, he will have to just add or remove the action name from the allowed_actions list.
before:

after:

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. The name add_reviewer may lead the reader to think that the method adds a reviewer to a collection of reviewers (e.g. a list of reviewers). Changing the name to assign_reviewer_manually informs the reader that the method assigns a reviewer (to a submission) and hence improves the readability of the code.

Test Plan

Rspec Unit Tests

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

Note: Tests have been run for three controllers (one existing and two new).

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.

[3] # 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[4]

Regression and Unit Testing of Controllers Demo[5]

Automatic Assignment of Reviewer Demo [6]