CSC/ECE 517 Fall 2015/ossE1558BGJ

Introduction

This page provides a description of the modifications and improvements made to the Expertiza project’s source code as part of an Expertiza based Open Source Software project. Expertiza is a web based application which allows students to submit and peer-review classmates’ work, including written articles and development projects. The specific changes made during the course of this project were to the ReviewResponseMap.rb file, with additional changes made to other related files in support of refactoring ReviewResponseMap.rb.

Problem Statement

ReviewResponseMap is the model class used to manage the relationship between contributors, reviewers, and assignments. The intent of the changes were to refactor the code for better readability and adherence to Ruby coding standards and best practices. Primarily these changes involved the refactoring of overly complex methods, renaming methods for better readability, and the addition of missing unit tests.

Project Requirements:

1. Code Climate<ref name="Code Climate">https://codeclimate.com</ref> shows import method is complex, because of lots of checks. This method can be fixed by adding private methods for raising import error.
2. Get_assessments_round_for method can be renamed to get_team_responses_for_round. Team_id private variable is not needed.
3. metareview_response_maps rename, refactoring can be done. No need to do second iteration.
4. write missing unit tests for existing methods.

Design Patterns

As part of our project, we refactored the model to follow the Single Responsibility and Don't Repeat Yourself (DRY) Principles. Both of the principles focus on reducing code repetition and increasing the cohesion of the individual methods in the class. The Single Responsibility Principle states that each class and method should have sole responsibility over one single part of the functionality of the system<ref>https://en.wikipedia.org/wiki/Single_responsibility_principle</ref>. The DRY principle states that whenever the same types of logic and functionality are being performed in a class the duplicated logic should be extracted into a new method that can be called in place of the repeated lines of code <ref>https://en.wikipedia.org/wiki/Don%27t_repeat_yourself</ref>. A prime example of a method that initially violated these principles was the import method, which in addition to performing the core import processes also performed a number of checks that would be more properly extracted into private methods.

Code Changes

Refactoring of import method

CodeClimate reports showed the import method to be overly complex, with a number of checks being included within the import method itself. The resolution for this problem was to refactor the import method, creating private methods to perform the null checks and throw import errors. Prior to refactoring, all null checks were performed within the import method. After refactoring, the import method adheres to the single responsibility principle, performing only key import tasks while making calls to private methods for any necessary validation. The size of the import method was reduced by doing this and counts for better readability of the code. Also, this method is a very important one and care was taken so that any existing functionality did not break.

Before refactoring:

def self.import(row, session, id)
   if row.length < 2
     raise ArgumentError, "Not enough items"
   end
   assignment = Assignment.find(id)
   if assignment.nil?
     raise ImportError, "The assignment with id \"#{id}\" was not found. <a href='/assignment/new'>Create</a> this assignment?"
   end
   index = 1
   while index < row.length
     user = User.find_by_name(row[index].to_s.strip)
     if user.nil?
       raise ImportError, "The user account for the reviewer \"#{row[index]}\" was not found. <a href='/users/new'>Create</a> this user?"
     end
     reviewer = AssignmentParticipant.where(user_id: user.id, parent_id:  assignment.id).first
     if reviewer == nil
       raise ImportError, "The reviewer \"#{row[index]}\" is not a participant in this assignment. <a href='/users/new'>Register</a> this user as a participant?"
     end
     if assignment.team_assignment
       reviewee = AssignmentTeam.where(name: row[0].to_s.strip, parent_id:  assignment.id).first
       if reviewee == nil
         raise ImportError, "The author \"#{row[0].to_s.strip}\" was not found. <a href='/users/new'>Create</a> this user?"
       end
       existing = ReviewResponseMap.where(reviewee_id: reviewee.id, reviewer_id:  reviewer.id).first
       if existing.nil?
         ReviewResponseMap.create(:reviewer_id => reviewer.id, :reviewee_id => reviewee.id, :reviewed_object_id => assignment.id)
       end
     else
       puser = User.find_by_name(row[0].to_s.strip)
       if user == nil
         raise ImportError, "The user account for the reviewee \"#{row[0]}\" was not found. <a href='/users/new'>Create</a> this user?"
       end
       reviewee = AssignmentParticipant.where(user_id: puser.id, parent_id:  assignment.id).first
       if reviewee == nil
         raise ImportError, "The author \"#{row[0].to_s.strip}\" was not found. <a href='/users/new'>Create</a> this user?"
       end
       team_id = TeamsUser.team_id(reviewee.parent_id, reviewee.user_id)
       existing = ReviewResponseMap.where(reviewee_id: team_id, reviewer_id:  reviewer.id).first
       if existing.nil?
         ReviewResponseMap.create(:reviewee_id => team_id, :reviewer_id => reviewer.id, :reviewed_object_id => assignment.id)
       end
     end
     index += 1
   end
 end

