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.
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.
Link to the Pull Request Submitted: [1]
link to the deployed project: [2]
Link to the Repository: [3]
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.
*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.
*Removed code redundancy from review_mapping_controller#peer_review_strategy. Long and reusable code were sorted out to form a new helper function.
*Replaced the one in the before(:each) loop by @instructor = build(:instructor, id: 1) and used @instructor class variable, wherever required.
2. Rename variable names and remove hardcoded paramters.
*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.
*Changed :add_reviewer to :assign_reviewer_dynamically.
*Changed :student_review_num to :num_reviews_per_students.
*Changed :submission_review_num to :num_reviews_per_submission.
*Changed :participants_hash to :team_participants_hash.
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. 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 followed the implementation flowchart provided in the above section step by step. In the refactoring process, we made sure that the changes did not break any functionality and passed all the existing test cases. Additionally, we added select_metareviewer to capture edge case scenarios. Test case select_metareviewer makes sure that ReponseMap exists before calling.
The current test suit carefully follows Test Driven Development (TDD) approach, and offers testers automated testing environment using rSpec. The tests take advantages of double and stub features of rSpec gems, and pre-define of different user types for each case for different purpose. Testers can enter "rspec spec/controllers/review_mapping_controller_spec.rb" command in the terminal to execute the automated rspec test suit. After execution of the test cases, result of passing all test cases is shown as below.
Capybara Integration and Functional Tests
Our project passed the Travis CI build test.
Code Coverage
Code Coverage for Controllers section climbed up.
[4] # Link for the COVERALLS stats of our pull request.
Project Mentor
Jialin Cui (jcui9@ncsu.edu)
Team Members
Yi Li (yli273@ncsu.edu)
Zijun Lu (zlu5@ncsu.edu)
Huangxing Chen (hchen63@ncsu.edu)