CSC/ECE 517 Fall 2017/E1769 Refactor assignment form.rb: Difference between revisions
No edit summary |
|||
(38 intermediate revisions by 3 users not shown) | |||
Line 3: | Line 3: | ||
==Expertiza Background == | ==Expertiza Background == | ||
Expertiza is a web application where students can submit and peer-review learning objects (articles, codes, websites, etc). Instructors add and edit assignments to Expertiza. Students can be assigned in teams based on their selection of the topics. | Expertiza is a web application where students can submit and peer-review learning objects (articles, codes, websites, etc). Instructors add and edit assignments to Expertiza. Students can be assigned in teams based on their selection of the topics. | ||
== Problem Statement == | == Problem Statement == | ||
The task of the project is to refactor | The task of the project is to refactor assignment_form.rb and write unit tests for it. assignment_form.rb is a fairly complex file. It contains a lot of methods that are long and hard to understand. These methods need to be broken down into simpler and more specific methods that are easier to read/understand. Also, the few instances of code duplication that exist should be removed. | ||
* Refactor `add_to_delayed_queue` method | |||
* Rename method `is_calibrated` to `calibrated?` and all other places it is used | |||
* Use `find_by` instead of `where.first` | |||
* Complete pending tests in sign_up_sheet_controller.rb | |||
== Modified File == | == Modified File == | ||
* Refactor the assignment.rb | |||
* Refactor the assignment_form.rb | * Refactor the assignment_form.rb | ||
* Complete relating | * Complete relating tests in assignment_form_rspec.rb and assignment.rb | ||
* Complete | * Complete tests in sign_up_sheet_controller_rspec.rb | ||
== Refactor == | |||
=== Refactor `add_to_delayed_queue` method === | |||
`add_to_delayed_queue` is a long method with some duplicate codes and hard to understand. In order to encourage code reuse and make it easier to understand semantics of the function, we split it into three methods, name the added two as `get_diff_from_now` and `add_delayed_job`. The changes are displayed below, | |||
Original Code: | |||
# Adds items to delayed_jobs queue for this assignment | |||
def add_to_delayed_queue | |||
duedates = AssignmentDueDate.where(parent_id: @assignment.id) | |||
duedates.each do |due_date| | |||
deadline_type = DeadlineType.find(due_date.deadline_type_id).name | |||
due_at = due_date.due_at.to_s(:db) | |||
Time.parse(due_at) | |||
due_at = Time.parse(due_at) | |||
mi = find_min_from_now(due_at) | |||
diff = mi - due_date.threshold * 60 | |||
next unless diff > 0 | |||
dj = DelayedJob.enqueue(DelayedMailer.new(@assignment.id, deadline_type, due_date.due_at.to_s(:db)), | |||
1, diff.minutes.from_now) | |||
change_item_type(dj.id) | |||
due_date.update_attribute(:delayed_job_id, dj.id) | |||
# If the deadline type is review, add a delayed job to drop outstanding review | |||
if deadline_type == "review" | |||
dj = DelayedJob.enqueue(DelayedMailer.new(@assignment.id, "drop_outstanding_reviews", due_date.due_at.to_s(:db)), | |||
1, mi.minutes.from_now) | |||
change_item_type(dj.id) | |||
end | |||
# If the deadline type is team_formation, add a delayed job to drop one member team | |||
next unless deadline_type == "team_formation" and @assignment.team_assignment? | |||
dj = DelayedJob.enqueue(DelayedMailer.new(@assignment.id, "drop_one_member_topics", due_date.due_at.to_s(:db)), | |||
1, mi.minutes.from_now) | |||
change_item_type(dj.id) | |||
end | |||
endBoth variable names and method names are not in a good manner. We changed it to the version below. | |||
Modified Code: | |||
# Adds items to delayed_jobs queue for this assignment | |||
def add_to_delayed_queue | |||
duedates = AssignmentDueDate.where(parent_id: @assignment.id) | |||
duedates.each do |due_date| | |||
deadline_type = DeadlineType.find(due_date.deadline_type_id).name | |||
diff, mi = get_diff_from_now(due_date) | |||
next unless diff > 0 | |||
dj = add_delayed_job(@assignment, deadline_type, due_date, diff) | |||
due_date.update_attribute(:delayed_job_id, dj.id) | |||
# If the deadline type is review, add a delayed job to drop outstanding review | |||
if deadline_type == "review" | |||
add_delayed_job(@assignment, "drop_outstanding_reviews", due_date, mi) | |||
end | |||
# If the deadline type is team_formation, add a delayed job to drop one member team | |||
next unless deadline_type == "team_formation" and @assignment.team_assignment? | |||
add_delayed_job(@assignment, "drop_one_member_topics", due_date, mi) | |||
end | |||
end | |||
# get time difference between due_date and now | |||
def get_diff_from_now(due_date) | |||
due_at = due_date.due_at.to_s(:db) | |||
Time.parse(due_at) | |||
def | due_at = Time.parse(due_at) | ||
mi = find_min_from_now(due_at) | |||
diff = mi - due_date.threshold * 60 | |||
[diff, mi] | |||
end | end | ||
def | # add DelayedJob into queue and return it | ||
def add_delayed_job(assignment, deadline_type, due_date, diff) | |||
dj = DelayedJob.enqueue(DelayedMailer.new(assignment.id, deadline_type, due_date.due_at.to_s(:db)), | |||
1, diff.minutes.from_now) | |||
change_item_type(dj.id) | |||
dj | |||
end | end | ||
=== | === Rename method 'is_calibrated?' to 'calibrated?' and all other places it is used === | ||
We | In Ruby, the common naming pattern for a method which return a boolean value is an adjective followed by '?', so the method name 'is_calibrated?' is bad and a preferable one should be directly 'calibrated?'. We refactorer this method by using 'Refactor' operation in RubyMine. We renamed it to 'calibrated?' and RubyMine found all place it appeared then changed them. | ||
Before: | Before: | ||
def is_calibrated? | |||
self.is_calibrated | |||
end | |||
After: | After: | ||
def calibrated? | |||
self.is_calibrated | |||
end | end | ||
=== Use 'find_by' instead of 'where.first' === | |||
If we use 'where' and then use 'first' method to return the first record matches the condition, the application will find all eligable records and return the first one. However, 'find_by' method will return a record once it find one and then terminate itself. Therefore, conpared with 'where.first' method, 'find_by' is much more efficient. That's why we use 'find_by' instead of 'where.first'. | |||
Before: | |||
log = Version.where(item_type: "Delayed::Backend::ActiveRecord::Job", item_id: delayed_job_id).first | |||
After: | |||
log = Version.find_by(item_type: "Delayed::Backend::ActiveRecord::Job", item_id: delayed_job_id) | |||
== Rspec Test == | == Rspec Test == | ||
We write rspec test for relating methods in assignment_form_spec.rb. We test `add_to_delayed_queue` method | |||
describe '#add_to_delayed_queue' do | |||
before(:each) do | |||
future_due = build(:assignment_due_date, due_at: Time.now.utc.advance(weeks: 1)) | |||
allow(AssignmentDueDate).to receive_message_chain(:where).and_return([future_due]) | |||
allow_any_instance_of(AssignmentDueDate).to receive(:update_attribute) | |||
end | |||
context 'when the deadline type is review' do | |||
it 'adds two delayed jobs and changes the # of DelayedJob by 2' do | |||
allow(DeadlineType).to receive_message_chain(:find, :name).and_return("review") | |||
num_delayed_job = DelayedJob.count | |||
assignment_form.add_to_delayed_queue | |||
expect(DelayedJob.count).to eq(num_delayed_job + 2) | |||
end | |||
end | |||
context 'when the deadline type is team formation and current assignment is team-based assignment' do | |||
it 'adds a delayed job and changes the # of DelayedJob by 2' do | |||
allow(DeadlineType).to receive_message_chain(:find, :name).and_return("team_formation") | |||
num_delayed_job = DelayedJob.count | |||
assignment_form.add_to_delayed_queue | |||
expect(DelayedJob.count).to eq(num_delayed_job + 2) | |||
end | |||
end | |||
Next we test `change_item_type` method | |||
it | describe '#change_item_type' do | ||
let(:log) { double(Version) } | |||
it 'changes the item_type displayes in the log' do | |||
allow(Version).to receive(:find_by).with(any_args).and_return(log) | |||
allow(log).to receive(:update_attribute).with(any_args) | |||
expect(log).to receive(:update_attribute).with(:item_type, String) | |||
assignment_form.change_item_type(1) | |||
end | |||
end | end | ||
We | We also add test for `calibrated?` methods. | ||
it | describe '#calibrated?' do | ||
context 'when assignment is calibrated' do | |||
it 'return true' do | |||
allow(assignment).to receive(:is_calibrated).and_return(true) | |||
expect(assignment.calibrated?).to be(true) | |||
end | |||
context 'when assignment is not calibrated' do | |||
it 'return false' do | |||
allow(assignment).to receive(:is_calibrated).and_return(false) | |||
expect(assignment.calibrated?).to be(false) | |||
end | |||
We also completed remaining tests in `sign_up_sheet_controller_spec.rb`. There are a lot of methods in `sign_up_sheet_controller.rb`, and it can be executed by 'rspec spec/controllers/sign_up_sheet_controllers_spec.rb' | |||
~/expertiza$ rspec spec/controllers/sign_up_sheet_controller_spec.rb | |||
... | |||
Finished in 6.1 seconds (files took 7.16 seconds to load) | |||
27 examples, 0 failures | |||
Randomized with seed 58167 | |||
==References == | ==References == | ||
Youtube: [https://youtu.be/F1WQDdmUQDk] |
Latest revision as of 02:18, 28 October 2017
This page give a description of the changes made for the assignment_form.rb and related files of Expertiza based OSS project.
Expertiza Background
Expertiza is a web application where students can submit and peer-review learning objects (articles, codes, websites, etc). Instructors add and edit assignments to Expertiza. Students can be assigned in teams based on their selection of the topics.
Problem Statement
The task of the project is to refactor assignment_form.rb and write unit tests for it. assignment_form.rb is a fairly complex file. It contains a lot of methods that are long and hard to understand. These methods need to be broken down into simpler and more specific methods that are easier to read/understand. Also, the few instances of code duplication that exist should be removed.
- Refactor `add_to_delayed_queue` method
- Rename method `is_calibrated` to `calibrated?` and all other places it is used
- Use `find_by` instead of `where.first`
- Complete pending tests in sign_up_sheet_controller.rb
Modified File
- Refactor the assignment.rb
- Refactor the assignment_form.rb
- Complete relating tests in assignment_form_rspec.rb and assignment.rb
- Complete tests in sign_up_sheet_controller_rspec.rb
Refactor
Refactor `add_to_delayed_queue` method
`add_to_delayed_queue` is a long method with some duplicate codes and hard to understand. In order to encourage code reuse and make it easier to understand semantics of the function, we split it into three methods, name the added two as `get_diff_from_now` and `add_delayed_job`. The changes are displayed below,
Original Code:
# Adds items to delayed_jobs queue for this assignment def add_to_delayed_queue duedates = AssignmentDueDate.where(parent_id: @assignment.id) duedates.each do |due_date| deadline_type = DeadlineType.find(due_date.deadline_type_id).name due_at = due_date.due_at.to_s(:db) Time.parse(due_at) due_at = Time.parse(due_at) mi = find_min_from_now(due_at) diff = mi - due_date.threshold * 60 next unless diff > 0 dj = DelayedJob.enqueue(DelayedMailer.new(@assignment.id, deadline_type, due_date.due_at.to_s(:db)), 1, diff.minutes.from_now) change_item_type(dj.id) due_date.update_attribute(:delayed_job_id, dj.id) # If the deadline type is review, add a delayed job to drop outstanding review if deadline_type == "review" dj = DelayedJob.enqueue(DelayedMailer.new(@assignment.id, "drop_outstanding_reviews", due_date.due_at.to_s(:db)), 1, mi.minutes.from_now) change_item_type(dj.id) end # If the deadline type is team_formation, add a delayed job to drop one member team next unless deadline_type == "team_formation" and @assignment.team_assignment? dj = DelayedJob.enqueue(DelayedMailer.new(@assignment.id, "drop_one_member_topics", due_date.due_at.to_s(:db)), 1, mi.minutes.from_now) change_item_type(dj.id) end endBoth variable names and method names are not in a good manner. We changed it to the version below.
Modified Code:
# Adds items to delayed_jobs queue for this assignment def add_to_delayed_queue duedates = AssignmentDueDate.where(parent_id: @assignment.id) duedates.each do |due_date| deadline_type = DeadlineType.find(due_date.deadline_type_id).name diff, mi = get_diff_from_now(due_date) next unless diff > 0 dj = add_delayed_job(@assignment, deadline_type, due_date, diff) due_date.update_attribute(:delayed_job_id, dj.id) # If the deadline type is review, add a delayed job to drop outstanding review if deadline_type == "review" add_delayed_job(@assignment, "drop_outstanding_reviews", due_date, mi) end # If the deadline type is team_formation, add a delayed job to drop one member team next unless deadline_type == "team_formation" and @assignment.team_assignment? add_delayed_job(@assignment, "drop_one_member_topics", due_date, mi) end end
# get time difference between due_date and now def get_diff_from_now(due_date) due_at = due_date.due_at.to_s(:db) Time.parse(due_at) due_at = Time.parse(due_at) mi = find_min_from_now(due_at) diff = mi - due_date.threshold * 60 [diff, mi] end
# add DelayedJob into queue and return it def add_delayed_job(assignment, deadline_type, due_date, diff) dj = DelayedJob.enqueue(DelayedMailer.new(assignment.id, deadline_type, due_date.due_at.to_s(:db)), 1, diff.minutes.from_now) change_item_type(dj.id) dj end
Rename method 'is_calibrated?' to 'calibrated?' and all other places it is used
In Ruby, the common naming pattern for a method which return a boolean value is an adjective followed by '?', so the method name 'is_calibrated?' is bad and a preferable one should be directly 'calibrated?'. We refactorer this method by using 'Refactor' operation in RubyMine. We renamed it to 'calibrated?' and RubyMine found all place it appeared then changed them.
Before:
def is_calibrated? self.is_calibrated end
After:
def calibrated? self.is_calibrated end
Use 'find_by' instead of 'where.first'
If we use 'where' and then use 'first' method to return the first record matches the condition, the application will find all eligable records and return the first one. However, 'find_by' method will return a record once it find one and then terminate itself. Therefore, conpared with 'where.first' method, 'find_by' is much more efficient. That's why we use 'find_by' instead of 'where.first'.
Before:
log = Version.where(item_type: "Delayed::Backend::ActiveRecord::Job", item_id: delayed_job_id).first
After:
log = Version.find_by(item_type: "Delayed::Backend::ActiveRecord::Job", item_id: delayed_job_id)
Rspec Test
We write rspec test for relating methods in assignment_form_spec.rb. We test `add_to_delayed_queue` method
describe '#add_to_delayed_queue' do before(:each) do future_due = build(:assignment_due_date, due_at: Time.now.utc.advance(weeks: 1)) allow(AssignmentDueDate).to receive_message_chain(:where).and_return([future_due]) allow_any_instance_of(AssignmentDueDate).to receive(:update_attribute) end context 'when the deadline type is review' do it 'adds two delayed jobs and changes the # of DelayedJob by 2' do allow(DeadlineType).to receive_message_chain(:find, :name).and_return("review") num_delayed_job = DelayedJob.count assignment_form.add_to_delayed_queue expect(DelayedJob.count).to eq(num_delayed_job + 2) end end
context 'when the deadline type is team formation and current assignment is team-based assignment' do it 'adds a delayed job and changes the # of DelayedJob by 2' do allow(DeadlineType).to receive_message_chain(:find, :name).and_return("team_formation") num_delayed_job = DelayedJob.count assignment_form.add_to_delayed_queue expect(DelayedJob.count).to eq(num_delayed_job + 2) end end
Next we test `change_item_type` method
describe '#change_item_type' do let(:log) { double(Version) } it 'changes the item_type displayes in the log' do allow(Version).to receive(:find_by).with(any_args).and_return(log) allow(log).to receive(:update_attribute).with(any_args) expect(log).to receive(:update_attribute).with(:item_type, String) assignment_form.change_item_type(1) end end
We also add test for `calibrated?` methods.
describe '#calibrated?' do context 'when assignment is calibrated' do it 'return true' do allow(assignment).to receive(:is_calibrated).and_return(true) expect(assignment.calibrated?).to be(true) end
context 'when assignment is not calibrated' do it 'return false' do allow(assignment).to receive(:is_calibrated).and_return(false) expect(assignment.calibrated?).to be(false) end
We also completed remaining tests in `sign_up_sheet_controller_spec.rb`. There are a lot of methods in `sign_up_sheet_controller.rb`, and it can be executed by 'rspec spec/controllers/sign_up_sheet_controllers_spec.rb'
~/expertiza$ rspec spec/controllers/sign_up_sheet_controller_spec.rb ... Finished in 6.1 seconds (files took 7.16 seconds to load) 27 examples, 0 failures Randomized with seed 58167
References
Youtube: [1]