After refactoring:

 #Imports new reponse maps if they do not already exist
  def self.import(row, _session, id)
      if row.length < 2
          raise ArgumentError, 'Not enough items'
      end
      assignment = find_assignment(id)
      index = 1
      reviewee_name = row[0]
      while index < row.length
          reviewee_id = nil
          reviewer_name = row[index]
          reviewer = get_assignment_participant(reviewer_name,  assignment.id, "reviewer")
          participant_nil?(reviewer, reviewer_name , "reviewer")
         
         #Find reviewee if assignment is a team assignment
         if assignment.team_assignment
             reviewee = AssignmentTeam.where(name: reviewee_name .to_s.strip, parent_id: assignment.id).first
             participant_nil?(reviewee, reviewee_name, "author")
             reviewee_id = reviewee.id
         #Find reviewee if assignment is not a team assignment
         else
	     reviewee = get_assignment_participant(reviewee_name, assignment.id, "reviewee")
             participant_nil?(reviewee, reviewee_name, "author")
             reviewee_id  = TeamsUser.team_id(reviewee.parent_id, reviewee.user_id)
         end
         create_response_map(reviewer, reviewee_id,  assignment)
         index += 1
      end
  end

Private methods added:

 # Check for if assignment value is null
 def self.find_assignment(id)
     begin
         assignment = Assignment.find(id)
     rescue ActiveRecord::RecordNotFound
         raise ImportError, "The assignment with id \"#{id}\" was not found.<a href='/assignment/new'>Create</a> this assignment?"
     end
 end

 # Check for if participant is null
 def self.participant_nil?(participant, user_name, participant_type) 
     error_message = nil
     if participant_type == "author"
         error_message = "The author \"#{user_name.to_s.strip}\" was not found.<a href='/users/new'>Create</a> this user?"
     else
         error_message =  "The reviewer \"#{user_name}\" is not a participant in this assignment.<a href='/users/new'>Register</a> this user as a participant?"
     end
     check_nil?(participant, error_message)
 end

 # Check for if user is null
 def self.user_nil?(user, user_name, user_type)
     check_nil?( user, "The user account for the \"#{user_type}\" \"#{user_name}\" was not found.<a href='/users/new'>Create</a> this user?")
 end

#Throws import error for nil objects
 def self.check_nil?(object, error_message)
     if object.nil?
         raise ImportError, error_message
     end
 end

Refactoring of get_assessments_round_for method

The get_assessments_round_for method’s name failed to provide a clear idea of the method’s purpose and functionality. This is why it was renamed to get_team_responses_for_round, the new name of the method provides a clearer idea of its purpose. Additionally, there was a private variable, team_id, introduced in the method that was unnecessary and could be replaced by using the id property of the team parameter directly. We also removed the return statement at the end of the method.

Before refactoring:

 def self.get_assessments_round_for(team,round)
   team_id =team.id
   responses = Array.new
   if team_id
     maps = ResponseMap.where(:reviewee_id => team_id, :type => "ReviewResponseMap")
     maps.each{ |map|
       if !map.response.empty? && !map.response.reject{|r| r.round!=round}.empty?
         responses << map.response.reject{|r| r.round!=round}.last
       end
     }
     responses.sort! {|a,b| a.map.reviewer.fullname <=> b.map.reviewer.fullname }
   end
   return responses
 end

