CSC/ECE 517 Fall 2019 - E1948. Refactor review mapping helper.rb

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

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 Also, it is missing proper comments for each functionality. Cyclomatic complexity of most of the methods is way too high as per the standard defined in the Code Climate.

Flowchart

The following process is carried out to complete the project-

Refactoring

The method get_review_metrics had cognitive complexity of 6 and ABC size complexity 25. It was refactored using .dig() function and removing .to_s function by changing the variable type.

Before:

def get_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 get_review_metrics(round, team_id)
    ['max', 'min', 'avg'].each {|metric| instance_variable_set('@' + metric, '-----') }
    x = @avg_and_ranges.dig(team_id, round)
    if x && %i[max min avg].all? {|k| x.key? k }
      ['max', 'min', 'avg'].each do |metric|
	average_metric = @avg_and_ranges.dig(team_id, round, metric)
        metric_value = average_metric.nil? ? '-----' : average_metric.round(0).to_s + '%'
        instance_variable_set('@' + metric, metric_value)
      end
    end
  end

The method get_awarded_review_score had ABC size complexity of 19. It was refactored using .dig() function and creating new variable for redundant computations.

Before:

def get_awarded_review_score(reviewer_id, team_id)
    (1..@assignment.num_review_rounds).each {|round| instance_variable_set("@score_awarded_round_" + round.to_s, '-----') }
    (1..@assignment.num_review_rounds).each do |round|
      if @review_scores[reviewer_id] && @review_scores[reviewer_id][round] && @review_scores[reviewer_id][round][team_id] && @review_scores[reviewer_id][round][team_id] != -1.0
        instance_variable_set("@score_awarded_round_" + round.to_s, @review_scores[reviewer_id][round][team_id].inspect + '%')
      end
    end
  end

After:

def get_awarded_review_score(reviewer_id, team_id) 
	num_rounds = @assignment.num_review_rounds
    (1..num_rounds).each {|round| instance_variable_set("@score_awarded_round_" + round.to_s, '-----') }
    (1..num_rounds).each do |round|
	  teamID = @review_scores.dig(reviewer_id, round, team_id)
      if teamID != nil && teamID != -1.0
         instance_variable_set("@score_awarded_round_" + round.to_s, teamID.inspect + '%')
      end
    end
  end

The method get_team_color had Cyclomatic, Perceived, Lines of Code and ABC size complexity. It was refactored by breaking the logical functionality into two sub functions and the main (get_team_color) function.

Before:

def get_team_colour(response_map)
    assignment_created = @assignment.created_at
    assignment_due_dates = DueDate.where(parent_id: response_map.reviewed_object_id)
    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
        color = []
        (1..@assignment.num_review_rounds).each do |round|
          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? or (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
        color[-1]
      end
    else
      'red'
    end
  end

After:

def get_team_colour(response_map)
    assignment_created = @assignment.created_at
    assignment_due_dates = DueDate.where(parent_id: response_map.reviewed_object_id)
    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_colour(response_map,assignment_created,assignment_due_dates)
      end
    else
      'red'
    end
  end
  # loops through the number of assignment review rounds and obains the team colour
  def obtain_team_colour(response_map, assignment_created, assignment_due_dates)
    color = []
    (1..@assignment.num_review_rounds).each do |round|
      check_submission_state(response_map, assignment_created, assignment_due_dates, round)
    end
    color[-1]
  end
  # checks the submission state within each round and assigns team colour
  def check_submission_state(response_map, assignment_created, assignment_due_dates, round)
    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? or (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

The method get_each_review_and_feedback_response_map had high ABC size complexity. It was refactored by breaking the logical functionality into a sub function and the main (get_each_review_and_feedback_map) function.

Before:

def get_each_review_and_feedback_response_map(author)
    @team_id = TeamsUser.team_id(@id.to_i, author.user_id)
    # Calculate how many responses one team received from each round
    # It is the feedback number each team member should make
    @review_response_map_ids = ReviewResponseMap.where(["reviewed_object_id = ? and reviewee_id = ?", @id, @team_id]).pluck("id")
    {1 => 'one', 2 => 'two', 3 => 'three'}.each do |key, round_num|
      instance_variable_set('@review_responses_round_' + round_num,
                            Response.where(["map_id IN (?) and round = ?", @review_response_map_ids, key]))
      # Calculate feedback response map records
      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
    # rspan means the all peer reviews one student received, including unfinished one
    @rspan_round_one = @review_responses_round_one.length
    @rspan_round_two = @review_responses_round_two.length
    @rspan_round_three = @review_responses_round_three.nil? ? 0 : @review_responses_round_three.length
  end

After:

def get_each_review_and_feedback_response_map(author)
    @team_id = TeamsUser.team_id(@id.to_i, author.user_id)
    # Calculate how many responses one team received from each round
    # It is the feedback number each team member should make
    @review_response_map_ids = ReviewResponseMap.where(["reviewed_object_id = ? and reviewee_id = ?", @id, @team_id]).pluck("id")
    feedback_response_map_record(author)
    # rspan means the all peer reviews one student received, including unfinished one
    @rspan_round_one = @review_responses_round_one.length
    @rspan_round_two = @review_responses_round_two.length
    @rspan_round_three = @review_responses_round_three.nil? ? 0 : @review_responses_round_three.length
  end
  def feedback_response_map_record(author)
    {1 => 'one', 2 => 'two', 3 => 'three'}.each do |key, round_num|
      instance_variable_set('@review_responses_round_' + round_num,
                            Response.where(["map_id IN (?) and round = ?", @review_response_map_ids, key]))
      # Calculate feedback response map records
      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
  end

The method get_css_style_for_calibration_report had ABC size complexity of 19. It was refactored using .dig() function and creating new variable for redundant computations.

Before:

def get_css_style_for_calibration_report(diff)
    # diff - difference between stu's answer and instructor's answer
    dict = {0 => 'c5',1 => 'c4',2 => 'c3',3 => 'c2'}
    if dict.key?(diff.abs)
      css_class = dict[diff.abs]
    else
      css_class = 'c1'
    end
    css_class
  end

After:

def get_css_style_for_calibration_report(diff)
    # diff - difference between stu's answer and instructor's answer
    dict = {0 => 'c5', 1 => 'c4', 2 => 'c3', 3 => 'c2'}
    css_class = if dict.key?(diff.abs)
                  dict[diff.abs]
                else
                  css_class = 'c1'
                end
    css_class
  end

Testing

Test Plan

There were refactoring changes made in the function. Testing was carried out to check the Cognitive, Cyclomatic, Perceived, Lines of Code and Assignment Branch Condition (ABC) size complexities of these functions. Rubocop was used to test these complexities of the functions. For these cases, RSpec testing, was also carried out using the original spec file and it showed no new errors. Also, Travis-CI build of each pull request merge was checked to ensure that the code is working properly after refactoring.

Rubocop Testing

Rubocop testing results for "get_review_metrics" function after refactoring.

Rubocop testing results for "get_awarded_review_score" function after refactoring.

Rubocop testing results for "get_team_color" function after refactoring.

Rubocop testing results for "get_each_review_and_feedback_response_map" function after refactoring.

Rubocop testing results for "get_css_style_for_calibration_report" function after refactoring.

References

1) https://en.wikipedia.org/wiki/Cyclomatic_complexity

2) https://docs.codeclimate.com/docs/cognitive-complexity

3) https://en.wikipedia.org/wiki/ABC_Software_Metric

Team Members

Aditya Govil

Arshdeep Singh Syal

Rohan Rajesh Patil