CSC/ECE 517 Fall 2023 - E2356. Refactor review mapping controller.rb: Difference between revisions
(18 intermediate revisions by 2 users not shown) | |||
Line 7: | Line 7: | ||
== About Controller == | == About Controller == | ||
The functionality of review_mapping_controller is to | The functionality of '''review_mapping_controller''' is to map the reviewer to an assignment. Basically, the controller handles the assignment of reviews to different teams or individual student users, covering scenarios like peer reviews and self-reviews. Additionally, the controller plays a crucial role in handling requests from student users, facilitating extra bonus reviews in alignment with the assignment policy. | ||
== Problem Statement== | == Problem Statement== | ||
Line 18: | Line 18: | ||
== Design Pattern == | == Design Pattern == | ||
During the code refactoring process, we conscientiously followed various design patterns to enhance the overall structure. One of the most common ones is the “Extract Method.” There were several instances where certain methods became overly lengthy and intricate. By extracting specific functionalities into separate methods, we significantly improved the code's readability and comprehension. This way, it became easier and clearer to understand what the method achieved. Furthermore, we tackled the issue of excessive conditional statements by employing the "Refactoring Conditionals" design pattern. This approach involves isolating the block of code within a conditional statement into a distinct method and subsequently invoking that method. This strategic refactoring not only streamlined our code but also contributed to a more readable and maintainable codebase. | |||
[[File:flow_Diagram_review_mapping_controller.png|1000px]] | [[File:flow_Diagram_review_mapping_controller.png|1000px]] | ||
== File(s) Modified == | == File(s) Modified == | ||
1. review_mapping_controller.rb<br> | 1. <code>review_mapping_controller.rb</code><br> | ||
2. review_mapping_controller_spec.rb | 2. <code>review_mapping_controller_spec.rb</code><br> | ||
Changes can be seen in the pull request created [https://github.com/expertiza/expertiza/pull/2657/files here]. | |||
==Solutions/Details of Changes Made== | ==Solutions/Details of Changes Made== | ||
Line 62: | Line 62: | ||
v) Describe functionality of '''delete_reviewer''', '''delete_metareviewer''', '''delete_metareview''', and several other functions. | v) Describe functionality of '''delete_reviewer''', '''delete_metareviewer''', '''delete_metareview''', and several other functions. | ||
[[File:Comment4_review_mapping.png| | [[File:Comment4_review_mapping.png|1000px]] | ||
[[File:Comment1_review_mapping.png| | [[File:Comment1_review_mapping.png|1000px]] | ||
[[File:Comment3_review_mapping.png| | [[File:Comment3_review_mapping.png|1000px]] | ||
b) Edit Comments | b) Edit Comments | ||
Line 153: | Line 153: | ||
== Test Plan== | == Test Plan== | ||
In the realm of Test-Driven Development (TDD) research, our team made the strategic decision to utilize Mustafa Olmez's meticulously crafted test skeletons. These skeletons provided us with invaluable guidance, outlining the essential tests required for our unit. '''By incorporating 20 of these meticulously designed test skeletons into our work, we were able to conduct rigorous testing of our controller module.''' This approach allowed us to delve deeply into the functionality of our controller, ensuring thorough examination and validation of its behavior. The integration of these test skeletons not only provided a robust framework for our unit tests but also enhanced the overall quality and reliability of our codebase. | === Test Plan, Testing Strategy and Results=== | ||
In the realm of Test-Driven Development (TDD) research, our team made the strategic decision to '''utilize Mustafa Olmez's meticulously crafted test skeletons'''. These skeletons provided us with invaluable guidance, outlining the essential tests required for our unit. '''By incorporating 20 of these meticulously designed test skeletons into our work, we were able to conduct rigorous testing of our controller module.''' This approach allowed us to delve deeply into the functionality of our controller, ensuring thorough examination and validation of its behavior. The integration of these test skeletons not only provided a robust framework for our unit tests but also enhanced the overall quality and reliability of our codebase. | |||
For a detailed view of the changes made, please refer to the complete file at the following link: | For a detailed view of the changes made, please refer to the complete file at the following link: | ||
Line 159: | Line 160: | ||
https://github.com/expertiza/expertiza/pull/2657/files#diff-bccc23eb7ba2a55b74837eed3f4c85aa1d8c55d356ce0e0bc7de7afd50121b13 | https://github.com/expertiza/expertiza/pull/2657/files#diff-bccc23eb7ba2a55b74837eed3f4c85aa1d8c55d356ce0e0bc7de7afd50121b13 | ||
The additional tests ensure every method of review_mapping_controller is tested. By adopting this approach we have '''increased the coverage from 47.65% to 47.8%'''. | The additional tests ensure every method of review_mapping_controller is tested. By adopting this approach we have '''increased the overall coverage from 47.65% to 47.8%'''. | ||
The changes made passed all the necessary Travis CI/CD Pipeline tests. | |||
[[File:Test1ReviewMapping.png|750px]] | |||
===SimpleCov Coverage Detail Report of <code>review_mapping_controller.rb</code>=== | |||
Tests prior to the changes covered 69.65 % of <code>review_mapping_controller.rb</code> | |||
[[File:Old_coverage_report_simplecov.png|1000px]] | |||
[[File: | After adding tests, the tests covered 69.76 % of refactored <code>review_mapping_controller.rb</code>. It improved slightly to be an even better coverage. | ||
[[File:New_coverage_report_simplecov.png|1000px]] | |||
== Relevant Links == | |||
* '''Github Repository:''' https://github.com/amit-99/NCSU_OODD_expertiza | |||
* '''Pull Request:''' https://github.com/expertiza/expertiza/pull/2657 | |||
* '''VCL Server:''' http://152.7.178.252:8080/ | |||
== Team == | == Team == |
Latest revision as of 19:17, 30 November 2023
This wiki page is for information regarding the changes made for the E2356 Refactor review_mapping_controller.rb OSS assignment for Fall 2023, CSC/ECE 517.
Introduction
Expertiza is an open-source project developed on Ruby on Rails. This web application is maintained by the students and faculty at NC State. This application gives complete control to the instructor to maintain the assignments in their class. With multiple functionalities such as adding topics, creating groups, and peer reviews, Expertiza is a well-developed application that can handle all types of assignments. To learn more about the full functionality Expertiza has to offer, visit the Expertiza wiki.
About Controller
The functionality of review_mapping_controller is to map the reviewer to an assignment. Basically, the controller handles the assignment of reviews to different teams or individual student users, covering scenarios like peer reviews and self-reviews. Additionally, the controller plays a crucial role in handling requests from student users, facilitating extra bonus reviews in alignment with the assignment policy.
Problem Statement
- Refactor the long methods in review_mapping_controller.rb like assign_reviewer_dynamically, add_reviewer, automatic_review_mapping,peer_review_strategy etc.
- Some variables in review_mapping_controller.rb are not named properly. Rename variable names to convey what they are used for.
- Functions are lengthy and difficult to read. Replace switch statements with subclasses methods, if any.
- Create subclasses and models wherever necessary.
- Add meaningful comments and edit/remove unnecessary comments.
- Ensure that code changes do not break any functionality. Refactoring method names might cause cascaded updates in other files.
Design Pattern
During the code refactoring process, we conscientiously followed various design patterns to enhance the overall structure. One of the most common ones is the “Extract Method.” There were several instances where certain methods became overly lengthy and intricate. By extracting specific functionalities into separate methods, we significantly improved the code's readability and comprehension. This way, it became easier and clearer to understand what the method achieved. Furthermore, we tackled the issue of excessive conditional statements by employing the "Refactoring Conditionals" design pattern. This approach involves isolating the block of code within a conditional statement into a distinct method and subsequently invoking that method. This strategic refactoring not only streamlined our code but also contributed to a more readable and maintainable codebase.
File(s) Modified
1. review_mapping_controller.rb
2. review_mapping_controller_spec.rb
Changes can be seen in the pull request created here.
Solutions/Details of Changes Made
1. Changes to variable names to make them reflect what they actually do.
i) Changed variable name from mmappings to meta_review_mappings to provide more context.
ii) Used assignment_id which is declared before, instead of hardcoding params[:id].to_i
2. Added/Edited comments to improve the readability of code and make commenting proper.
a) Adding comments
Elaborate commenting has been performed for every method present in the review_mapping_comtroller. A few examples include:
i) Explains the functionality of add_calibration method in a simple manner.
ii) Inform that an assignment can have multiple topics which need to be taken care of.
iii) Explains the functionality of automatic_review_mapping
iv) Describes an error encountered in the stable version of Expertiza, and suggests potential fix.
v) Describe functionality of delete_reviewer, delete_metareviewer, delete_metareview, and several other functions.
b) Edit Comments
i) Describes the working of assign_reviewer_dynamically in a concise manner
ii) Explains the function review_allowed? better, the previous comment was too repetitive.
iii) Precisely mentions the criteria for requesting reviews by the user.
c) Deleting Comments
Comments about previous teams progress and other redundant comments were removed to improve readability and maintainability.
3. Reducing cyclomatic complexity of functions by dividing them into smaller methods.
Changes were made to the automatic_review_mapping method, keeping the single responsibilty principle in mind.
i) Modularized the functionality of previous version in a private method called handle_review_assignment which checks conditions to proceed with assigning reviewers to artifacts.
ii) Different components of the method are now encapsulated in their respective private functions
NOTE: Private functions are used to increase maintainability of the codebase since these do not break already existing test suites.
vi)
vii)
viii) Dividing long peer_review_strategy method in smaller modular functions
4. Reshuffling parts of code to make the method look consistent and easy to read and understand
The declarations for the automatic_review_mapping are grouped together for better readability.
Making changes in peer_review_strategy method to make the code look inline with other parts
5. Bug Fixes
Changes made to check_outstanding_reviews method to return the correct results. Previously if all the reviews were completed by a participant, the method returned true (num_reviews_in_progress = 0), which would not be the right output. This is fixed by ensuring a return value of true only if num_reviews_in_progress > 0.
6. Test Cases Addition
Test Cases have been added to improve test coverage. Additional details can be found in Test Plan section - https://wiki-expertiza.csc.ncsu.edu/index.php/CSC/ECE_517_Fall_2023_-_E2356._Refactor_review_mapping_controller.rb#Test_Plan
Test Plan
Test Plan, Testing Strategy and Results
In the realm of Test-Driven Development (TDD) research, our team made the strategic decision to utilize Mustafa Olmez's meticulously crafted test skeletons. These skeletons provided us with invaluable guidance, outlining the essential tests required for our unit. By incorporating 20 of these meticulously designed test skeletons into our work, we were able to conduct rigorous testing of our controller module. This approach allowed us to delve deeply into the functionality of our controller, ensuring thorough examination and validation of its behavior. The integration of these test skeletons not only provided a robust framework for our unit tests but also enhanced the overall quality and reliability of our codebase.
For a detailed view of the changes made, please refer to the complete file at the following link:
The additional tests ensure every method of review_mapping_controller is tested. By adopting this approach we have increased the overall coverage from 47.65% to 47.8%. The changes made passed all the necessary Travis CI/CD Pipeline tests.
SimpleCov Coverage Detail Report of review_mapping_controller.rb
Tests prior to the changes covered 69.65 % of review_mapping_controller.rb
After adding tests, the tests covered 69.76 % of refactored review_mapping_controller.rb
. It improved slightly to be an even better coverage.
Relevant Links
- Github Repository: https://github.com/amit-99/NCSU_OODD_expertiza
- Pull Request: https://github.com/expertiza/expertiza/pull/2657
- VCL Server: http://152.7.178.252:8080/
Team
Mentor
- Ameya Vaichalikar (agvaicha)
Team Members
- Chirag Bheemaiah Palanganda Karumbaiah (cpalang)
- Shanmukh Pawan Moparthi (smopart2)
- Amit Bhujbal (abhujba)