CSC/ECE 517 Spring 2022 - E2239: Further refactoring and improvement of review mapping helper.rb: Difference between revisions
Line 92: | Line 92: | ||
Rspec file <code> /rspec/review_mapping_helper_spec.rb</code> will be updated depending on the following cases. | Rspec file <code> /rspec/review_mapping_helper_spec.rb</code> will be updated depending on the following cases. | ||
1. | 1. Case#1 : Methods not included in Rspec => review the method to refactor & include in Rspec file. | ||
- <code>get_data_for_review_report</code> | - <code>get_data_for_review_report</code> |
Revision as of 02:01, 12 April 2022
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 moreso 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
.
Problems and planned changes
Histories: Previous project for refactoring review_mapping_helper.rb
* E2225 [1] * E2162 [2] * E1913 [3]
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
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 intoteam_color?
. Other method names beginning with verbs will also be reviewed
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
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.
- Proposed Solution : Modify the method to be able to sort by multiple different metrics such as number of suggestions, or number of suggestions + number of problems detected. This will also jusity 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 fromreview_mapping_helper
, and better illustrate their independent usage byreview_report.html.erb
andanswer_tagging.html.erb
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. Needs clarification if this function is being used or not.
- Proposed Solution : See if there is a way to get rid of it or incorporate it into the main method.
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
FeedbackResponseMap found in two places in review_mapping_helper.rb
=> will add comments to make this code understandable.
def get_certain_review_and_feedback_response_map(author) @feedback_response_maps = FeedbackResponseMap.where(['reviewed_object_id IN (?) and reviewer_id = ?', @all_review_response_ids, author.id]) end
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
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 inreveiw_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
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 intoget_team_color
, as the the former is called by the latter and is not unique enough to exist as a separate methodinitialze_chart_elements
,display_volume_metric_chart
,display_tagging_interval_chart
, andcalculate_key_chart_information
have been refactored into a separate helper classreview_mapping_chart_helper.rb
, as their functionality is self-contained enough to exist on their ownReviewStrategy
,StudentReviewStrategy
, andTeamReviewStrategy
have ben refactored as inner classes ofreview_mapping_model
as this is the only class that interacts with these inner-classes.- All method names beginning with
get
have been refactored to use?
instead ofget
in order to conform to ruby standards
Test Plan
- RSpec Unit Tests:
Rspec file /rspec/review_mapping_helper_spec.rb
will be updated depending on the following cases.
1. Case#1 : Methods not included in Rspec => review the method to refactor & include in Rspec file.
-get_data_for_review_report
-initialize_chart_elements
-display_volume_metric_chart
-display_tagging_interval_chart
-list_hyperlink_submission
-get_each_review_and_feedback_response_map
-get_certain_review_and_feedback_response_map
-Class ReviewStrategy
-Class StudentReviewStrategy
-Class TeamReviewStrategy
2. Case #2: Refactoring may cause to break existing Rspec. => refactor Rspec as well
3. Case #3: Adding a new method => Add new Rspec scenario
4. Case #4: Removing methods => Remove from Rspec
- Functionality Tests:
Functionality test will be added when our refactor causes to change in the logic. Our focus on this project is refactoring review_mapping_helper.rb
to be more readable and understandable.
GitHub links and Pull Request
GitHub link here
Team Members
Evan Brown
Chaitanya Patel
Rachel Hyo Son
Andrew Pippin