CSC/ECE 517 Spring 2022 - E2239: Further refactoring and improvement of review mapping helper.rb

Background

Within Expertiza, one of the functionalities is for one participant to review the submission of another participant (or team) by answering specific questions about the submission and leaving comments and feedback. This involves the interaction of many tables: users, assignments, teams, reviews, and review_mappings. The review_mapping_controller is the primary file which handles the code for reviews, but it also relies on review_mapping_helper for additional methods. While review_mapping_controller focuses mostly on CRUD operations for reviews, reviewers, and scores, review_mapping_helper focuses more on which data are presented and how (colors, sorting, charts, etc). This project focuses on refactoring review_mapping_helper and seeks to make the current code more understandable and transparent, such as by using partials, renaming methods with more standard conventions, adding additional comments, removing/modifying unnecessary methods, and making other methods more robust and useful. This project builds on the work done by E2125, which focused on simplifying some methods in review_mapping_helper.

History: Previous project for refactoring review_mapping_helper.rb

* E2225 [1]
* E2162 [2]
* E1913 [3]

Previous Methods/Classes and their Usages

This Chart shows each method in the review_mapping_helper.rb file before our changes will be implemented. Since this project is a refactor, the overall usage of the file will not change, but some methods may be combined, added, destroyed, or moved to other files. The helper file also contains inner classes that work with the review_mapping_controller.rb.

Problems and planned changes

Problem 1: The method get_data_for_review_report violates the Expert pattern

get_data_for_review_report passes back a data structure with multiple response objects to the _review_report.html.erb view. This violates the Expert pattern, because the given view does not know how to break apart the structure passed to it.

• Proposed Solution : get_data_for_review_report will be refactored to use partials

• Solution Implemented : The columns of the view that were displaying the score and average score were already using partials. This logic was adopted to change the "Team Reviewed" section to use a partial as well. This included moving the code from this section of _review_report.html.erb into a new file in order to render the partial: _team_reviewed.html.erb. This new file was commented to note what it is doing, and the calling code was modified to pass necessary local variables. The code was also cleaned up to look nicer and align with how other calls in the same block of code were being done.

     <% @response_maps.each_with_index do |reviewer_map, index| %>
       <% if Team.where(id: reviewer_map.reviewee_id).length > 0 %>
         <% @team = Team.find(reviewer_map.reviewee_id) %>
         <%= render partial: 'team_reviewed', locals: {bgcolor: @bgcolor, team_id: @team.id, reviewer_id: reviewer.id, reviewer_map: reviewer_map, index: index, reviewer: reviewer} %>
       <% end %>
     <% end %>

• Changes Made : File Changes: here, here

Problem 2: Several method names begin with get

This naming convention is not Ruby-like, and needs to be changed.

• Proposed Solution : All method names beginning with get will be refactored to use more appropriate names. e.g. get_team_color will be refactored into team_color?. Other method names beginning with verbs will also be reviewed
• Solution Implemented : All method names beginning with get have been refactored to remove the get, and now follow standard naming conventions. e.g. get_team_color to team_color. Respective calls to these methods in various views and tests have also been refactored to use the updated names
• Changes Made : File Changes: here

Problem 3: The method get_awarded_review_score needs to be updated

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 .

• Proposed Solution : Change the method to utilize the standard code from response_map.rb , and rename it to awarded_review_score?
• Solution Implemented : After investigation, it was found that get_awarded_review_score does not actually perform any score computation, and shares very similar functionality with review_metrics. Both methods set instance variables that are retrieved by team_score_score_awarded.html.erb and team_score.html.erb respectively. The scoring.rb model and the models that include it were investigated to find code that could possibly substitute for get_awarded_review_score, but it was found that no comparable method exists. Ultimately, it was decided to keep get_awarded_review_score as-is, but rename it to awarded_review_metrics?, both to standardize its naming with review_metrics and to better reflect the fact this method is not involved in score calculation.
• Changes Made : link

Problem 4: The method sort_reviewer_by_review_volume_desc needs to be generalized

sort_reviewer_by_review_volume_desc only sorts by review volume, which is very specific. This method should not be counting the number of review rounds, as that is calculated elsewhere in the system.

• Proposed Solution : Modify the method to be able to sort by multiple different metrics such as a number of suggestions, or a number of suggestions + number of problems detected. This will also justify a method name change (and possible parameter change/additions), which will have to be updated elsewhere in the app.

Problem 5: Several methods are used for generating charts and should be moved to another file

Including initialize_chart_elements , display_volume_metric_chart , display_tagging_interval_chart , calculate_key_chart_information .They are cohesive enough to just be moved to another file.

• Proposed Solution : These methods will be moved to a new helper class, review_mapping_chart_helper.rb. This should separate their functionality from review_mapping_helper, and better illustrate their independent usage by review_report.html.erb and answer_tagging.html.erb

