CSC/ECE 517 Fall 2017/E1769 Refactor assignment form.rb

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_form.rb
• Refactor the assignment.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. 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