CSC/ECE 517 Spring 2019 - Project E1913. 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 wrong naming convention, they are classic examples of bad function names. These functions are to be refactored accordingly. Few of the variables names used in the method must also be refactored to make it more relevant and understandable. In addition, some function's code is to be optimized to ensure that it follows DRY principle. Tests must be written for the functions which are modified. Also, comments must be added to all functions of the file. The create_report_table_header function contains HTML code which should be ideally placed in the view as partials, which will allow to easily reuse the code in Rails application.

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

The method get_css_style_for_calibration_report had a switch case. It was refactored using a dictionary where the case numbers were keys and the outputs of each case were the corresponding values.

Before:

def get_css_style_for_calibration_report(diff)
    # diff - difference between stu's answer and instructor's answer
    css_class = case diff.abs
                when 0
                  'c5'
                when 1
                  'c4'
                when 2
                  'c3'
                when 3
                  'c2'
                else
                  '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'}
    if dict.key?(diff.abs)
      css_class = dict[diff.abs]
    else
      css_class = 'c1'
    end
    css_class
  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

Test Plan

There were changes made in the function name as well as one of the function was added to partials. Testing was carried out to check the behavior of these functions. In one of the case, a drop-down menu was tested i.e, all the links that were accessed were tested. It was checked that clicking on every link produced the desired output in all the options of the drop-down menu. For these cases, Manual testing, as well as RSpec testing, was carried out.

Manual Testing

For the methods which were called in views, manual testing was carried out to check that the code was not affected after refactoring methods.

RSpec Testing

RSpec testing was carried out for the methods that had been refactored. The partial that was added had impacts on the drop-down menus. Given below is one of the snippets of the entire code written in the file review_mapping_helper_spec.rb. The file path is spec/features/review_mapping_helper_spec.rb.

require 'rspec'

describe 'Tests Review report' do
  before(:each) do
    create(:instructor)
    create(:assignment, course: nil, name: 'Test Assignment')
    assignment_id = Assignment.where(name: 'Test Assignment')[0].id
    login_as 'instructor6'
    visit "/reports/response_report?id=#{assignment_id}"
    click_on 'View'
  end

  it "can display review metrics" do
    expect(page).to have_content('Metrics')
  end

  it "can display review grades of each round" do
    expect(page).to have_content('Score awarded')
  end

  it "can display reviews done" do
    expect(page).to have_content('Reviews done')
  end

  it "can display Reviewer" do
    expect(page).to have_content('Reviewer')
  end

  it "can display team reviewed" do
    expect(page).to have_content('Team reviewed')
  end
end

RSpec.describe ReviewMappingHelper, :type => :helper do
  describe "test get_css_style_for_calibration_report" do
    it "should return correct css for calibration report" do
      expect(helper.get_css_style_for_calibration_report(1)).to eq('c4')
      expect(helper.get_css_style_for_calibration_report(0)).to eq('c5')
      expect(helper.get_css_style_for_calibration_report(2)).to eq('c3')
      expect(helper.get_css_style_for_calibration_report(3)).to eq('c2')
      expect(helper.get_css_style_for_calibration_report(4)).to eq('c1')
    end
  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. For our project Strategy Pattern was very helpful in adding functions to views as partials.

Screencast

To access the screencast for our project, please click Screencast

References

1) https://www.rubyguides.com/2015/12/ruby-refactoring/

2) http://rspec.info/documentation/3.8/rspec-core/

3) http://wiki.expertiza.ncsu.edu/index.php/Main_Page

Team Members

Ashish Kumar Jayantilal Jain

Aishwarya Tirumala

Devang Upadhyay