After refactoring:

 # return the responses for specified round,
 # for varying rubric feature -Yang
 def self.get_team_responses_for_round(team, round)
   responses = []
   if team.id
     maps = ResponseMap.where(reviewee_id: team.id, type: 'ReviewResponseMap')
     maps.each do |map|
       unless map.response.empty? && map.response.reject { |r| r.round != round }.empty?
         responses << map.response.reject { |r| r.round != round }.last
       end
     end
     responses.sort! { |a, b| a.map.reviewer.fullname <=> b.map.reviewer.fullname }
   end
   responses
 end

The change in method name also required changes to the following files that referenced it:

/app/models/assignment.rb:436:

 assessments = ReviewResponseMap.get_assessments_round_for(team,i)

./app/models/assignment_participant.rb:268:

scores[questionnaire_symbol][:assessments] = questionnaire.get_assessments_round_for(self,round)

Refactoring of metareview_response_maps method

The metareview_response_maps method was unnecessarily complex. It contained an unneeded private variable and an unnecessary nested loop. Originally, it added each metareview map individually in the nested loop. We decided to remove the nested loop and use the concat method for readability and maintainability. These changes allowed us to remove the private variable "metareview_list" and only maintain the "metareview_response_maps" variable. We also renamed the metareview_response_maps method to get_metareview_response_maps method to be more descriptive about the intent of the method.

Before refactoring:

def metareview_response_maps
   responses = Response.where(map_id:self.id)
   metareview_list=[]
   responses.each do |response|
     metareview_response_maps = MetareviewResponseMap.where(reviewed_object_id:response.id)
     metareview_response_maps.each do |metareview_response_map|
       metareview_list<<metareview_response_map
     end
   end
   metareview_list
end

After refactoring:

 # Returns the response maps for all the metareviews
 def get_metareview_response_maps
   responses = Response.where(map_id: id)
   metareview_response_maps = []
   responses.each do |response|
     metareview_response_maps.concat MetareviewResponseMap.where(reviewed_object_id: response.id)
   end
   metareview_response_maps
 end

The change from this renaming also requires changes in the following files: app/models/assignment.rb:361:

response_map_set.sort! { |a, b| a.metareview_response_maps.count <=> b.metareview_response_maps.count }

app/models/assignment.rb:362:

min_metareviews = response_map_set.first.metareview_response_maps.count

app/ models/assignment.rb:363:

response_map_set.reject! { |response_map| response_map.metareview_response_maps.count > min_metareviews }

app/models/assignment.rb:377:

response_map_set.sort! { |a, b| a.metareview_response_maps.count <=> b.metareview_response_maps.count }

app/models/assignment.rb:378:

min_metareviews = response_map_set.first.metareview_response_maps.count

app/models/assignment.rb:379:

response_map_set.sort! { |a, b| a.metareview_response_maps.last.id <=> b.metareview_response_maps.last.id } if min_metareviews > 0

app/models/assignment_participant.rb:73:

self.response_maps.metareview_response_maps.each do |metaresponse_map|

app/models/response.rb:4:

has_many :metareview_response_maps, :class_name => 'MetareviewResponseMap', :foreign_key => 'reviewed_object_id', dependent: :destroy

Refactoring of export method

The export method contained mappings.sort! { |a, b| a.reviewee.name <=> b.reviewee.name }. We could not get past this when running tests, we kept getting a message saying, "sort! was not a method for an ActiveRecord_Relation". But, ActiveRecord_Relation does allow the use of sort. So we changed that line of code to mappings = mappings.sort { |a, b| a.reviewee.name <=> b.reviewee.name }

Before refactoring:

 def self.export(csv, parent_id, options)
   mappings = where(reviewed_object_id: parent_id)
   mappings.sort! { |a, b| a.reviewee.name <=> b.reviewee.name }
   mappings.each {
     |map|
     csv << [
       map.reviewee.name,
       map.reviewer.name
     ]
   }
 end

