CSC/ECE 517 Fall 2024 - E2453. Refactor review mapping helper.rb
This wiki page is about E2453 for Expertiza.
Background
Expertiza is software that uses Ruby on Rails to manage a website for managing assignments, teams, submissions, and reviews. This web application is managed by students and faculty at NC State, offering instructors full control over class assignments. Expertiza supports a variety of functions, including topic creation, group assignments, and peer review, making it a robust tool for handling diverse types of assignments. For a complete overview of Expertiza's capabilities, visit the Expertiza wiki.
Problem Statement
The review_mapping_helper.rb file has methods that exceed the limit on lines of code. Also, it is missing proper comments for each functionality. The cyclomatic complexity and ABC complexity of most of the methods is way too high as per the standard defined in Code Climate. The scope of this project is to refactor the review_mapping_helper.rb to make it more readable and reduce complexity. This file is generally used to hold helper methods for review frontend pages.
Requirements
Complexity
There are several methods that exceed cyclomatic complexity and ABC complexity. This is the main priority for our refactoring.
Refactor for readability
- Several methods have confusing variable names and method names
- Some comments are unnecessary or are ambiguous.
- Some inconsistent formatting in the code.
Files Modified
Only three files were changed as this file is not called in many other places. These are:
- app/helpers/review_mapping_helpers.rb
- app/views/reports/_review_report.html.erb
- spec/helpers/review_mapping_helper_spec.rb
Changes Made
Cyclomatic Complexity
get_team_color
Old code:
New code:
This code exceeded cyclomatic complexity due to the nesting if statements. We can split the if statements into separate methods as they represent their own functionalities. This also reduces the ABC complexity as well.
Commit: [1]
check_submission_state
Old code:
New code:
This code exceeded cyclomatic complexity due to the nesting if statements. Along with this, the conditional is complicated enough to be its own method. We can split this into many methods to ensure the method is not doing too much.
Along with this, this method has 5 parameters, which is too much. We removed the colors parameter and decided to use colors outside the method so that we could push strings into it.
It also had an ABC complexity of 16.79 so the method split reduces this.
Commit: [2]
ABC Complexity
list_hyperlink_submission
Old code:
New code:
This code exceeded ABC complexity. Looking at this method, we can see that it is doing several functionalities at once. We can separate this into multiple methods for readability and reduced complexity.
Commit: [3]
sort_reviewer_by_review_volume_desc
Old code:
New code:
This method was the hardest to refactor as it was doing too much in one method. Initially, this had an ABC Complexity of 32.7, which is very high. We split the functionalities for getting volumes for each review and the average volume into its own private methods. This significantly reduced the ABC complexity of this method.
Commit: [4]
Renaming Variable Names and Methods
Several variables were ambiguously named. For example, rspan was supposed to represent the row number, but this is not obvious so we changed it to row_number.
Another example is using r to represent a reviewer. We change this to reviewer as it is more readable.
Along with this, we changed various method names as they can be ambiguous in terms of the intended function of these methods.
Commit: [5]
Consistent Formatting
There was a big portion of code that was indented inconsistently compared to the rest of the code. This was fixed.
Also, there was inconsitent spacing between methods, so we changed this spacing to be one line between each method.
Commit: [6]
Reformatting Test File
As we changed method names, we had to refactor the test file to match the new method names.
Along with this, get_team_color had a massive change in output, so we had to change the test to match the new version of the method.
Commit: [7]
Testing
All of the tests pass for helpers. This is shown in the image below. We did not significantly alter the testing from the previous iteration.
Future Scope
- spec/helpers/review_mapping_helper_spec.rb has repetitive code that can be combined into a method
- There are parts of review_mapping_helper with long JSON data that exceeds line count. This could be migrated to a JSON file instead of storing it in the code. One example of this is the data variable from method display_volume_metric_chart.
- In review_mapping_helper.rb, there is a method get_css_style_for_calibration_report that should be moved to a different file.
- review_mapping_helper.rb can always use more refactoring for variable and method names
Team Members
- Akhilesh Neeruganti (asneerug@ncsu.edu)
- Krishna Pallavalli (kpallav@ncsu.edu)
Mentor: Richard Li (rli14@ncsu.edu)
Relevant Links
Github Pull Request - https://github.com/expertiza/expertiza/pull/2886
Github Repo - https://github.com/neerua08/Expertiza_Refactor_ReviewMappingHelper
[8] contains detailed information on the previous team's work on this feature.