CSC/ECE 517 Fall 2024 - E2453. Refactor review mapping helper.rb

From Expertiza_Wiki
Jump to navigation Jump to search

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

  1. spec/helpers/review_mapping_helper_spec.rb has repetitive code that can be combined into a method
  2. 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.
  3. In review_mapping_helper.rb, there is a method get_css_style_for_calibration_report that should be moved to a different file.
  4. review_mapping_helper.rb can always use more refactoring for variable and method names

Team Members

  1. Akhilesh Neeruganti (asneerug@ncsu.edu)
  2. 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.