CSC/ECE 517 Fall 2023 - E2356. Refactor review mapping controller.rb

From Expertiza_Wiki
Jump to navigation Jump to search

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:

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 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

Team

Mentor

  • Ameya Vaichalikar (agvaicha)

Team Members

  • Chirag Bheemaiah Palanganda Karumbaiah (cpalang)
  • Shanmukh Pawan Moparthi (smopart2)
  • Amit Bhujbal (abhujba)
Retrieved from "https://wiki.expertiza.ncsu.edu/index.php?title=CSC/ECE_517_Fall_2023_-_E2356._Refactor_review_mapping_controller.rb&oldid=151842"