CSC/ECE 517 Fall 2019 - E1942. Refactor stage deadlines in assignment.rb: Difference between revisions
Line 114: | Line 114: | ||
<h4>Screenshots</h4> | <h4>Screenshots</h4> | ||
[[File:duedate_lm.png]] | |||
== '''Automated Testing using RSPEC''' == | == '''Automated Testing using RSPEC''' == |
Revision as of 03:07, 7 November 2019
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 above methods 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
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.