CSC/ECE 517 Fall 2022 - E2262: Refactor review mapping helper.rb

E2262. Further refactoring and improvement of review_mapping_helper

This page provides a description of the Expertiza based OSS project.

About Expertiza

Expertiza is an open source project based on Ruby on Rails framework. Expertiza is an all inclusive web service that allows instructors to create, edit and manage assignments. This includes the functionality for topic selection and team formation across various projects and assignments. Expertiza allows for a variety of submission types including URL and PDF. Expertiza also allows the instructor to assign peer reviews and team assessments.

Team

Mentor

Jialin Cui (jcui9 ncsu.edu)

Team Members

Gun Ju Im (gim@ncsu.edu)
Rithik Jain (rjain25@ncsu.edu)
Rohan Shiveshwarkar (rsshives@ncsu.edu)

Description of Project

This wiki describes the work done in refactoring the app/helpers/review_mapping_helper.rb and associated files calling the methods contained in it. The main goal of the tasks was to make the code clearer and more understandable to the reader while retaining the functionality. The goal included inserting comments wherever necessary.

File Description

The review_mapping_helper.rb has methods that help return charts and other graphical data to the frontend

What is Needed

This project builds on E2225, and should begin with the refactoring done by that project. That project focused on simplifying the methods in review_mapping_helper.rb, while this project looks at making the code more understandable and transparent.

Files Modified

app/helpers/review_mapping_helper.rb
app/views/reports/_calibration_report.html.erb
app/views/reports/_feedback_report.html.erb
app/views/reports/_review_report.html.erb
app/views/reports/_team_score.html.erb
app/views/reports/_team_score_score_awarded.html.erb
spec/features/review_mapping_helper_spec.rb
spec/helpers/review_mapping_helper_spec.rb

Issues to be Addressed

method get_data_for_review_report marshals a lot of data together and passes back a data structure. It is used in views/reports/_review_report.html.erb, but it is quite difficult to see how the data is displayed. It violates the Expert pattern because the view needs to know how to break apart the structure that is passed to it. Doing the same thing with partials in views/reports would make the code easier to follow
The next three methods involve team “color”. Color coding is explained on this page and this page for the E1789 project and this page for E1815. The code for these methods is not at all clear and should be refactored. And please use the American spelling “color”.
Various method names begin with get. This is not Ruby-like. Change the names to something more appropriate.
get_awarded_review_score computes an overall score based on scores awarded in individual rounds. This is one of many places in Expertiza where scores are being calculated. Score-calculation code for multiple rounds is being standardized now, in response_map.rb. Contact Nicholas Himes (nnhimes@ncsu.edu) for particulars. Change this method to use the new code.
The method sort_reviewer_by_review_volume_desc should be generalized so that it can sort by any metric, not just review volume. Other metrics might include number of suggestions or number of suggestions + number of problems detected. This method should not be counting the number of review rounds! Since other places in the code will need to know the number of review rounds, it should be calculated somewhere else in the system.
The next several methods generate charts. They are cohesive enough that they should be in their own separate file, either another helper or a mixin.
Then there is a method list_review_submissions. It’s not at all clear that this method is needed, though it is used in one view. Look on the Expertiza wiki and see if there is a better way.
There are several methods for feedback_response_maps. It is not at all clear why they are here. Look on the Expertiza wiki for the documentation, and see if you can replace them by calls to other methods, or at least make it clearer what they are doing.
The file ends with three small classes being defined. There are no comments at all to explain what is being done. Look them up on the Expertiza wiki and refactor or comment them, whichever seems more appropriate.

Problems Tackled

Issue #1: <Insert>

Methods involved: <Insert>
Initial Code: <Insert>
Problem: <Insert>
Solution: <Insert>
Contributor: <Insert>

Issue #2: The code was not clear to the reader

