CSC/ECE 517 Spring 2020 Refactor assignment.rb

Abstract

Expertiza is a web application using Ruby on Rails framework. The creation and maintenance of this application is handled by NCSU students and faculty. The basic functionalities allow the instructor to create and edit, both new and existing assignments. Also it allows the publishing of surveys and peer reviews, while allowing students the opportunity to sign up for topics, teams, submit assignments and peer reviews.

Problem Statement

Assignment.rb is a large file, that has dozens of methods and fields. Some methods seem redundant, and some fields and string literals are repeated multiple times throughout the file. Ie. 'Finished'. Each method in the file is essential and provides different data, so the goal for my team was to rename and merge code where we could, while not affecting the end functionality.

Problem Solution

Due to the fragile nature of the methods in Assignment.rb, our team decided to focus on redundancy in the code and anywhere we could merge redundant code into common methods. Also there were quite a few strings that were used constantly in the file, ie. 'Finished' that we decided to make constants, to reduce complexity if the values ever changed.

Refactors

- One of the main issues I saw in the code was the reuse of string literals in comparisons. These would cause hige efforts to change later if there are hundreds of places where these strings are used. Instead I replaced all instances of these strings in the file with constants, to lower complexity.

Added these constants to file:

 # Constants for common fields, to reduce complexity if change is needed
 FINISHED_CONST = 'Finished'.freeze
 UNKNOWN_CONST = 'Unknown'.freeze

Removed references to string literals and made constant values

 due_date == FINISHED_CONST
 return (topic_id.nil? ? UNKNOWN_CONST : get_current_stage(topic_id))

- The next issue I found was in multiple places where the code was making the same boolean checks. Instead rewtiting tge checks each time I made a method to return True/False for the check.

# Function to check if topic id is null when its a staggered assignment, to prevent redundancy
 def topic_missing?(topic_id = nil)
   topic_id.nil? and self.staggered_deadline?
 end

Testing

For testing, assignment.rb was already tested pretty well. According to Code Climate it had a B grade for test coverage. Thus the testing effort for this project was not heavy, but we did want to be sure that none of our changes broke any existing tests, and all of our additional methods were tested. For example, one new method we added was:

# New function to check if the assignment is finished
 def finished?(topic_id = nil)
   next_due_date(topic_id).nil?
 end

For this method we added a few tests to validate the method was returning the values we expect.

 describe '#finished?' do
   context 'when assignment next due date is nil' do
     it 'returns True' do
       allow(DueDate).to receive(:get_next_due_date).with(1, 123).and_return(nil)
       expect(assignment.finished?(123)).to eq(true)
     end
   end

   context 'when there is a next due date' do
     it 'returns False' do
       allow(DueDate).to receive(:get_next_due_date).with(1, 123).and_return('2021-11-11 11:11:11')
       expect(assignment.finished?(123)).to eq(false)
     end
   end
 end

Also there was a method response_map_to_metareview which was not being tested in case of an error, so I added a test case to be sure the exception was raised.