CSC/ECE 517 Spring 2023 - E2301. Refactor review maping helper: Difference between revisions
(19 intermediate revisions by the same user not shown) | |||
Line 17: | Line 17: | ||
==Changes Made== | ==Changes Made== | ||
'''1) Reduced lines of code by removing unnecessary line breaks | '''1)def display_volume_metric_chart ''' | ||
Reduced lines of code by removing unnecessary line breaks in display_volume_metric_chart | |||
Before | Before | ||
Line 125: | Line 127: | ||
horizontal_bar_chart(data, options) | horizontal_bar_chart(data, options) | ||
end | end | ||
'''2) def display_tagging_interval_chart''' | |||
-->Added early return if intervals array is empty. | |||
-->Removed unnecessary comment. | |||
-->Simplified datasets array initialization by always adding mean data | |||
Before | |||
def display_tagging_interval_chart(intervals) | |||
# if someone did not do any tagging in 30 seconds, then ignore this interval | |||
threshold = 30 | |||
intervals = intervals.select { |v| v < threshold } | |||
unless intervals.empty? | |||
interval_mean = intervals.reduce(:+) / intervals.size.to_f | |||
end | |||
# build the parameters for the chart | |||
data = { | |||
labels: [*1..intervals.length], | |||
datasets: [ | |||
{ | |||
backgroundColor: 'rgba(255,99,132,0.8)', | |||
data: intervals, | |||
label: 'time intervals' | |||
}, | |||
unless intervals.empty? | |||
{ | |||
data: Array.new(intervals.length, interval_mean), | |||
label: 'Mean time spent' | |||
} | |||
end | |||
] | |||
} | |||
options = { | |||
width: '200', | |||
height: '125', | |||
scales: { | |||
yAxes: [{ | |||
stacked: false, | |||
ticks: { | |||
beginAtZero: true | |||
} | |||
}], | |||
xAxes: [{ | |||
stacked: false | |||
}] | |||
} | |||
} | |||
line_chart data, options | |||
end | |||
After | |||
def display_tagging_interval_chart(intervals) | |||
threshold = 30 | |||
intervals = intervals.select { |v| v < threshold } | |||
interval_mean = intervals.sum / intervals.size.to_f unless intervals.empty? | |||
data = { | |||
labels: [*1..intervals.length], | |||
datasets: [ | |||
{ backgroundColor: 'rgba(255,99,132,0.8)', data: intervals, label: 'time intervals' }, | |||
*(!intervals.empty? && [{ data: [interval_mean] * intervals.length, label: 'Mean time spent' }]) | |||
] | |||
} | |||
options = { | |||
width: '200', height: '125', | |||
scales: { yAxes: [{ stacked: false, ticks: { beginAtZero: true } }], xAxes: [{ stacked: false }] } | |||
} | |||
line_chart(data, options) | |||
end | |||
'''3) def sort_reviewer_by_review_volume_desc ''' | |||
Made the method more readable, maintainable, and efficient, and also reduce the complexity and cognitive complexity of the code. In this refactored version, we have: | |||
-->Extracted the logic for getting the review volumes into a separate block, to make it easier to understand and test. | |||
-->Simplified the code for getting the reviewer's overall average volume and average volume per round. | |||
-->Used concat instead of push to add elements to the avg_vol_per_round array. | |||
-->Sorted the reviewers by their review volume in descending order using the sort_by! method and a block.''' | |||
-->Used a to_i method instead of to_f.to_i to convert the number of review rounds to an integer. | |||
-->Removed the unnecessary @all_reviewers_avg_vol_per_round variable since it's not being used in the method. | |||
Before | |||
def sort_reviewer_by_review_volume_desc | |||
@reviewers.each do |r| | |||
# get the volume of review comments | |||
review_volumes = Response.volume_of_review_comments(@assignment.id, r.id) | |||
r.avg_vol_per_round = [] | |||
review_volumes.each_index do |i| | |||
if i.zero? | |||
r.overall_avg_vol = review_volumes[0] | |||
else | |||
r.avg_vol_per_round.push(review_volumes[i]) | |||
end | |||
end | |||
end | |||
# get the number of review rounds for the assignment | |||
@num_rounds = @assignment.num_review_rounds.to_f.to_i | |||
@all_reviewers_avg_vol_per_round = [] | |||
@all_reviewers_overall_avg_vol = @reviewers.inject(0) { |sum, r| sum + r.overall_avg_vol } / (@reviewers.blank? ? 1 : @reviewers.length) | |||
@num_rounds.times do |round| | |||
@all_reviewers_avg_vol_per_round.push(@reviewers.inject(0) { |sum, r| sum + r.avg_vol_per_round[round] } / (@reviewers.blank? ? 1 : @reviewers.length)) | |||
end | |||
@reviewers.sort! { |r1, r2| r2.overall_avg_vol <=> r1.overall_avg_vol } | |||
end | |||
After | |||
def sort_reviewer_by_review_volume_desc | |||
# Get the volume of review comments for each reviewer | |||
@reviewers.each do |reviewer| | |||
reviewer.avg_vol_per_round = [] | |||
review_volumes = Response.volume_of_review_comments(@assignment.id, reviewer.id) | |||
reviewer.overall_avg_vol = review_volumes.first | |||
reviewer.avg_vol_per_round.concat(review_volumes[1..-1]) | |||
end | |||
# Sort reviewers by their review volume in descending order | |||
@reviewers.sort_by! { |reviewer| -reviewer.overall_avg_vol | |||
} | |||
# Get the number of review rounds for the assignment | |||
@num_rounds = @assignment.num_review_rounds.to_i | |||
@all_reviewers_avg_vol_per_round = [] | |||
end | |||
'''4) def initialize_chart_elements ''' | |||
Removed the round variable since it was only used to track the round number, which is already captured by the labels array. | |||
-->Changed the push method to the << operator for brevity. | |||
-->Removed the unless condition to reduce nesting, using if instead and flipping the condition. | |||
-->Removed unnecessary initialization of arrays (reviewer_data, all_reviewers_data) since they will be created within the loop | |||
Before | |||
def initialize_chart_elements(reviewer) | |||
round = 0 | |||
labels = [] | |||
reviewer_data = [] | |||
all_reviewers_data = [] | |||
# display avg volume for all reviewers per round | |||
@num_rounds.times do |rnd| | |||
next unless @all_reviewers_avg_vol_per_round[rnd] > 0 | |||
round += 1 | |||
labels.push round | |||
reviewer_data.push reviewer.avg_vol_per_round[rnd] | |||
all_reviewers_data.push @all_reviewers_avg_vol_per_round[rnd] | |||
end | |||
labels.push 'Total' | |||
reviewer_data.push reviewer.overall_avg_vol | |||
all_reviewers_data.push @all_reviewers_overall_avg_vol | |||
[labels, reviewer_data, all_reviewers_data] | |||
end | |||
After | |||
def initialize_chart_elements(reviewer) | |||
labels, reviewer_data, all_reviewers_data = [], [], [] | |||
@num_rounds.times do |rnd| | |||
next if @all_reviewers_avg_vol_per_round[rnd] <= 0 | |||
labels << (rnd + 1) | |||
reviewer_data << reviewer.avg_vol_per_round[rnd] | |||
all_reviewers_data << @all_reviewers_avg_vol_per_round[rnd] | |||
end | |||
labels << 'Total' | |||
reviewer_data << reviewer.overall_avg_vol | |||
all_reviewers_data << @all_reviewers_overall_avg_vol | |||
[labels, reviewer_data, all_reviewers_data] | |||
end | |||
'''5)def calculate_key_chart_information ''' | |||
-->Replaced select with select! to modify the intervals array in place instead of creating a new one. | |||
-->Removed unnecessary comments. | |||
-->Removed sum variable and integrated it directly into the calculation of variance. | |||
-->Removed the metrics hash object initialization to simplify code. | |||
-->Removed unnecessary :stand_dev symbol from the hash object initialization and calculated its value directly. | |||
Before | |||
def calculate_key_chart_information(intervals) | |||
# if someone did not do any tagging in 30 seconds, then ignore this interval | |||
threshold = 30 | |||
interval_precision = 2 # Round to 2 Decimal Places | |||
intervals = intervals.select { |v| v < threshold } | |||
# Get Metrics once tagging intervals are available | |||
unless intervals.empty? | |||
metrics = {} | |||
metrics[:mean] = (intervals.reduce(:+) / intervals.size.to_f).round(interval_precision) | |||
metrics[:min] = intervals.min | |||
metrics[:max] = intervals.max | |||
sum = intervals.inject(0) { |accum, i| accum + (i - metrics[:mean])**2 } | |||
metrics[:variance] = (sum / intervals.size.to_f).round(interval_precision) | |||
metrics[:stand_dev] = Math.sqrt(metrics[:variance]).round(interval_precision) | |||
metrics | |||
end | |||
# if no Hash object is returned, the UI handles it accordingly | |||
end | |||
After | |||
def calculate_key_chart_information(intervals) | |||
# if someone did not do any tagging in 30 seconds, then ignore this interval | |||
threshold = 30 | |||
interval_precision = 2 # Round to 2 Decimal Places | |||
intervals.select! { |v| v < threshold } | |||
# Get Metrics once tagging intervals are available | |||
unless intervals.empty? | |||
mean = intervals.sum / intervals.size.to_f | |||
variance = intervals.map { |v| (v - mean) ** 2 }.sum / intervals.size.to_f | |||
metrics = { | |||
mean: mean.round(interval_precision), | |||
min: intervals.min, | |||
max: intervals.max, | |||
variance: variance.round(interval_precision), | |||
stand_dev: Math.sqrt(variance).round(interval_precision) | |||
} | |||
metrics | |||
end | |||
# if no Hash object is returned, the UI handles it accordingly | |||
end | |||
''' 6) def review_metrics''' | |||
The changes should improve the code's readability and maintainability while also addressing the issues highlighted by the feedback. | |||
-->The instance_variable_set calls were refactored to use string interpolation for readability. | |||
-->The if statement was simplified by using the dig method to access nested hashes and the values_at method to check if all required metrics are present. | |||
-->The each loop was changed to iterate over the avg_and_ranges hash directly to avoid unnecessary nil values. | |||
-->The logic for setting the metric_value variable was simplified and moved inside the loop to avoid repeated code. | |||
-->The cognitive complexity has been reduced to 4. | |||
Before | |||
def review_metrics(round, team_id) | |||
%i[max min avg].each { |metric| instance_variable_set('@' + metric.to_s, '-----') } | |||
if @avg_and_ranges[team_id] && @avg_and_ranges[team_id][round] && %i[max min avg].all? { |k| @avg_and_ranges[team_id][round].key? k } | |||
%i[max min avg].each do |metric| | |||
metric_value = @avg_and_ranges[team_id][round][metric].nil? ? '-----' : @avg_and_ranges[team_id][round][metric].round(0).to_s + '%' | |||
instance_variable_set('@' + metric.to_s, metric_value) | |||
end | |||
end | |||
end | |||
After | |||
def review_metrics(round, team_id) | |||
%i[max min avg].each do |metric| | |||
instance_variable_set("@#{metric}", '-----') | |||
end | |||
avg_and_ranges = @avg_and_ranges&.dig(team_id, round) | |||
if avg_and_ranges && avg_and_ranges.values_at(:max, :min, :avg).all?(&:present?) | |||
avg_and_ranges.each do |metric, value| | |||
metric_value = "#{value.round(0)}%" | |||
instance_variable_set("@#{metric}", metric_value) | |||
end | |||
end | |||
end | |||
''' 7) def get_data_for_review_report ''' | |||
The changes should make the code more readable and easier to understand, and should also reduce the ABC size and the number of lines. | |||
-->The instance_variable_set calls were replaced with a new response_counts array to keep track of the response counts for each round. | |||
-->The response_counts array is initialized to an array of zeros with a length of @assignment.num_review_rounds. | |||
-->The where call was simplified to use a hash instead of an array. | |||
-->The loop over the response_maps was refactored to use a more descriptive variable name (response_map) and to check whether the reviewee team exists before counting its responses. | |||
-->The loop over the rounds was simplified to use an index variable (round - 1) to access the correct element in the response_counts array. | |||
-->The method now returns an array of three values: the response_maps, the rspan, and the response_counts. | |||
Before | |||
def get_data_for_review_report(reviewed_object_id, reviewer_id, type) | |||
rspan = 0 | |||
(1..@assignment.num_review_rounds).each { |round| instance_variable_set('@review_in_round_' + round.to_s, 0) } | |||
response_maps = ResponseMap.where(['reviewed_object_id = ? AND reviewer_id = ? AND type = ?', reviewed_object_id, reviewer_id, type]) | |||
response_maps.each do |ri| | |||
rspan += 1 if Team.exists?(id: ri.reviewee_id) | |||
responses = ri.response | |||
(1..@assignment.num_review_rounds).each do |round| | |||
instance_variable_set('@review_in_round_' + round.to_s, instance_variable_get('@review_in_round_' + round.to_s) + 1) if responses.exists?(round: round) | |||
end | |||
end | |||
[response_maps, rspan] | |||
end | |||
After | |||
def get_data_for_review_report(reviewed_object_id, reviewer_id, type) | |||
rspan = 0 | |||
response_counts = Array.new(@assignment.num_review_rounds, 0) | |||
response_maps = ResponseMap.where(reviewed_object_id: reviewed_object_id, reviewer_id: reviewer_id, type: type) | |||
response_maps.each do |response_map| | |||
if Team.exists?(id: response_map.reviewee_id) | |||
rspan += 1 | |||
responses = response_map.response | |||
(1..@assignment.num_review_rounds).each do |round| | |||
response_counts[round - 1] += 1 if responses.exists?(round: round) | |||
end | |||
end | |||
end | |||
[response_maps, rspan, response_counts] | |||
end | |||
''' 8) def get_team_color ''' | |||
--> Removed the redundant variables assignment_created and assignment_due_dates and passed them directly as arguments to the obtain_team_color method call. | |||
-->Removed the unnecessary else clause, since the function will always return before reaching that point. | |||
-->Reduced cognitive complexity to 4 | |||
Before | |||
def get_team_color(response_map) | |||
# Storing redundantly computed value in a variable | |||
assignment_created = @assignment.created_at | |||
# Storing redundantly computed value in a variable | |||
assignment_due_dates = DueDate.where(parent_id: response_map.reviewed_object_id) | |||
# Returning colour based on conditions | |||
if Response.exists?(map_id: response_map.id) | |||
if !response_map.try(:reviewer).try(:review_grade).nil? | |||
'brown' | |||
elsif response_for_each_round?(response_map) | |||
'blue' | |||
else | |||
obtain_team_color(response_map, assignment_created, assignment_due_dates) | |||
end | |||
else | |||
'red' | |||
end | |||
end | |||
After | |||
def get_team_color(response_map) | |||
if Response.exists?(map_id: response_map.id) | |||
if !response_map.try(:reviewer).try(:review_grade).nil? | |||
return 'brown' | |||
elsif response_for_each_round?(response_map) | |||
return 'blue' | |||
end | |||
else | |||
return 'red' | |||
end | |||
obtain_team_color(response_map, @assignment.created_at, DueDate.where(parent_id: response_map.reviewed_object_id)) | |||
end | |||
''' 9) def submitted_within_round ''' | |||
-->Extracted the logic for querying the SubmissionRecord table into a separate variable submission, so that it can be modified based on the round number. | |||
-->Used an if/else statement instead of two separate queries with similar conditions. | |||
-->Removed the call to try(:first).try(:created_at) at the end, as it's redundant with the exists? Method. | |||
-->Reduced cognitive complexity to 3. | |||
Before | |||
def submitted_within_round?(round, response_map, assignment_created, assignment_due_dates) | |||
submission_due_date = assignment_due_dates.where(round: round, deadline_type_id: 1).try(:first).try(:due_at) | |||
submission = SubmissionRecord.where(team_id: response_map.reviewee_id, operation: ['Submit File', 'Submit Hyperlink']) | |||
subm_created_at = submission.where(created_at: assignment_created..submission_due_date) | |||
if round > 1 | |||
submission_due_last_round = assignment_due_dates.where(round: round - 1, deadline_type_id: 1).try(:first).try(:due_at) | |||
subm_created_at = submission.where(created_at: submission_due_last_round..submission_due_date) | |||
end | |||
!subm_created_at.try(:first).try(:created_at).nil? | |||
end | |||
After | |||
def submitted_within_round?(round, response_map, assignment_created, assignment_due_dates) | |||
submission_due_date = assignment_due_dates.where(round: round, deadline_type_id: 1).try(:first).try(:due_at) | |||
submission = SubmissionRecord.where(team_id: response_map.reviewee_id, operation: ['Submit File', 'Submit Hyperlink']) | |||
if round > 1 | |||
submission_due_last_round = assignment_due_dates.where(round: round - 1, deadline_type_id: 1).try(:first).try(:due_at) | |||
submission = submission.where(created_at: submission_due_last_round..submission_due_date) | |||
else | |||
submission = submission.where(created_at: assignment_created..submission_due_date) | |||
end | |||
submission.exists? | |||
end | |||
''' 10) def get_data_for_review_report ''' | |||
-->Moved the initialization of rspan and response_counts outside of the loop. | |||
-->Used each_with_object instead of manually accumulating values. | |||
-->Used then to make the code more readable. | |||
-->Simplified the logic for checking if a team exists | |||
Before | |||
def get_data_for_review_report(reviewed_object_id, reviewer_id, type) | |||
rspan = 0 | |||
(1..@assignment.num_review_rounds).each { |round| instance_variable_set('@review_in_round_' + round.to_s, 0) } | |||
response_maps = ResponseMap.where(['reviewed_object_id = ? AND reviewer_id = ? AND type = ?', reviewed_object_id, reviewer_id, type]) | |||
response_maps.each do |ri| | |||
rspan += 1 if Team.exists?(id: ri.reviewee_id) | |||
responses = ri.response | |||
(1..@assignment.num_review_rounds).each do |round| | |||
instance_variable_set('@review_in_round_' + round.to_s, instance_variable_get('@review_in_round_' + round.to_s) + 1) if responses.exists?(round: round) | |||
end | |||
end | |||
[response_maps, rspan] | |||
end | |||
After | |||
def get_data_for_review_report(reviewed_object_id, reviewer_id, type) | |||
response_maps = ResponseMap.where(reviewed_object_id: reviewed_object_id, reviewer_id: reviewer_id, type: type) | |||
response_counts = Array.new(@assignment.num_review_rounds, 0) | |||
response_maps.each_with_object([[], 0]) do |response_map, (response_maps_accumulator, rspan)| | |||
next unless Team.exists?(id: response_map.reviewee_id) | |||
rspan += 1 | |||
responses = response_map.response | |||
response_counts = (1..@assignment.num_review_rounds).each_with_object(response_counts) do |round, response_counts_accumulator| | |||
response_counts_accumulator[round - 1] += 1 if responses.exists?(round: round) | |||
response_counts_accumulator | |||
end | |||
response_maps_accumulator << response_map | |||
end.then { |response_maps_accumulator, rspan| [response_maps_accumulator, rspan, response_counts] } | |||
end | |||
''' 11) def get_awarded_review_score ''' | |||
-->Extracting the names of the round-specific instance variables to a separate array, which is generated using map. | |||
-->Using each instead of each_with_index to set initial values for the instance variables. | |||
-->Combining the check for team_id with the subsequent loop over the rounds to avoid redundancy. | |||
-->Adding a check for nil before setting the instance variable to avoid raising an error if a score is missing. | |||
Before | |||
def get_awarded_review_score(reviewer_id, team_id) | |||
# Storing redundantly computed value in num_rounds variable | |||
num_rounds = @assignment.num_review_rounds | |||
# Setting values of instance variables | |||
(1..num_rounds).each { |round| instance_variable_set('@score_awarded_round_' + round.to_s, '-----') } | |||
# Iterating through list | |||
(1..num_rounds).each do |round| | |||
# Changing values of instance variable based on below condition | |||
unless team_id.nil? || team_id == -1.0 | |||
instance_variable_set('@score_awarded_round_' + round.to_s, @review_scores[reviewer_id][round][team_id].to_s + '%') | |||
end | |||
end | |||
end | |||
After | |||
def get_awarded_review_score(reviewer_id, team_id) | |||
num_rounds = @assignment.num_review_rounds | |||
round_variable_names = (1..num_rounds).map { |round| '@score_awarded_round_' + round.to_s } | |||
round_variable_names.each { |name| instance_variable_set(name, '-----') } | |||
unless team_id.nil? || team_id == -1.0 | |||
(1..num_rounds).each do |round| | |||
score = @review_scores[reviewer_id][round][team_id] | |||
instance_variable_set('@score_awarded_round_' + round.to_s, score.to_s + '%') unless score.nil? | |||
end | |||
end | |||
end | |||
''' 12) def list_review_submissions ''' | |||
-->This extracts the condition that checks whether team and participant are not nil into a separate method called review_submissions_available?. This makes the code more readable and easier to understand, while also reducing the complexity of the list_review_submissions method. | |||
Before | |||
def list_review_submissions(participant_id, reviewee_team_id, response_map_id) | |||
participant = Participant.find(participant_id) | |||
team = AssignmentTeam.find(reviewee_team_id) | |||
html = '' | |||
unless team.nil? || participant.nil? | |||
review_submissions_path = team.path + '_review' + '/' + response_map_id.to_s | |||
files = team.submitted_files(review_submissions_path) | |||
html += display_review_files_directory_tree(participant, files) if files.present? | |||
end | |||
html.html_safe | |||
end | |||
After | |||
def list_review_submissions(participant_id, reviewee_team_id, response_map_id) | |||
participant = Participant.find(participant_id) | |||
team = AssignmentTeam.find(reviewee_team_id) | |||
html = '' | |||
if review_submissions_available?(team, participant) | |||
review_submissions_path = team.path + '_review' + '/' + response_map_id.to_s | |||
files = team.submitted_files(review_submissions_path) | |||
html += display_review_files_directory_tree(participant, files) if files.present? | |||
end | |||
html.html_safe | |||
end | |||
def review_submissions_available?(team, participant) | |||
team.present? && participant.present? | |||
end | |||
''' 13) def check_submission_state ''' | |||
Few existing methods name are too long, this could have been changed by the team if they were working on that file - get_each_review_and_feedback_response_map, get_css_style_for_calibration_report | |||
-->Removed the unnecessary color argument and made the method return the color. | |||
-->Reduced the Cognitive Complexity by simplifying the conditional logic and nesting levels. | |||
-->Made the link check more concise. | |||
-->Broke the long line by formatting the conditional statements. | |||
-->Reduced cognitive complexity to 3. | |||
Before | |||
def check_submission_state(response_map, assignment_created, assignment_due_dates, round, color) | |||
if submitted_within_round?(round, response_map, assignment_created, assignment_due_dates) | |||
color.push 'purple' | |||
else | |||
link = submitted_hyperlink(round, response_map, assignment_created, assignment_due_dates) | |||
if link.nil? || (link !~ %r{https*:\/\/wiki(.*)}) # can be extended for github links in future | |||
color.push 'green' | |||
else | |||
link_updated_at = get_link_updated_at(link) | |||
color.push link_updated_since_last?(round, assignment_due_dates, link_updated_at) ? 'purple' : 'green' | |||
end | |||
end | |||
end | |||
After | |||
def check_submission_state(response_map, assignment_created, assignment_due_dates, round, color) | |||
if submitted_within_round?(round, response_map, assignment_created, assignment_due_dates) | |||
color.push('purple') | |||
else | |||
link = submitted_hyperlink(round, response_map, assignment_created, assignment_due_dates) | |||
if link.nil? || !link.start_with?('https://wiki') | |||
color.push('green') | |||
else | |||
link_updated_at = get_link_updated_at(link) | |||
if link_updated_since_last?(round, assignment_due_dates, link_updated_at) | |||
color.push('purple') | |||
else | |||
color.push('green') | |||
end | |||
end | |||
end | |||
end | |||
'''14) def get_awarded_review_score ''' | |||
In the refactored version, we have removed the unless condition and replaced it with a simple return statement, which reduces the cognitive complexity. We have also used the dig method to access the nested value of the @review_scores hash, which is more concise than the original version. Additionally, we have concatenated the score and the % symbol in a single string interpolation to make it more readable. Finally, we have removed the unnecessary use of the each method and initialized the round_variable_names variable outside of the block, which simplifies the code | |||
Before | |||
def get_awarded_review_score(reviewer_id, team_id) | |||
num_rounds = @assignment.num_review_rounds | |||
round_variable_names = (1..num_rounds).map { |round| '@score_awarded_round_' + round.to_s } | |||
round_variable_names.each { |name| instance_variable_set(name, '-----') } | |||
return if team_id.nil? || team_id == -1.0 | |||
(1..num_rounds).each do |round| | |||
score = @review_scores.dig(reviewer_id, round, team_id) | |||
instance_variable_set('@score_awarded_round_' + round.to_s, "#{score}%" ) unless score.nil? | |||
end | |||
end | |||
After | |||
def get_awarded_review_score(reviewer_id, team_id) | |||
num_rounds = @assignment.num_review_rounds | |||
round_variable_names = (1..num_rounds).map { |round| "@score_awarded_round_#{round}" } | |||
round_variable_names.each { |name| instance_variable_set(name, '-----') } | |||
return if team_id.nil? || team_id == -1.0 | |||
(1..num_rounds).each do |round| | |||
score = @review_scores.dig(reviewer_id, round, team_id) | |||
next if score.nil? | |||
instance_variable_set("@score_awarded_round_#{round}", "#{score}%") | |||
end | |||
end | |||
== Testing == | |||
''' 1)def display_volume_metric_chart''' | |||
[[File:refactor_2301_1.png|center|border|1000px]] | |||
''' 2)def display_tagging_interval_chart(intervals)''' | |||
[[File:refactor_2301_2.png|center|border|1000px]] | |||
''' 3)def sort_reviewer_by_review_volume_desc''' | |||
[[File:Refactor_2301_3.png|center|border|1000px]] | |||
''' 4)def initialize_chart_elements(reviewer)''' | |||
[[File:Refactor_2301_4.png|center|border|1000px]] | |||
''' 5)def calculate_key_chart_information(intervals)''' | |||
[[File:Refactor_2301_5.png|center|border|1000px]] | |||
''' 6)def review_metrics(round, team_id)''' | |||
[[File:Refactor_2301_6.png|center|border|1000px]] | |||
''' 7)def get_data_for_review_report(reviewed_object_id, reviewer_id, type)''' | |||
[[File:Refactor_2301_7.png|center|border|1000px]] | |||
''' 8)def get_team_color(response_map)''' | |||
[[File:Refactor_2301_8.png|center|border|1000px]] | |||
''' 9)def submitted_within_round?(round, response_map, assignment_created, assignment_due_dates)''' | |||
[[File:Refactor_2301_9.png|center|border|1000px]] | |||
''' 10) def get_data_for_review_report(reviewed_object_id, reviewer_id, type)''' | |||
[[File:Refactor_2301_10.png|center|border|1000px]] | |||
''' 11) def get_awarded_review_score(reviewer_id, team_id)''' | |||
[[File:Refactor_2301_11.png|center|border|1000px]] | |||
''' 12)def list_review_submissions(participant_id, reviewee_team_id, response_map_id)''' | |||
[[File:Refactor_2301_12.png|center|border|1000px]] | |||
''' 13) def check_submission_state(response_map, assignment_created, assignment_due_dates, round, color)''' | |||
[[File:Refactor_2301_13.png|center|border|1000px]] | |||
== References== | |||
[ http://wiki.expertiza.ncsu.edu/index.php/CSC/ECE_517_Fall_2019_-_E1948._Refactor_review_mapping_helper.rb ] | |||
[ https://github.com/expertiza/expertiza/pull/1526 ] | |||
[ https://bit.ly/2NiWlgI ] | |||
[ https://github.com/ArshdeepSinghSyal/expertiza/tree/beta ] | |||
== Project Mentors == | |||
1) Edward Gehringer (efg@ncsu.edu) | |||
2) Jialin Cui (jcui9@ncsu.edu) | |||
== Team Members == | |||
1) Kunal Shah (kshah24@ncsu.edu) | |||
2) Ritwik Vaidya (rvaidya2@ncsu.edu) | |||
3) Dakshil Kanakia (drkanaki@ncsu.edu) |
Latest revision as of 05:25, 28 March 2023
Introduction
This page gives a description of the changes made for the review_mapping_helper.rb of Expertiza based OSS project.
Overview of Expertiza
Expertiza is a learning management system that is available as open source software and was created using Ruby on Rails. It has a wide range of features and capabilities, where students can submit and peer-review learning objects (articles, codes, websites, etc). Instructors add and grade the assignments submitted by students to Expertiza. The files that are largely addressed in this project, such as assignment_node.rb, course_node.rb, team_node.rb, folder_node.rb, and questionnaire_node.rb, are essential in executing this functionality. It is supported by the National Science Foundation.
Problem Statement
The review_mapping_helper.rb has multiple functions with a high complexities namely - Cognitive, Perceived, Cyclomatic, Assignment Branch Condition size (ABC size) and Lines of Code (LOC). The review_mapping_helper.rb has methods which exceeds the limit on lines of code. It is missing proper comments for each functionality.
Changes Made
1)def display_volume_metric_chart
Reduced lines of code by removing unnecessary line breaks in display_volume_metric_chart
Before
def display_volume_metric_chart(reviewer) labels, reviewer_data, all_reviewers_data = initialize_chart_elements(reviewer) data = { labels: labels, datasets: [ { label: 'vol.', backgroundColor: 'rgba(255,99,132,0.8)', borderWidth: 1, data: reviewer_data, yAxisID: 'bar-y-axis1' }, { label: 'avg. vol.', backgroundColor: 'rgba(255,206,86,0.8)', borderWidth: 1, data: all_reviewers_data, yAxisID: 'bar-y-axis2' } ] } options = { legend: { position: 'top', labels: { usePointStyle: true } }, width: '200', height: '125', scales: { yAxes: [{ stacked: true, id: 'bar-y-axis1', barThickness: 10 }, { display: false, stacked: true, id: 'bar-y-axis2', barThickness: 15, type: 'category', categoryPercentage: 0.8, barPercentage: 0.9, gridLines: { offsetGridLines: true } }], xAxes: [{ stacked: false, ticks: { beginAtZero: true, stepSize: 50, max: 400 } }] } } horizontal_bar_chart data, options end
After
def display_volume_metric_chart(reviewer) labels, reviewer_data, all_reviewers_data = initialize_chart_elements(reviewer) data = { labels: labels, datasets: [ { label: 'vol.', backgroundColor: 'rgba(255,99,132,0.8)', borderWidth: 1, data: reviewer_data, yAxisID: 'bar-y-axis1' }, { label: 'avg. vol.', backgroundColor: 'rgba(255,206,86,0.8)', borderWidth: 1, data: all_reviewers_data, yAxisID: 'bar-y-axis2' } ] } options = { legend: { position: 'top', labels: { usePointStyle: true } }, width: '200', height: '125', scales: { yAxes: [ { stacked: true, id: 'bar-y-axis1', barThickness: 10 }, { display: false, stacked: true, id: 'bar-y-axis2', barThickness: 15, type: 'category', categoryPercentage: 0.8, barPercentage: 0.9, gridLines: { offsetGridLines: true } } ], xAxes: [ { stacked: false, ticks: { beginAtZero: true, stepSize: 50, max: 400 } } ] } } horizontal_bar_chart(data, options) end
2) def display_tagging_interval_chart
-->Added early return if intervals array is empty.
-->Removed unnecessary comment.
-->Simplified datasets array initialization by always adding mean data
Before
def display_tagging_interval_chart(intervals) # if someone did not do any tagging in 30 seconds, then ignore this interval threshold = 30 intervals = intervals.select { |v| v < threshold } unless intervals.empty? interval_mean = intervals.reduce(:+) / intervals.size.to_f end # build the parameters for the chart data = { labels: [*1..intervals.length], datasets: [ { backgroundColor: 'rgba(255,99,132,0.8)', data: intervals, label: 'time intervals' }, unless intervals.empty? { data: Array.new(intervals.length, interval_mean), label: 'Mean time spent' } end ] } options = { width: '200', height: '125', scales: { yAxes: [{ stacked: false, ticks: { beginAtZero: true } }], xAxes: [{ stacked: false }] } } line_chart data, options end
After
def display_tagging_interval_chart(intervals) threshold = 30 intervals = intervals.select { |v| v < threshold } interval_mean = intervals.sum / intervals.size.to_f unless intervals.empty? data = { labels: [*1..intervals.length], datasets: [ { backgroundColor: 'rgba(255,99,132,0.8)', data: intervals, label: 'time intervals' }, *(!intervals.empty? && [{ data: [interval_mean] * intervals.length, label: 'Mean time spent' }]) ] } options = { width: '200', height: '125', scales: { yAxes: [{ stacked: false, ticks: { beginAtZero: true } }], xAxes: [{ stacked: false }] } } line_chart(data, options) end
3) def sort_reviewer_by_review_volume_desc
Made the method more readable, maintainable, and efficient, and also reduce the complexity and cognitive complexity of the code. In this refactored version, we have:
-->Extracted the logic for getting the review volumes into a separate block, to make it easier to understand and test.
-->Simplified the code for getting the reviewer's overall average volume and average volume per round.
-->Used concat instead of push to add elements to the avg_vol_per_round array.
-->Sorted the reviewers by their review volume in descending order using the sort_by! method and a block.
-->Used a to_i method instead of to_f.to_i to convert the number of review rounds to an integer.
-->Removed the unnecessary @all_reviewers_avg_vol_per_round variable since it's not being used in the method.
Before
def sort_reviewer_by_review_volume_desc @reviewers.each do |r| # get the volume of review comments review_volumes = Response.volume_of_review_comments(@assignment.id, r.id) r.avg_vol_per_round = [] review_volumes.each_index do |i| if i.zero? r.overall_avg_vol = review_volumes[0] else r.avg_vol_per_round.push(review_volumes[i]) end end end # get the number of review rounds for the assignment @num_rounds = @assignment.num_review_rounds.to_f.to_i @all_reviewers_avg_vol_per_round = [] @all_reviewers_overall_avg_vol = @reviewers.inject(0) { |sum, r| sum + r.overall_avg_vol } / (@reviewers.blank? ? 1 : @reviewers.length) @num_rounds.times do |round| @all_reviewers_avg_vol_per_round.push(@reviewers.inject(0) { |sum, r| sum + r.avg_vol_per_round[round] } / (@reviewers.blank? ? 1 : @reviewers.length)) end @reviewers.sort! { |r1, r2| r2.overall_avg_vol <=> r1.overall_avg_vol } end
After
def sort_reviewer_by_review_volume_desc # Get the volume of review comments for each reviewer @reviewers.each do |reviewer| reviewer.avg_vol_per_round = [] review_volumes = Response.volume_of_review_comments(@assignment.id, reviewer.id) reviewer.overall_avg_vol = review_volumes.first reviewer.avg_vol_per_round.concat(review_volumes[1..-1]) end # Sort reviewers by their review volume in descending order @reviewers.sort_by! { |reviewer| -reviewer.overall_avg_vol } # Get the number of review rounds for the assignment @num_rounds = @assignment.num_review_rounds.to_i @all_reviewers_avg_vol_per_round = [] end
4) def initialize_chart_elements
Removed the round variable since it was only used to track the round number, which is already captured by the labels array.
-->Changed the push method to the << operator for brevity.
-->Removed the unless condition to reduce nesting, using if instead and flipping the condition.
-->Removed unnecessary initialization of arrays (reviewer_data, all_reviewers_data) since they will be created within the loop
Before
def initialize_chart_elements(reviewer) round = 0 labels = [] reviewer_data = [] all_reviewers_data = [] # display avg volume for all reviewers per round @num_rounds.times do |rnd| next unless @all_reviewers_avg_vol_per_round[rnd] > 0 round += 1 labels.push round reviewer_data.push reviewer.avg_vol_per_round[rnd] all_reviewers_data.push @all_reviewers_avg_vol_per_round[rnd] end labels.push 'Total' reviewer_data.push reviewer.overall_avg_vol all_reviewers_data.push @all_reviewers_overall_avg_vol [labels, reviewer_data, all_reviewers_data] end
After
def initialize_chart_elements(reviewer) labels, reviewer_data, all_reviewers_data = [], [], [] @num_rounds.times do |rnd| next if @all_reviewers_avg_vol_per_round[rnd] <= 0 labels << (rnd + 1) reviewer_data << reviewer.avg_vol_per_round[rnd] all_reviewers_data << @all_reviewers_avg_vol_per_round[rnd] end labels << 'Total' reviewer_data << reviewer.overall_avg_vol all_reviewers_data << @all_reviewers_overall_avg_vol [labels, reviewer_data, all_reviewers_data] end
5)def calculate_key_chart_information
-->Replaced select with select! to modify the intervals array in place instead of creating a new one.
-->Removed unnecessary comments.
-->Removed sum variable and integrated it directly into the calculation of variance.
-->Removed the metrics hash object initialization to simplify code.
-->Removed unnecessary :stand_dev symbol from the hash object initialization and calculated its value directly.
Before
def calculate_key_chart_information(intervals) # if someone did not do any tagging in 30 seconds, then ignore this interval threshold = 30 interval_precision = 2 # Round to 2 Decimal Places intervals = intervals.select { |v| v < threshold } # Get Metrics once tagging intervals are available unless intervals.empty? metrics = {} metrics[:mean] = (intervals.reduce(:+) / intervals.size.to_f).round(interval_precision) metrics[:min] = intervals.min metrics[:max] = intervals.max sum = intervals.inject(0) { |accum, i| accum + (i - metrics[:mean])**2 } metrics[:variance] = (sum / intervals.size.to_f).round(interval_precision) metrics[:stand_dev] = Math.sqrt(metrics[:variance]).round(interval_precision) metrics end # if no Hash object is returned, the UI handles it accordingly end
After
def calculate_key_chart_information(intervals) # if someone did not do any tagging in 30 seconds, then ignore this interval threshold = 30 interval_precision = 2 # Round to 2 Decimal Places intervals.select! { |v| v < threshold } # Get Metrics once tagging intervals are available unless intervals.empty? mean = intervals.sum / intervals.size.to_f variance = intervals.map { |v| (v - mean) ** 2 }.sum / intervals.size.to_f metrics = { mean: mean.round(interval_precision), min: intervals.min, max: intervals.max, variance: variance.round(interval_precision), stand_dev: Math.sqrt(variance).round(interval_precision) } metrics end # if no Hash object is returned, the UI handles it accordingly end
6) def review_metrics
The changes should improve the code's readability and maintainability while also addressing the issues highlighted by the feedback.
-->The instance_variable_set calls were refactored to use string interpolation for readability.
-->The if statement was simplified by using the dig method to access nested hashes and the values_at method to check if all required metrics are present.
-->The each loop was changed to iterate over the avg_and_ranges hash directly to avoid unnecessary nil values.
-->The logic for setting the metric_value variable was simplified and moved inside the loop to avoid repeated code.
-->The cognitive complexity has been reduced to 4.
Before
def review_metrics(round, team_id) %i[max min avg].each { |metric| instance_variable_set('@' + metric.to_s, '-----') } if @avg_and_ranges[team_id] && @avg_and_ranges[team_id][round] && %i[max min avg].all? { |k| @avg_and_ranges[team_id][round].key? k } %i[max min avg].each do |metric| metric_value = @avg_and_ranges[team_id][round][metric].nil? ? '-----' : @avg_and_ranges[team_id][round][metric].round(0).to_s + '%' instance_variable_set('@' + metric.to_s, metric_value) end end end
After
def review_metrics(round, team_id) %i[max min avg].each do |metric| instance_variable_set("@#{metric}", '-----') end avg_and_ranges = @avg_and_ranges&.dig(team_id, round) if avg_and_ranges && avg_and_ranges.values_at(:max, :min, :avg).all?(&:present?) avg_and_ranges.each do |metric, value| metric_value = "#{value.round(0)}%" instance_variable_set("@#{metric}", metric_value) end end end
7) def get_data_for_review_report
The changes should make the code more readable and easier to understand, and should also reduce the ABC size and the number of lines.
-->The instance_variable_set calls were replaced with a new response_counts array to keep track of the response counts for each round.
-->The response_counts array is initialized to an array of zeros with a length of @assignment.num_review_rounds.
-->The where call was simplified to use a hash instead of an array.
-->The loop over the response_maps was refactored to use a more descriptive variable name (response_map) and to check whether the reviewee team exists before counting its responses. -->The loop over the rounds was simplified to use an index variable (round - 1) to access the correct element in the response_counts array.
-->The method now returns an array of three values: the response_maps, the rspan, and the response_counts.
Before
def get_data_for_review_report(reviewed_object_id, reviewer_id, type) rspan = 0 (1..@assignment.num_review_rounds).each { |round| instance_variable_set('@review_in_round_' + round.to_s, 0) } response_maps = ResponseMap.where(['reviewed_object_id = ? AND reviewer_id = ? AND type = ?', reviewed_object_id, reviewer_id, type]) response_maps.each do |ri| rspan += 1 if Team.exists?(id: ri.reviewee_id) responses = ri.response (1..@assignment.num_review_rounds).each do |round| instance_variable_set('@review_in_round_' + round.to_s, instance_variable_get('@review_in_round_' + round.to_s) + 1) if responses.exists?(round: round) end end [response_maps, rspan] end
After
def get_data_for_review_report(reviewed_object_id, reviewer_id, type) rspan = 0 response_counts = Array.new(@assignment.num_review_rounds, 0) response_maps = ResponseMap.where(reviewed_object_id: reviewed_object_id, reviewer_id: reviewer_id, type: type) response_maps.each do |response_map| if Team.exists?(id: response_map.reviewee_id) rspan += 1 responses = response_map.response (1..@assignment.num_review_rounds).each do |round| response_counts[round - 1] += 1 if responses.exists?(round: round) end end end [response_maps, rspan, response_counts] end
8) def get_team_color
--> Removed the redundant variables assignment_created and assignment_due_dates and passed them directly as arguments to the obtain_team_color method call.
-->Removed the unnecessary else clause, since the function will always return before reaching that point.
-->Reduced cognitive complexity to 4
Before
def get_team_color(response_map) # Storing redundantly computed value in a variable assignment_created = @assignment.created_at # Storing redundantly computed value in a variable assignment_due_dates = DueDate.where(parent_id: response_map.reviewed_object_id) # Returning colour based on conditions if Response.exists?(map_id: response_map.id) if !response_map.try(:reviewer).try(:review_grade).nil? 'brown' elsif response_for_each_round?(response_map) 'blue' else obtain_team_color(response_map, assignment_created, assignment_due_dates) end else 'red' end end
After
def get_team_color(response_map) if Response.exists?(map_id: response_map.id) if !response_map.try(:reviewer).try(:review_grade).nil? return 'brown' elsif response_for_each_round?(response_map) return 'blue' end else return 'red' end obtain_team_color(response_map, @assignment.created_at, DueDate.where(parent_id: response_map.reviewed_object_id)) end
9) def submitted_within_round
-->Extracted the logic for querying the SubmissionRecord table into a separate variable submission, so that it can be modified based on the round number.
-->Used an if/else statement instead of two separate queries with similar conditions.
-->Removed the call to try(:first).try(:created_at) at the end, as it's redundant with the exists? Method.
-->Reduced cognitive complexity to 3.
Before
def submitted_within_round?(round, response_map, assignment_created, assignment_due_dates) submission_due_date = assignment_due_dates.where(round: round, deadline_type_id: 1).try(:first).try(:due_at) submission = SubmissionRecord.where(team_id: response_map.reviewee_id, operation: ['Submit File', 'Submit Hyperlink']) subm_created_at = submission.where(created_at: assignment_created..submission_due_date) if round > 1 submission_due_last_round = assignment_due_dates.where(round: round - 1, deadline_type_id: 1).try(:first).try(:due_at) subm_created_at = submission.where(created_at: submission_due_last_round..submission_due_date) end !subm_created_at.try(:first).try(:created_at).nil? end
After
def submitted_within_round?(round, response_map, assignment_created, assignment_due_dates) submission_due_date = assignment_due_dates.where(round: round, deadline_type_id: 1).try(:first).try(:due_at) submission = SubmissionRecord.where(team_id: response_map.reviewee_id, operation: ['Submit File', 'Submit Hyperlink']) if round > 1 submission_due_last_round = assignment_due_dates.where(round: round - 1, deadline_type_id: 1).try(:first).try(:due_at) submission = submission.where(created_at: submission_due_last_round..submission_due_date) else submission = submission.where(created_at: assignment_created..submission_due_date) end submission.exists? end
10) def get_data_for_review_report
-->Moved the initialization of rspan and response_counts outside of the loop.
-->Used each_with_object instead of manually accumulating values.
-->Used then to make the code more readable.
-->Simplified the logic for checking if a team exists
Before
def get_data_for_review_report(reviewed_object_id, reviewer_id, type) rspan = 0 (1..@assignment.num_review_rounds).each { |round| instance_variable_set('@review_in_round_' + round.to_s, 0) } response_maps = ResponseMap.where(['reviewed_object_id = ? AND reviewer_id = ? AND type = ?', reviewed_object_id, reviewer_id, type]) response_maps.each do |ri| rspan += 1 if Team.exists?(id: ri.reviewee_id) responses = ri.response (1..@assignment.num_review_rounds).each do |round| instance_variable_set('@review_in_round_' + round.to_s, instance_variable_get('@review_in_round_' + round.to_s) + 1) if responses.exists?(round: round) end end [response_maps, rspan] end
After
def get_data_for_review_report(reviewed_object_id, reviewer_id, type) response_maps = ResponseMap.where(reviewed_object_id: reviewed_object_id, reviewer_id: reviewer_id, type: type) response_counts = Array.new(@assignment.num_review_rounds, 0) response_maps.each_with_object([[], 0]) do |response_map, (response_maps_accumulator, rspan)| next unless Team.exists?(id: response_map.reviewee_id) rspan += 1 responses = response_map.response response_counts = (1..@assignment.num_review_rounds).each_with_object(response_counts) do |round, response_counts_accumulator| response_counts_accumulator[round - 1] += 1 if responses.exists?(round: round) response_counts_accumulator end response_maps_accumulator << response_map end.then { |response_maps_accumulator, rspan| [response_maps_accumulator, rspan, response_counts] } end
11) def get_awarded_review_score
-->Extracting the names of the round-specific instance variables to a separate array, which is generated using map.
-->Using each instead of each_with_index to set initial values for the instance variables.
-->Combining the check for team_id with the subsequent loop over the rounds to avoid redundancy.
-->Adding a check for nil before setting the instance variable to avoid raising an error if a score is missing.
Before
def get_awarded_review_score(reviewer_id, team_id) # Storing redundantly computed value in num_rounds variable num_rounds = @assignment.num_review_rounds # Setting values of instance variables (1..num_rounds).each { |round| instance_variable_set('@score_awarded_round_' + round.to_s, '-----') } # Iterating through list (1..num_rounds).each do |round| # Changing values of instance variable based on below condition unless team_id.nil? || team_id == -1.0 instance_variable_set('@score_awarded_round_' + round.to_s, @review_scores[reviewer_id][round][team_id].to_s + '%') end end end
After
def get_awarded_review_score(reviewer_id, team_id) num_rounds = @assignment.num_review_rounds round_variable_names = (1..num_rounds).map { |round| '@score_awarded_round_' + round.to_s } round_variable_names.each { |name| instance_variable_set(name, '-----') } unless team_id.nil? || team_id == -1.0 (1..num_rounds).each do |round| score = @review_scores[reviewer_id][round][team_id] instance_variable_set('@score_awarded_round_' + round.to_s, score.to_s + '%') unless score.nil? end end end
12) def list_review_submissions
-->This extracts the condition that checks whether team and participant are not nil into a separate method called review_submissions_available?. This makes the code more readable and easier to understand, while also reducing the complexity of the list_review_submissions method.
Before
def list_review_submissions(participant_id, reviewee_team_id, response_map_id) participant = Participant.find(participant_id) team = AssignmentTeam.find(reviewee_team_id) html = unless team.nil? || participant.nil? review_submissions_path = team.path + '_review' + '/' + response_map_id.to_s files = team.submitted_files(review_submissions_path) html += display_review_files_directory_tree(participant, files) if files.present? end html.html_safe end
After
def list_review_submissions(participant_id, reviewee_team_id, response_map_id) participant = Participant.find(participant_id) team = AssignmentTeam.find(reviewee_team_id) html = if review_submissions_available?(team, participant) review_submissions_path = team.path + '_review' + '/' + response_map_id.to_s files = team.submitted_files(review_submissions_path) html += display_review_files_directory_tree(participant, files) if files.present? end html.html_safe end def review_submissions_available?(team, participant) team.present? && participant.present? end
13) def check_submission_state
Few existing methods name are too long, this could have been changed by the team if they were working on that file - get_each_review_and_feedback_response_map, get_css_style_for_calibration_report
-->Removed the unnecessary color argument and made the method return the color.
-->Reduced the Cognitive Complexity by simplifying the conditional logic and nesting levels.
-->Made the link check more concise.
-->Broke the long line by formatting the conditional statements.
-->Reduced cognitive complexity to 3.
Before
def check_submission_state(response_map, assignment_created, assignment_due_dates, round, color) if submitted_within_round?(round, response_map, assignment_created, assignment_due_dates) color.push 'purple' else link = submitted_hyperlink(round, response_map, assignment_created, assignment_due_dates) if link.nil? || (link !~ %r{https*:\/\/wiki(.*)}) # can be extended for github links in future color.push 'green' else link_updated_at = get_link_updated_at(link) color.push link_updated_since_last?(round, assignment_due_dates, link_updated_at) ? 'purple' : 'green' end end end
After
def check_submission_state(response_map, assignment_created, assignment_due_dates, round, color) if submitted_within_round?(round, response_map, assignment_created, assignment_due_dates) color.push('purple') else link = submitted_hyperlink(round, response_map, assignment_created, assignment_due_dates) if link.nil? || !link.start_with?('https://wiki') color.push('green') else link_updated_at = get_link_updated_at(link) if link_updated_since_last?(round, assignment_due_dates, link_updated_at) color.push('purple') else color.push('green') end end end end
14) def get_awarded_review_score
In the refactored version, we have removed the unless condition and replaced it with a simple return statement, which reduces the cognitive complexity. We have also used the dig method to access the nested value of the @review_scores hash, which is more concise than the original version. Additionally, we have concatenated the score and the % symbol in a single string interpolation to make it more readable. Finally, we have removed the unnecessary use of the each method and initialized the round_variable_names variable outside of the block, which simplifies the code
Before def get_awarded_review_score(reviewer_id, team_id) num_rounds = @assignment.num_review_rounds round_variable_names = (1..num_rounds).map { |round| '@score_awarded_round_' + round.to_s } round_variable_names.each { |name| instance_variable_set(name, '-----') } return if team_id.nil? || team_id == -1.0 (1..num_rounds).each do |round| score = @review_scores.dig(reviewer_id, round, team_id) instance_variable_set('@score_awarded_round_' + round.to_s, "#{score}%" ) unless score.nil? end end
After
def get_awarded_review_score(reviewer_id, team_id) num_rounds = @assignment.num_review_rounds round_variable_names = (1..num_rounds).map { |round| "@score_awarded_round_#{round}" } round_variable_names.each { |name| instance_variable_set(name, '-----') } return if team_id.nil? || team_id == -1.0 (1..num_rounds).each do |round| score = @review_scores.dig(reviewer_id, round, team_id) next if score.nil? instance_variable_set("@score_awarded_round_#{round}", "#{score}%") end end
Testing
1)def display_volume_metric_chart
2)def display_tagging_interval_chart(intervals)
3)def sort_reviewer_by_review_volume_desc
4)def initialize_chart_elements(reviewer)
5)def calculate_key_chart_information(intervals)
6)def review_metrics(round, team_id)
7)def get_data_for_review_report(reviewed_object_id, reviewer_id, type)
8)def get_team_color(response_map)
9)def submitted_within_round?(round, response_map, assignment_created, assignment_due_dates)
10) def get_data_for_review_report(reviewed_object_id, reviewer_id, type)
11) def get_awarded_review_score(reviewer_id, team_id)
12)def list_review_submissions(participant_id, reviewee_team_id, response_map_id)
13) def check_submission_state(response_map, assignment_created, assignment_due_dates, round, color)
References
[ https://github.com/expertiza/expertiza/pull/1526 ]
[ https://github.com/ArshdeepSinghSyal/expertiza/tree/beta ]
Project Mentors
1) Edward Gehringer (efg@ncsu.edu)
2) Jialin Cui (jcui9@ncsu.edu)
Team Members
1) Kunal Shah (kshah24@ncsu.edu)
2) Ritwik Vaidya (rvaidya2@ncsu.edu)
3) Dakshil Kanakia (drkanaki@ncsu.edu)