CSC/ECE 517 Fall 2019 - E1942. Refactor stage deadlines in assignment.rb

Expertiza Background

Expertiza is an assignment portal developed by faculties and students at NCSU. It provides a platform for the faculties to create assignments for the students. Faculties can assign assignments with staged deadlines. Expertiza supports creation of teams for the students, tracking the team members and reviewing the work done by the other team members. Students can also review their peer team's assignment submission. Expertiza has been developed on Ruby on Rails and is available on github.

Project Description

Following is an OSS project which deals with refactoring of stage deadlines in the assignment.rb file. An assignment can have incremental deadlines for different topics in a single assignment. This project involves refactoring the functions in the assignment.rb file related to stage deadlines. There existed five functions to check the kind of stage an assignment is in. However, the names of these functions were ambiguous and functionalities implemented in some of them overlapped with each other. By the end of this project we have refactored these deadline functions.

Important Links

1. VCL Link for expertiza deployed with changes
2. OSS documentation

Current Scenario

The assignment.rb file has the following functions implemented :
1. current_stage_name(topic_id = nil)
2. find_current_stage(topic_id = nil)
3. get_current_stage(topic_id = nil)
4. link_for_current_stage(topic_id = nil)
5. stage_deadline(topic_id = nil)

Refractors implemented

The following is the list of refractors done in the code. It discusses the issues we found in the code with respect to Ruby conventions and the solutions that we provided for the same.

1.To avoid multiple calls to DueDate.get_next_due_date()

Problem with Existing code

In the existing code, DueDate.get_next_due_date(self.id, topic_id) has been called at numerous places. This does not follow the ruby coding standards.

Solution

New private method next_due_date(topic_id) has been added. This function returns the next_due_date by calling the get_next_due_date method on DueDate.

File: app/models/assignment.rb

Code snippet

def next_due_date(topic_id = nil)
 DueDate.get_next_due_date(self.id, topic_id)
end

Following places have been refactored.

Screenshots

2.To not have finished status checked with static string “Finished”

Problem with Existing code

The condition, if @assignment.current_stage_name(@topic_id) != 'Finished', have been used at various places to check finish status of assignment by comparing with “Finished”. This does not follow ruby coding standards, as we should never compare with static things. Otherwise it becomes difficult to refactor the code later.

Solution

Assignment is said to be finished if the next due is nil. A new private method finished? has been added. It calls the above next_due_date method and returns true if the next_due_date is nil.

File: app/models/assignment.rb

Code

def finished?( topic_id = nil )
 next_due_date(topic_id).nil?
end

The check with “Finished” has been replaces with a call to finished? . The code has been refactored at following places.

Screenshots

3.Return “Unknown” when topic_id is missing

Problem with existing code:

When it is staggered deadline and the topic_id is nil, “Unknown” is returned. This does not follow the DRY Principle. Never should we compare with static strings.

Solution

To make the code more readable and understandable, and to DRY out the code we have added a new private method topic_missing? to check if the topic is missing in case of staggered deadline.

File:app/models/assignment.rb

Code snippet

def topic_missing?( topic_id = nil)
 topic_id.nil? and self.staggered_deadline?
end

The code has been refactored at following places:

Screenshots

Topic_missing.PNG

4.find_current_stage method removed

Problem with the existing code:

The method find_current_stage has been used locally in assignment.rb and once in students controller. However internally, this method calls the next_due_date method for the assignment which has already been separated out.

Solution

The method find_current_stage has been removed. The places where it was called has been replaced by a call to next_due_date method.

File: app/models/assignment.rb , app/controllers/student_controller.rb

Code snippet

Before:
 due_date = find_current_stage(topic_id)

After:
 due_date = DueDate.get_next_due_date(self.id.topic_id)

The code has been refactored in assignments.rb and the students controller at following places:

Screenshots

5.link_for_current_stage method removed

Problem with existing code

The method link_for_current_stage was called once in list.html.erb. This method checks if the current assignment has any URL specified with it and would return it if present. However, the database has no such value. As a result, the if condition where the function was called always evaluated to false.

Solution

The function link_for_current_stage has been removed. The call to this function has also been removed and the code has been refactored accordingly.

File: app/models/assignment.rb , app/views/list.html.erb

The code in list.html.erb has been refactored as follows:

Screenshots

Automated Testing using RSPEC

Tests for get_current_stage()

The current stage of expertiza did not have any tests for get_current_stage. We have added the test for the same as follows:

File: spec/models/assignment_spec.rb

Code snippet

describe '#get_current_stage ' do
   context 'when topic id is nil and current assignment has staggered deadline' do
     it 'returns Unknown' do
       allow(assignment).to receive(:topic_missing?).and_return(true)
       expect(assignment.get_current_stage).to eq('Unknown')
     end
   end
   context 'when current assignment does not have staggered deadline' do
     context 'when due date is nil' do
       it 'returns Finished' do
         allow(assignment).to receive(:finished?).with(123).and_return(true)
         expect(assignment.get_current_stage (123)).to eq('Finished')
       end
     end
     context 'when due date is not nil and due date is not equal to Finished' do
       it 'returns current stage name' do
         allow(assignment).to receive(:finished?).with(123).and_return(false)
         allow(assignment).to receive(:topic_missing?).with(123).and_return(false)
         allow(assignment).to receive(:next_due_date).with(123).and_return(assignment_due_date)
         deadline = create(:deadline_type, id: 1, name:"Review")
         allow(DeadlineType).to receive(:find).with(1).and_return(deadline)
         expect(assignment.get_current_stage(123)).to eq('Review')
       end
     end
   end
end

Tests for finished?

Tests for new finished? function have been added.
File: spec/models/assignment_spec.rb

Code snippet

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
 end
 describe '#finished?' do
   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

topic_missing method is a private method and hence no tests have been added for the same.

Team

Omkar Kashid
Deeksha Rao
Swathi Dinakaran