Methods involved: team_color(), obtain_team_color(), check_submission_state()
Initial Code: The method obtain_team_color was calling the method check_submission_state() and passing 'color' array. The method check_submission_state() was checking multiple parameters and was pushing certain colors into the array. The method obtain_team_color() was then returning the last element of the array to the method team_color()
Problem: The color array being passed around was unclear and was an unrequired data structure.
Code was as follows

def 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 color 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

  # loops through the number of assignment review rounds and obtains the team color
  def obtain_team_color(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, color)
    end
    color[-1]
  end

  # checks the submission state within each round and assigns team color
  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

Solution: The color was changed to a variable, and on each check in the method check_submission_state(), the color variable is updated.
Updated Code is as follows:

def team_color(response_map)
    color = 'red'
    # 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 color based on conditions
    if Response.exists?(map_id: response_map.id)
      if !response_map.try(:reviewer).try(:review_grade).nil?
        color = 'brown'
      elsif response_for_each_round?(response_map)
        color = 'blue'
      else
        color = obtain_team_color(response_map, assignment_created, assignment_due_dates)
      end
      color #returning the value of color
    end
  end

  # loops through the number of assignment review rounds and obtains the team color
  def obtain_team_color(response_map, assignment_created, assignment_due_dates)
    color = 'red' # assigning the color default to red
    (1..@assignment.num_review_rounds).each do |round|
      check_submission_state(response_map, assignment_created, assignment_due_dates, round, color)
    end
    color
  end

  # checks the submission state within each round and assigns team color
  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 = '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 = 'green'
      else
        link_updated_at = get_link_updated_at(link)
        color = link_updated_since_last?(round, assignment_due_dates, link_updated_at) ? 'purple' : 'green'
      end
    end
  end

Contributor: Rohan Shiveshwarkar

Issue #3: Various method names are not Ruby-like.

Methods involved: get_data_for_review_report(), get_team_color(), get_link_updated_at(), get_team_reviewed_link_name(), get_awarded_review_score(), get_each_review_and_feedback_response_map(), get_certain_review_and_feedback_response_map(), get_css_style_for_calibration_report()
Initial Code: Same as methods involved above
Problem: According to Ruby Style Guide, we should avoid prefixing method names with get_.
Solution: I got rid of get_ on every method name which begins with get_. I also searched same methods which were used in other files, and also changed their names.
Contributor: Gunju Im

Issue #4: <Insert>

Methods involved: <Insert>
Initial Code: <Insert>
Problem: <Insert>
Solution: <Insert>
Contributor: <Insert>

Issue #5: Generalizing the method sort_reviewer_by_review_volume_desc()

Methods involved: sort_reviewer_by_review_volume_desc()
Initial Code: The method was only taking into account the metric: review volume
Problem: The Response has several other metrics to be considered including number of suggestions, or number of suggestions + number of problems detected
Code was as follows
review_mapping_helper.rb

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

_review_report.html.erb

<!-- Set the review data and row span for each reviewer -->
(Line 61) <% sort_reviewer_by_review_volume_desc.each_with_index do |reviewer, index| %>

Solution: Created a generalized method sort_reviewer_by_review_metric_desc() which takes in a string parameter specifying the metric to be considered (eg. "review_volume") and edited the function call in _review_report.html.erb file and in spec/helpers/review_mapping_helper_spec.rb file
Code is as follows:
review_mapping_helper.rb

  # Generalized function which takes in string parameter specifying the metric to be sorted
  def sort_reviewer_by_review_metric_desc(metric) #metric is the string value of the metric to be sorted with
    if (metric.eql?("review_volume"))
      @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
  end

_review_report.html.erb

<!-- Set the review data and row span for each reviewer -->
(Line 61) <% sort_reviewer_by_review_metric_desc("review_volume").each_with_index do |reviewer, index| %>

Contributor: Rohan Shiveshwarkar

Issue #6: Refactor charts generating methods into another file as a helper/mixin