• Solution Implemented : Done 4/21/2022 These methods were moved to review_mapping_chart_helper.rb, all related tests were also moved, comments were added to the view files to reflect changes.

• Changes Made : File Changes: here, here, here

Problem 6: Investigate the use of list_review_submissions

It is not all clear that this method is needed, though it is used in one view (_review_report.html.erb). Need clarification if this function is being used or not.

• Proposed Solution : Investigate the validity of the method. If useless, remove it; else, document it and refactor if necessary.

• Solution Implemented : Investigation yields the purpose of this method - it adds a link to the associated files that a reviewer uploaded in association with a review (as seen in below image). As such, this is a useful function, so it remains in place. Comments were added to the method header to explain what it is doing. Additionally, the file name was changed to list_review_uploaded_files to better reflect its functionality.

• Changes Made : File Changes: here

Problem 7: The method feedback_response_maps violates the Expert pattern

It is not clear why feedback_response_maps are here.

• Proposed Solution :

Look up the expertiza wiki for the documentation, and make it clearer what they are doing

 def feedback_response_map_record(author)
     instance_variable_set('@feedback_response_maps_round_' + round_num,
                           FeedbackResponseMap.where(['reviewed_object_id IN (?) and reviewer_id = ?',
                                                      instance_variable_get('@all_review_response_ids_round_' + round_num), author.id]))
 end

• Solution Implemented:The method feedback_response_map_record is for the assignment has multiple rounds. However, the same function is already in views/reports/feedback_report and feedback_response_map. This method is better to be removed from this helper method for DRY. Rspec test also applied this update. The function.

• Changes Made: here

Problem 8: three small classes violates the Expert pattern

It is not clear why ReviewStrategy, StudentReviewStrategy, TeamReviewStrategy are here.

• Proposed Solution :

Class ReviewStrategy is called in reveiw_mapping_controller.rb => Better to move this Class to reveiw_mapping_model.rb

• Class ReviewStrategy example in reveiw_mapping_controller.rb

   def automatic_review_mapping_strategy()
   ...
      if !student_review_num.zero? && submission_review_num.zero?
        review_strategy = ReviewMappingHelper::StudentReviewStrategy.new(participants, teams, student_review_num)
      elsif student_review_num.zero? && !submission_review_num.zero?
        review_strategy = ReviewMappingHelper::TeamReviewStrategy.new(participants, teams, submission_review_num)
      end
   ...
   end

• Solution Implemented: To remove expert pattern violation, a model is newly added: review_strategy.rb and it includes student_review_strategy and team_review_strategy classes.

The associated method automatic_reivew_mapping_startegy rspec test pass with this change.

• Changes Made:here

Problem 9: The method get_team_color begins with get_ and another method called obtain_team_color exists

get_team_color violates naming conventions, obtain_team_color also has a similar name and is only used by get_team_color. obtain_team_color can be considered a private method, so testing could be removed if its callee is alredy being tested.

• Proposed Solution : get_team_color will be renamed to team_color?, the contents of obtain_team_color will be moved to team_color?.

• Solution Implemented : Done 4/22/2022. The get_team_color method was renamed to team_color?, tests and view files have been updated with the proper naming. obtain_team_color's code as been moved into team_color?. The tests for it have also been deleted since the code was already testing the called method contents before refactoring.

• Changes Made : File Changes: here, here

UML Diagram

A picture of the current and proposed UML diagram is included below

The updated schema makes these changes:

• obtain_team_color has been refactored into team_color?, as the the former was called by the latter and was not unique enough to exist as a separate method
• initialze_chart_elements,display_volume_metric_chart,display_tagging_interval_chart, and calculate_key_chart_information have been refactored into a separate helper class review_mapping_chart_helper.rb, as their functionality is self-contained enough to exist on their own
• ReviewStrategy,StudentReviewStrategy, and TeamReviewStrategy have ben refactored as inner classes of review_mapping_model as this is the only class that interacts with these inner-classes.
• team_reviewed.html.erb has been added to render partials used by _review_report.html.erb
• All method names beginning with get have been refactored to use ? instead of get in order to conform to ruby standards

Test Plan

• RSpec Unit Tests: Refactor Aim for " Always have working Code"

1. Updated Rspec files according to refactoring project:

 * /rspec/review_mapping_helper_spec.rb 
 * /rspec/review_mapping_chart_helper_spec.rb 

2. Rspec Tested files: All rspec tests are passed

 * /rspec/review_mapping_helper_spec.rb 
 * /rspec/review_mapping_controller.rb 
 * /rspec/review_mapping_chart_helper_spec.rb 

• Functionality Tests:

Our refactoring didn't change functionality. Functionality tests were not needed and included.

GitHub links and Pull Request

• GitHub link here
• GitHub Pull Request here

Team Members

Evan Brown

Chaitanya Patel

Rachel Hyo Son

Andrew Pippin