CSC/ECE 517 Fall 2021 - E2162. Further refactoring and improvement of review mapping helper
Introduction
This page gives a description of the changes made for the review_mapping_helper.rb of Expertiza based OSS project.
Expertiza Background
Expertiza is a web application where students can submit and peer-review learning objects (articles, codes, websites, etc). Instructors add and grade the assignments submitted by students to Expertiza. Students can be assigned in teams based on their selection of the topics. It has functionalities such as peer reviews in which students can provide feedback on other's work which helps peer in better developing the project. It is supported by the National Science Foundation.
Problem Statement
What is needed: This project builds on E2125, and should begin with the refactoring done by that project. That project focused on simplifying the methods in review_mapping_helper, while this project looks at making the code more understandable and transparent.
- 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.
- The 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.
- 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.
- Also 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.
Implementation
Flowchart
The following process is carried out to complete the project-
Refactoring
Refactoring method names
All the method names that are refactored are mentioned below-
Before:
get_each_round_review_and_feedback_response_map_for_feedback_report
After:
get_each_review_and_feedback_response_map [Since the method and file is already about feedback report, we do not need to put feedback_report in the method name]
Before:
get_certain_round_review_and_feedback_response_map_for_feedback_report
After:
get_certain_review_and_feedback_response_map [Since the method and file is already about feedback report, we do not need to put feedback_report in the method name]
Before:
get_team_name_color_in_review_report
After:
get_team_colour [The method suggests that team color is defined in the review report, so we do not need to put in_review_report in the method name]
Before:
get_each_round_score_awarded_for_review_report
After:
get_awarded_review_score [The method gets the awarded score for review, so we do not need to put for_review_report in the method name]
Before:
get_min_max_avg_value_for_review_report
After:
get_review_metrics [Since minimum, maximum and average value is the part of metrics in review report, we can replace it all by metrics]
Refactoring method
The following method was not used or called in any of the files in the project. Due to that reason, the entire function was commented out.
def get_current_round(reviewer_id)
user_id = Participant.find(reviewer_id).user.id
topic_id = SignedUpTeam.topic_id(@assignment.id, user_id)
@assignment.number_of_current_round(topic_id)
@assignment.num_review_rounds if @assignment.get_current_stage(topic_id) == "Finished" || @assignment.get_current_stage(topic_id) == "metareview"
end
Comments added to various functions
There were several methods which didn't have any comments or the comments weren't meaningful. In all those cases, comments have been added or changed which are mentioned as follows:
# gets the review score awarded based on each round of the review def get_each_round_score_awarded_for_review_report(reviewer_id, team_id)
# gets minimum, maximum and average value for all the reviews def get_min_max_avg_value_for_review_report(round, team_id)
# sorts the reviewers by the average volume of reviews in each round, in descending order def sort_reviewer_by_review_volume_desc
# displays all the average scores in round 1, 2 and 3 def display_volume_metric(overall_avg_vol, avg_vol_in_round_1, avg_vol_in_round_2, avg_vol_in_round_3)
# moves data of reviews in each round from a current round def initialize_chart_elements(reviewer)
# The data of all the reviews is displayed in the form of a bar chart def display_volume_metric_chart(reviewer)
# For assignments with 1 team member, the following method returns user's fullname else it returns "team name" that a particular reviewee belongs to. def get_team_reviewed_link_name(max_team_size, response, reviewee_id)
# if the current stage is "submission" or "review", function returns the current round number otherwise # if the current stage is "Finished" or "metareview", function returns the number of rounds of review completed def get_current_round_for_review_report(reviewer_id)
# gets the response map data such as reviewer id, reviewd object id and type for the review report def get_data_for_review_report(reviewed_object_id, reviewer_id, type)
# gets the team name's color according to review and assignment submission status def get_team_name_color_in_review_report(response_map)
# checks if a review was submitted in every round and gives the total responses count def response_for_each_round?(response_map)
# returns hyperlink of the assignment that has been submitted on the due date def submitted_hyperlink(round, response_map, assignment_created, assignment_due_dates)
Function placed in the view as partials
The following method has been removed from the review_mapping_helper.rb and added to the views as partials:
def create_report_table_header(headers = {})
table_header = "<div class = 'reviewreport'>\
<table width='100% cellspacing='0' cellpadding='2' border='0' class='table table-striped'>\
<tr bgcolor='#CCCCCC'>"
headers.each do |header, percentage|
table_header += if percentage
"<th width = #{percentage}>\
#{header.humanize}\
</th>"
else
"<th>\
#{header.humanize}\
</th>"
end
end
table_header += "</tr>"
table_header.html_safe
end
The following is the code available in views in the file _report_table_header.html.erb:
<% table_header = "<div class = 'reviewreport'><table width='100% cellspacing='0' cellpadding='2' border='0' class='table table-striped'><tr bgcolor='#CCCCCC'>" %>
<% headers.each do |header, percentage| %>
<% table_header += if percentage %>
<% "<th width = #{percentage}> #{header.humanize} </th>" %>
<% else %>
<%"<th> #{header.humanize} </th>" %>
<% end %>
<% end %>
<% table_header += "</tr>"%>
<% return table_header.html_safe %>
Testing
RSpec Testing
RSpec testing was carried out for the methods that had been refactored. Given below are the tests which are written for the functions that are modified:
describe 'My Test Cases' do
before(:each) do
create(:instructor)
create(:role_of_student)
login_as("instructor6")
visit '/tree_display/list'
click_link 'View reports'
expect(page).to have_current_path('/reports/response_report')
click_link 'View'
end
it "can display review metrics", js: true do
expect(page).to have_content('Metrics')
end
it "can display review grades of each round", js: true do
expect(page).to have_content('Score awarded')
end
it "can display review summary", js: true do
expect(page).to have_content('Reviews done')
end
it "can display review summary", js: true do
expect(page).to have_content('Reviewer')
end
it "can display review summary", js: true do
expect(page).to have_content('Team reviewed')
end
end
Design Pattern
A design pattern is a general repeatable solution to a commonly occurring problem in software design. It is a description or template for how to solve a problem that can be used in many different situations.
During the process of refactoring methods as well as method names,Strategy Pattern was used in the implementation. The Strategy pattern is most useful when you want to provide multiple ways of processing a request, without hard-coding knowledge about those different methods into the object that handles the request.
References
Team Members
Ashish Kumar Jayantilal Jain
Aishwarya Tirumala
Devang Upadhyay