After refactoring:

 # options parameter used as a signature in other models
 def self.export(csv, parent_id, options)
   mappings = where(reviewed_object_id: parent_id)
   mappings = mappings.sort { |a, b| a.reviewee.name <=> b.reviewee.name }
   mappings.each do
   |map|
     csv << [map.reviewee.name, map.reviewer.name]
   end
 end

Refactoring of final_versions_from_reviewer method

The final_versions_from_reviewer method was too long and repetitive. We removed common functionality from within the if and else statement and put it in a private method final_versions_from_reviewer. Then we created the get_responses method to find the responses based on the arguments values. These private methods make the class more readable for other programmers.

Before refactoring:

 def self.final_versions_from_reviewer(reviewer_id)
   maps = ReviewResponseMap.where(reviewer_id: reviewer_id)
   assignment = Assignment.find(Participant.find(reviewer_id).parent_id)
   review_final_versions = Hash.new
   if !assignment.varying_rubrics_by_round?
     #same review rubric used in multiple rounds
     review_final_versions[:review] = Hash.new
     review_final_versions[:review][:questionnaire_id] = assignment.get_review_questionnaire_id
     response_ids = Array.new
     maps.each do |map|
       responses = Response.where(map_id:map.id)
       if !responses.empty?
         response_ids << responses.last.id
       end
     end
     review_final_versions[:review][:response_ids] = response_ids
   else
     #vary rubric by round
     rounds_num = assignment.rounds_of_reviews
     for round in 1..rounds_num
       symbol = ("review round"+round.to_s).to_sym
       review_final_versions[symbol] = Hash.new
       review_final_versions[symbol][:questionnaire_id] = assignment.get_review_questionnaire_id(round)
       response_ids = Array.new
       maps.each do |map|
         responses = Response.where(map_id:map.id, round:round)
         if !responses.empty?
           response_ids << responses.last.id
         end
       end
       review_final_versions[symbol][:response_ids] = response_ids
     end
   end
   review_final_versions
 end

After refactoring:

 # wrap The latest version of responses in each response map,
 # together with the questionnaire_id will be used to
 # display the reviewer summary
 def self.final_versions_from_reviewer(reviewer_id)
   maps = ReviewResponseMap.where(reviewer_id: reviewer_id)
   assignment = find_assignment(Participant.find(reviewer_id).parent_id)
   review_final_versions = {}
   unless assignment.varying_rubrics_by_round?
     #same review rubric used in multiple rounds
     review_final_versions = review_final_version_responses(:review, :questionnaire_id, assignment, maps)
   else
     # vary rubric by round
     rounds_num = assignment.rounds_of_reviews
     (1..rounds_num).each do |round|
       symbol = ('review round' + round.to_s).to_sym
       review_final_versions = review_final_version_responses(symbol, :questionnaire_id, assignment, maps, round)
     end
   end
   review_final_versions
 end

Private methods added:

 # Compute list of responses and return it
 def self.review_final_version_responses(symbol, questionnaire_id, assignment, maps, round = nil)
   review_final_versions = {}
   review_final_versions[symbol] = {}
   review_final_versions[symbol][questionnaire_id] = assignment.get_review_questionnaire_id(round)
   response_ids = []
   maps.each do |map|
     responses =  get_responses(map.id, round)
     unless responses.empty?
       response_ids << responses.last.id
     end
   end
   review_final_versions[symbol][:response_ids] = response_ids
 end

 #Get responses by map_id and round (if exists)
 def self.get_responses(id, round)
    if round.nil?
      responses = Response.where(map_id: id)
    else
      responses = Response.where(map_id: id, round: round)
    end	
 end

Refactoring of other methods

We removed the return statement from the get_title, export_fields, and show_feedback methods because return statements are unnecessary in Ruby.

Addition of code comments

We added comments to all of the methods in the ReviewResponseMap.rb file and corrected instances where CodeClimate Identified opportunities for code improvement. One issue flagged by CodeClimate that we did not change was to use the find_by method instead of where().first method. The where().first method occurs in the import, get_assignment_participant, and create_response_map methods. It is not always appropriate to use the find_by method, as documented in this github post: https://github.com/bbatsov/rubocop/issues/1938 . In instances where ordering by id is the desired behavior, where().first will order by default, but in Rails 4+ find_by does not honor that default behavior. For this reason we left the instances of where().first unchanged.

