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.

Expertiza Background

Expertiza is an educational web application created and maintained by the joint efforts of the students and the faculty at NC State University. It’s an open-source project developed on Ruby on Rails platform and its codebase is available on Github. It facilitates peer review among the students allowing them to improve their work from the feedback. It also assists the faculty in customizing the specifications for the projects and Assignments.

Description of the current project

The following is an Expertiza based OSS project which deals primarily with a controller namely review_mapping_controller.rb. It focusses on refactoring some of the more complex methods, modifying some of the language to make it more Ruby friendly, removing redundant code etc among the other things. The goal of this project is to attempt to make this part of the application easier to read and maintain.

Files modified in the current project

A controller and a helper file were modified for this project namely:
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

Tests were removed from this file which belongs to the isolated methods.

AssignQuizController

Assigning Quizzes (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 that has seemingly different functionality than Review Mapping Controller and hence moved 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.

views/partials

Routes were changed in the views and partials.

Details of changes

1. Changed 'instructor = build(:instructor)' to ‘@instructor = build(:instructor, id: 1)’.
       *Removed code redundancy. 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.
       *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.

4. Created a variable named ‘allowed_actions’ in method choose_case(action_in_params)
5. Refactored Peer_review_strategy by using a helper method gen_random_participant_id
6. Refactored automatic_review_mapping by using a helper method check_num_reviews_args
7. Modularized helper methods (which are not accessed by any other functionality outside the review_mapping_controller) into a module and was mixed in the ReviewMappingController Class.
8. Abided to the principles of Magic Tricks of testing specified in wand did not test any internally used methods, The other tests are written were already following this principle.
9. Isolated AssignQuizDynamically method into a separate controller as the functionality was not related to ReviewMappingController.
10. Associated Specs/routes/views/partials have been modified to adapt the change in controllers.
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.