CSC/ECE 517 Fall 2023 - E2353. Further refactoring and improvement of review mapping helper

Problem Statement

Our project concentrates on making things readable and understandable in app/helpers/review_mapping_helper.rb file. We have have refactored some methods mentioned below for better understanding while maintaining the functionality. The aim is to also make it better understandable for other developers by adding comments wherever necessary.

Team

Mentor

Devashish Vachhani (dvachha@ncsu.edu)

Team Members

Harsh Mauny (hrmauny@ncsu.edu)
Connor Davidson (cdavids@ncsu.edu)
Aditya Joshi (ajoshi25@ncsu.edu)

Issues to be Addressed

• method get_data_for_review_report marshals a lot of data together and passes back a data structure. It is used in views/reports/_review_report.html.erb, but it is quite difficult to see how the data is being displayed. It violates the Expert pattern, because the view needs to know how to break apart the structure that is passed to it. Doing the same thing with partials in views/reports would make the code easier to follow

• The next three methods involve team “color”. Color-coding is explained on this page and this page for the E1789 project and this page for E1815. The code for these methods is not at all clear, and should be refactored. And please use the American spelling “color”.

• Various method names begin with get. This is not Ruby-like. Change the names to something more appropriate.
• get_awarded_review_score computes an overall score based upon scores awarded in individual rounds. This is one of many places in Expertiza where scores are being calculated. Score-calculation code for multiple rounds is being standardized now, in response_map.rb. Contact Nicholas Himes (nnhimes@ncsu.edu) for particulars. Change this method to use the new code.

• The method sort_reviewer_by_review_volume_desc should be generalized so that it can sort by any metric, not just review volume. Other metrics might include number of suggestions, or number of suggestions + number of problems detected. This method should not be counting the number of review rounds! Since other places in the code will need to know the number of review rounds, it should be calculated somewhere else in the system.

• The next several methods generate charts. They are cohesive enough that they should be in their own separate file, either another helper or a mixin.

• Then there is a method list_review_submissions. It’s not at all clear that this method is needed, though it is used in one view. Look on the Expertiza wiki and see if there is a better way.

• There are several methods for feedback_response_maps. It is not at all clear why they are here. Look on the Expertiza wiki for the documentation, and see if you can replace them by calls to other methods, or at least make it clearer what they are doing.

• The file ends with three small classes being defined. There are no comments at all to explain what is being done. Look them up on the Expertiza wiki and refactor or comment them, whichever seems more appropriate.

Plan of work

We started our work by analysing the review_mapping_helper.rb how it links to rest of the code and discussing the flow of the project in detail with our mentor. Based on our understanding of the code we divided our project requirements in terms of difficulty levels as shown in the document [1]. then we distributed the tasks among us and started refactoring various methods and creating new files as and when necessary. We also ensured the test cases passing earlier were intact and added new ones if necessary.

Files Modified

• app/helpers/charts_helper.rb
• app/helpers/review_mapping_helper.rb
• app/helpers/review_mapping_helper.rb
• app/views/reports/_calibration_report.html.erb
• app/views/reports/_feedback_report.html.erb
• app/views/reports/_review_report.html.erb
• app/views/reports/_team_score.html.erb
• app/views/reports/_team_score_score_awarded.html.erb
• spec/features/review_mapping_helper_spec.rb
• spec/helpers/review_mapping_helper_spec.rb

Project tracking

• Issue #1: make a separate file for for chart related elements in review_mapping_helper.rb
  

Methods involved: initialize_chart_elements(), display_volume_metric_chart(), display_tagging_interval_chart(), calculate_key_chart_information()

Previous Code: All these methods were previously a part of the review_mapping_helper.rb when it should have its separate helper file
Problem: These methods perform similar tasks of generating graphs and can have there separate module or a mixin and also each of the methods more than 30 lines inside them making them too large
Solution: distributed this code among 2 newly created files named review_mapping_charts_helper.rb and data_mapping_helper.rb within helper folder. This way the chart helper methods are separated from review_mapping_helper where they are out of place and difficult for any developer to find. review_mapping_charts_helper.rb contains the original methods and data_mapping_helper.rb manages the data need by these chart methods

review_mapping_charts_helper.rb

module  ReviewMappingChartsHelper
...
    def initialize_chart_elements()
    def display_volume_metric_chart()
    def display_tagging_interval_chart()
    def calculate_key_chart_information()

all link related to these changes:
review_mapping_charts_helper.rb
review_mapping_helper.rb
data_mapping_helper.rb
Contributor: Harsh Mauny

• Issue #2: change methods names starting with "get" to something appropriate
  

Methods involved: get_data_for_review_report(), get_team_color(), get_link_updated_at(), get_team_reviewed_link_name(), get_awarded_review_score(), get_each_review_and_feedback_response_map(), get_certain_review_and_feedback_response_map(), get_css_style_for_calibration_report()
Previous Code: previous code used "get" in method names
Problem: All these methods used "get_" which is not ruby-like.
Solution: change method names to a more ruby-like name that was appropriate for the given context

Contributor: Connor Davidson

• Issue #7: Several methods for feedback_response_maps, check docs to manage this confusion
  

Methods involved: feedback_response_for_author()
Previous Code: previous code had an ambiguous variable name
Problem: It wasn't clear what the variable was used for.
Solution: refactor the variable name to be more easily understood and add comments to give a clear explanation for why it exists

Contributor: Connor Davidson