Code Improvements

This refactoring was to improve readability and follow as many good Rubyist coding practices as possible without breaking the functionality of existing code. We used CodeClimate of the master branch code as a reference. We took the suggestions we got from CodeClimate and improved our code based on that. We also made some of the methods shorter by removing redundant functionality and adding private methods to methods with a lot internal functionality. We also added private methods in places where we saw repeated patterns to make the code DRYer. At the end of our refactoring, we managed to improve the rating on CodeClimate from a C to a B . Further improvements meant reducing the size of the entire class which required a much more significant refactoring in multiple files and folders. We focused on refactoring the existing model, ReviewResponseMap.

Unit Tests

The previous unit tests had syntactical and run time errors. We were not able to get the previous unit tests to run without errors. Because of this we rewrote all of the unit tests for the ReviewResponseModel. It is important to have working unit tests so that when internal changes are made to functions the external output remains the same.

Refactoring of Unit Tests

We added a test_helper file to the main test folder that requires a needed configuration file and needed gems. We also added 8 fixtures that were needed for data to run the unit tests. The following fixture files were added:

• assignment_questionnaires.yml
• assignments.yml
• participants.yml
• questionnaires.yml
• response_maps.yml
• responses.yml
• teams.yml
• users.yml

There is at least one unit test for every public method within the ReviewResponseModel. The test naming convention used is "method_<name of method that is being tested>". For example, the unit test for the export method is "method_export". If there are multiple test for one method the name of the test will include the test case after the normal naming convention. For example, one of the unit tests for the import method checks for an invalid assignment. The name of this method is "method_import_invalid_assignment". The following are the unit tests we created:

• method_export
• method_get_metareview_response_maps
• method_get_team_response_for_round
• method_final_versions_from_reviewer
• method_import
• method_import_invalid_reviewee
• method_import_invalid_reviewer
• method_import_invalid_assignment
• method_delete
• method_delete_no _force
• method_show_feedback
• method_get_title
• method_export_fields
• method_questionnaire
• method_add_reviewer

How to Run Unit Tests

To run the unit tests, follow these steps:

1. Download the master branch of the repo.
2. Setup the Databases for the test environment (we have used Zhewei's scrubbed expertiza DB )
   • Run the " rake db:create RAILS_ENV=test " command
   • Run the " rake db:reset RAILS_ENV=test " command
   • Scrub the DB using " mysql -u root expertiza_development < expertiza-scrubbed.sql"
   • Run the " rake db:migrate "
3. Run " rake test test/unit/review_response_map_test.rb " in the "expertiza/" directory.
   • If you run into a mysql log in error go into config/database.yml and edit the mysql credentials for the test section so that it matches the local environment's mysql configuration
4. Check if tests passed or failed.

Manual Test Case

In order to test the changes made to the ReviewResponseMap, bring up [[ http://152.46.16.195:5902/ | Expertiza]] and log in as an administrator. Once logged in, proceed with the following steps:

1. Go to impersonate user when logged in as instructor6 and type in student559 (or) just log in as student559. All passwords are password .
2. Pick any assignment to view.
3. Navigate to Other's work.
4. Check the different reviews and metareviews. Whether they are being displayed correctly or not.
5. Go to Edit or Give feedback and submit a feedback.
6. Go back and check if the feedback given is displayed correctly or not.
7. Check if author's feedback is correct or not.
8. Go to the Temp assignment and see if the page is being displayed correctly or not (since there is no assignment submission content or reviews for that).

NOTE: Since this was a refactoring of the code and methods, the existing functionality was supposed to remain the same and not break for the changes we've made.

References

<references/>

External Links

1. Expertiza Main Repo
   
2. Refactored ReviewResponseMap.rb
   
3. Test Fixtures
   
4. Unit Tests
   
5. Expertiza Deployed Site