CSC/ECE 517 Spring 2023 - E2301. Refactor review maping helper

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) 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) Simplified the unless condition by using a single-line unless expression and removing the interval_mean variable initialization outside of it. Used the sum method to calculate the sum of the intervals instead of reduce(:+). Used a single-line conditional operator (!intervals.empty? && ...) to add the "Mean time spent" dataset to the chart, avoiding the need for an unless statement. Removed unnecessary curly braces around single-line hashes

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) 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) 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)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