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-

Comments added within various functions

The comments about the internal functionality of the following functions were added : get_review_metrics, get_review_metrics, get_review_metrics, get_review_metrics, get_review_metrics and their sub-functions.

Example : get_review_metrics function

Before

  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

After

  # gets minimum, maximum and average value for all the reviews
 def get_review_metrics(round, team_id)
    # Setting values of instance variables
    ['max', 'min', 'avg'].each {|metric| instance_variable_set('@' + metric, '-----') }
    # Fetching value of @avg_and_ranges[team_id][round] 
    x = @avg_and_ranges.dig(team_id, round)

    if x && %i[max min avg].all? {|k| x.key? k }
      # Iterating though the list
      ['max', 'min', 'avg'].each do |metric|
        # setting values of variables based on certain conditions
	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

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. We also did automated testing using RSpec. 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.

Travis-CI Build Testing

Travis-CI Build Test of the beta branch after a refactored function is merged in the beta branch.

Automated RSpec Testing

We carried out automated testing using Rspec for the files where we made changes to, during the project. All the 14 test cases in the spec file successfully passed. Note: In some cases, to resolve the code climate issues we have broken down an existing long function into logical smaller functions. In these cases, we are neither manipulating any instance or class variables nor are we manipulating any values being fetched from the database in any way. Hence we decided it would not be practical or useful to write RSpec tests to test inbuilt Ruby and Rails functionalities. We followed "The Magic Tricks of Testing by Sandi Metz - Rails Conf 2013" [[1] which asserts the importance of not adding any unnecessary new tests as well as getting rid of any kind of unnecessary tests.

Link to Rspec Testing Video: [2]