CSC/ECE 517 Fall 2015/oss E1569 JNR

E1569: Refactoring ReviewMappingController

This page provides a description of our Expertiza-based OSS project, which aimed at refactoring the ReviewMappingController.

Introduction

Expertiza is an Open Source software tool developed at NC State University. It is used to facilitate assignment and course management. It is primarily intended to facilitate assignments being peer reviewed. It is written as a Ruby on Rails application, thus functioning natively in a web environment. It can be cloned from GitHub.

Refactoring

Refactoring is a process designed to change code without modifying the functionality. Refactoring can improve the readability and the logical design of the software, making sure everything is in the right place and has the right name. This allows code to be understood more quickly by a developer, which shortens the time it takes to develop new features.

Development environment setup

<ref>http://wiki.expertiza.ncsu.edu/index.php/Main_Page</ref>

Problem statement

The ReviewMappingController file in this application (review_mapping_controller.rb) is responsible for setting up mappings between reviewers and reviewees. It essentially connects a reviewer to an assignment. However, it is a fairly bloated file since it has to handle functionality for five different kinds of maps (review response, author feedback, teammate review, meta-review, and quiz response). It is a prime choice for refactoring because the refactoring can clarify the intent of the different methods in the file.

According to the problem statement<ref>https://docs.google.com/document/d/1uWs3zyrupTmrOFuv5IbVWCF4NRvCXqJmg8dZ0wCqgus/edit</ref>, there were 11 tasks involved in refactoring the ReviewMappingController:

• Method delete_rofreviewer should do the same thing as delete_metareviewer
• Method delete_participant is not used
• Method list_sortable is not used
• Method automatic_review_mapping_strategy is too long
• Method review_report has SQL-like code
• Method add_user_to_assignment should not be in this controller
• Method get_team_from_submission should not be in this controller
• Method delete_all_reviewers doesn’t actually delete all the reviewers, but just the outstanding review response maps. We want to keep this functionality
• Method delete_participant should not be in this controller
• Method name release_reservation doesn’t describe the functionality well
• Method delete_review is not used

Design Approach

Refactoring

guidelines <ref>https://docs.google.com/document/d/1qQD7fcypFk77nq7Jx7ZNyCNpLyt1oXKaq5G-W7zkV3k/edit#</ref> Single Responsibility Principle <ref>https://en.wikipedia.org/wiki/Single_responsibility_principle</ref>

delete_rofreviewer

This function has duplicate functionality to another function, delete_metareviewer, and the other function is named much better. The method delete_rofreviewer was removed.

delete_participant

This function was confirmed to not being used, since there is no other function in the application that calls it. Therefore, it was removed.

list_sortable

This function was confirmed to not being used, since there is no other function in the application that calls it. Therefore, it was removed.

automatic_review_mapping_strategy

This function was originally too long. Hence, we modularised it by moving the part of the code that loops over all teams and creates the ReviewResponseMap into a new function.

response_report

To make this code easier to read for a pure Ruby developer, the SQL-like code was rewritten with ActiveRecord.

add_user_to_assignment

This method does not belong in this controller, and it is only ever called by participants_helper. Therefore, the method should go into participants_controller, which is where it was moved.

get_team_from_submission

This method is responsible for returning the team given an assignment submission. It did not have any direct correlation with the reviewer/reviewee. Hence, it was moved to the AssignmentTeam model (assignment_team.rb) file as suggested.

delete_all_reviewers

Since the only problem with this method is that the name does not describe the actual or intended functionality, we replaced all instances of “delete_all_reviewers” with “delete_outstanding_reviewers”.

delete_participant

This function was confirmed to not be used, since there is no other function in the application that calls it. Therefore, it was removed.

release_reservation

Since the method name does not accurately describe the functionality within, all instances of “release_reservation” were replaced with “release_mapping”.

delete_review

This function was confirmed to not be used, since there is no other function in the application that calls it. Therefore, it was removed.

Testing

Test Case Development using RSpec

Test Case 1: Check if on getting release mapping it redirects to student review controller

   it "should use release_mapping to redirect to student_review controller" do
       get :release_mapping, id: @assignment.id
       expect(response).should redirect_to(:controller => 'student_review', :action => 'list', :id=> '828')
   end

Test Case 2: Check if on delete outstanding reviewers it redirects to list mapping

   it "should delete outstanding reviewers to redirect to list_mappings" do
       get :delete_outstanding_reviewers, id: @assignment.id, contributor_id: 200
       expect(response).should redirect_to(:action => 'list_mappings', :id => 200)
   end

Test Case 3: Check if rendered report matches the required template

   it "should render appropritate response report" do
       get :response_report, id: 723
       response.should render_template(:response_report)
   end

Test Case 4: Check if rendered report matches the required template for a specific user

   it "should render appropriate response report for the specific user" do
      get :response_report,  id: 723, :user => {:fullname=> '523, student'}
      response.should render_template(:response_report)
   end

Test Case 5: Check if rendered report matches the required template for a specific user with type FeedbackResponseMap

   it "should render appropriate response report for the specific user with type FeedbackResponseMap" do
      get :response_report,  id: 723, :report => {:type=> 'FeedbackResponseMap'}
      response.should render_template(:response_report)
   end

Testing from the UI

View Response Report

View Response Report by name

View Feedback Response Map

Conclusion

By refactoring ReviewMappingController, extraneous and unused code was removed, readability was increased, and tests confirm that its functionality remained intact. Users will experience incrementally better performance, and developers will have an easier time getting up to speed on the software, resulting in heightened developer efficiency and faster turnaround times for new features.

Project Links

• Run Expertiza hosted on VCL
• GitHub repository used for development
• GitHub pull request to merge to expertiza/master

References

<references/>