Methods involved: initialize_chart_elements(), display_volume_metric_chart(), display_tagging_interval_chart(), calculate_key_chart_information()
Initial Code: The methods above were included in the review_mapping_helper.rb file
Problem: They are cohesive enough that they should be in their own separate file, either another helper or a mixin
Solution: Created a new file chart_generator_helper.rb and created a new module ChartGeneratorHelper and shifted all the functions into that helper, and included it into the module ReviewMappingHelper in the review_mapping_helper.rb file
The new code is organized as below

chart_generator_helper.rb

module ChartGeneratorHelper 
    def initialize_chart_elements()
    def display_volume_metric_chart()
    def display_tagging_interval_chart()
    def calculate_key_chart_information()

review_mapping_helper.rb

module ReviewMappingHelper 
    include ChartGeneratorHelper 
    ...
    ...
    ...
    ...

Contributor: Rohan Shiveshwarkar

Issue #7: A method list_review_submissions is not clear that it is needed.

Methods involved: list_review_submissions
Initial Code: 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

Problem: I can’t find its usage even though it is used in a view file; app/views/reports/_review_report.html.erb.
Solution: I deleted the method in files; review_mapping_helper.rb and _review_report.html.erb.

Contributor: Gunju Im

Issue #8: <Insert>

Methods involved: <Insert>
Initial Code: <Insert>
Problem: <Insert>
Solution: <Insert>
Contributor: <Insert>

Issue #9: Three classes which locate in the last part of file have no comments to explain what is being done.

Methods involved: initialize(), reviews_per_team, reviews_needed, reviews_per_student
Initial Code: class ReviewStrategy

   attr_accessor :participants, :teams

   def initialize(participants, teams, review_num)
     @participants = participants
     @teams = teams
     @review_num = review_num
   end
 end

 class StudentReviewStrategy < ReviewStrategy
   def reviews_per_team
     (@participants.size * @review_num * 1.0 / @teams.size).round
   end

   def reviews_needed
     @participants.size * @review_num
   end

   def reviews_per_student
     @review_num
   end
 end

 class TeamReviewStrategy < ReviewStrategy
   def reviews_per_team
     @review_num
   end

   def reviews_needed
     @teams.size * @review_num
   end

   def reviews_per_student
     (@teams.size * @review_num * 1.0 / @participants.size).round
   end
 end

Problem: Three classes are unclear.
Solution: They are only used in this file. Also, they have methods that have same names with the other classes even if they are included in other class. At first, I tried to know what these classes meant and whether there was good way to refactor their method names. However, in conclusion, they are useless in the file, so I deleted them.
Contributor: Gunju Im

Peer Review Information

For viewing the deployed project (on VCL) please click on this link -> http://152.7.177.222:8080/
And use the following credentials: Username = instructor6 | Password = password

Test Cases

Rspec

This is a refactoring project, so it is expected that the original functionality should be retained. All the test cases passed which implies that the refactored code did not break the existing functionality.
Proof of test cases being passed:

Github Links

CSC/ECE 517 Fall 2022 - E2262: Refactor review mapping helper.rb

E2262. Further refactoring and improvement of review_mapping_helper

Contents

About Expertiza

Team

Mentor

Team Members

Description of Project

File Description

What is Needed

Files Modified

Issues to be Addressed

Problems Tackled

Peer Review Information

Test Cases

Rspec

Github Links

Link to Expertiza Repository

Link to Forked Repository

Link to Pull Request

Navigation menu

CSC/ECE 517 Fall 2022 - E2262: Refactor review mapping helper.rb

E2262. Further refactoring and improvement of review_mapping_helper

About Expertiza

Team

Mentor

Team Members

Description of Project

File Description

What is Needed

Files Modified

Issues to be Addressed

Problems Tackled

Peer Review Information

Test Cases

Rspec

Github Links

Link to Expertiza Repository

Link to Forked Repository

Link to Pull Request

Navigation menu

Search