CSC/ECE 517 Fall 2017/E1769 Refactor assignment form.rb: Difference between revisions

From Expertiza_Wiki
Jump to navigation Jump to search
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.
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.


== Problem Statement ==
== Problem Statement ==
The task of the project is to refactor leaderboard.rb and write unit tests for it. As the name of the methods are not in a standard style, we use snake_case for method names. There are also some useless assignments to several variables.  
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 test in assignment_form_rspec.rb
* Complete relating tests in assignment_form_rspec.rb and assignment.rb
* Complete test in sign_up_sheet_controller_rspec.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,


== Thoughts ==
Original Code:
What we need to do is to deal with problems listed by the code climate. And we also need to write a new test file to test the functionality of leaderboard.rb. However, the leaderboard itself has some problems and IDE will report some problems when we want to run it. So we need to know which method in this file is broken first and just test others instead.


  # 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.


== Tools==
Modified Code:
As for this project, we use the code climate plugin to test if our code is good enough. This plugin will obviously help us with the refactor part because it can show which part of the original codes have some problems. We also search through the internet and get to know how to write the Rspec files. We also ended up using the 'rspec/collection_matchers' class of rspec which allows us to match the number of items in a collection for an object.
 
 
== Refactor ==
We will list each kind of problems we solved below.


  # 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


=== Use snake_case for method names. ===
  # get time difference between due_date and now
As we all know the snake_case means we need to name the methods and variables in which elements are separated with one underscore character and no spaces. Each element's initial letter is usually lowercased within the compound and the first letter is either upper or lower case.
   def get_diff_from_now(due_date)
For example, we changed this method.
     due_at = due_date.due_at.to_s(:db)
 
    Time.parse(due_at)
   def self.getIndependantAssignments(user_id)
    due_at = Time.parse(due_at)
     assignmentIds = AssignmentParticipant.where(user_id: user_id).pluck(:parent_id)
    mi = find_min_from_now(due_at)
    diff = mi - due_date.threshold * 60
    [diff, mi]
   end
   end
Both variable names and method names are not in a good manner. We changed it to the version below.


   def self.get_independant_assignments(user_id)
  # add DelayedJob into queue and return it
     assignment_ids = AssignmentParticipant.where(user_id: user_id).pluck(:parent_id)
   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


=== Useless assignment to variable.===
=== Rename method 'is_calibrated?' to 'calibrated?' and all other places it is used  ===
We deleted those variables since we only wanted what was being assigned to them.  
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:  


   noCourseAssignments = Assignment.where(id: assignmentIds, course_id: nil)
   def is_calibrated?
    self.is_calibrated
  end


After:
After:


   Assignment.where(id: assignmentIds, course_id: nil)
   def calibrated?
 
    self.is_calibrated
=== Prefer `each` over `for`. ===
We know that 'each' function is more in ruby style for loops. So we would like to change all our 'for' to 'each'.
  for scoreEntry in scores
    ...
  end
We changed this loop to:
  scores.each do |scoreEntry|
    ...
   end
   end


And the problem would be fixed.
=== 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


=== Avoid more than 3 levels of block nesting.===
After:
   if qTypeHash.fetch(questionnaireType, {}).fetch(courseId, nil).nil?
   log = Version.find_by(item_type: "Delayed::Backend::ActiveRecord::Job", item_id: delayed_job_id)
    if qTypeHash.fetch(questionnaireType, nil).nil?
        ...
    end
    ...
  end
 
This is the third and fourth level of the block nesting. We need to merge them together.
 
  if q_type_hash.fetch(questionnaire_type, {}).fetch(course_id, nil).nil? && q_type_hash.fetch(questionnaire_type, nil).nil?
    ...
  elseif  q_type_hash.fetch(questionnaire_type, {}).fetch(course_id, nil).nil? && !q_type_hash.fetch(questionnaire_type, nil).nil?
    ...
  end
Then the 4-levels loop has been changed to 3-levels one.
 
=== `end` at 148, 1 is not aligned with `def` at 129, 2.''' ===
A space is needed to be added in the line of 148.


== Rspec Test ==
== Rspec Test ==
The Leaderboard Class did not have any unit tests associated with it coming into this project so we have to create unit tests from scratch. Upon analysis of this class, we noticed that Leaderboard did not hold any variables and was only made up of static methods. So the first round of unit tests that were created made sure that each method could be called from the Leaderboard class with the correct number of arguments.
We write rspec test for relating methods in assignment_form_spec.rb. We test `add_to_delayed_queue` method
 
  it "Leaderboard responds to get_assignment_mapping" do
    expect(Leaderboard).to respond_to(:get_assignment_mapping).with(3).argument
  end
 
Next we needed to go through the methods and create tests for them. But in order to do this we had to make objects that this class depended on since this class takes the values from other classes to output arrays and hashes. Factories were used to create these objects with a few variables being overridden
 
  before(:each) do
    @student1 = create(:student, name: "Student1", fullname: "Student1 Test", email: "student1@mail.com" )
    @student2 = create(:student, name: "Student2", fullname: "Student2 Test", email: "student2@mail.com" )
    @instructor = create(:instructor)
    @course = create(:course)
    @assignment = create(:assignment, name: "Assign1", course: nil)
    @assignment2 = create(:assignment, name: "Assign2")
    @participant = create(:participant, parent_id: @assignment.id, user_id: @student1.id)
    @participant2 = create(:participant, parent_id: @assignment2.id, user_id: @student2.id)
    @questionnaire=create(:questionnaire)
    @assignment_questionnaire1 =create(:assignment_questionnaire, user_id: @student1.id, assignment: @assignment)
    @assignment_questionnaire2 =create(:assignment_questionnaire, user_id: @student2.id, assignment: @assignment2)
    @assignment_team = create(:assignment_team, name: "TestTeam", parent_id: @assignment2.id)
    @team_user = create(:team_user, team_id: @assignment_team.id, user_id: @student2.id)
  end


We tested the methods by seeing whether if we got the right amount of elements in our arrays/hashes like the following.  
    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


  it "getAssignmentsInCourses should return an assignment" do
    context 'when the deadline type is team formation and current assignment is team-based assignment' do
    expect(Leaderboard.get_assignments_in_courses(1)).to have(1).items
      it 'adds a delayed job and changes the # of DelayedJob by 2' do
  end
        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


We also tested whether the code could handle invalid arguments like such.
Next we test `change_item_type` method


   it "leaderboard_heading should return No Entry with invalid input" do   
   describe '#change_item_type' do
    expect(Leaderboard.leaderboard_heading("Invalid")).to eq("No Entry")
    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 did face situations where methods were being called that were not defined anywhere so we had to use stubs to imitate their expected behavior.  
We also add test for `calibrated?` methods.  


   it "leaderboard_heading should return name" do
   describe '#calibrated?' do
    allow(Leaderboard).to receive(:find_by_qtype).and_return(@questionnaire).with(@questionnaire.id)
    context 'when assignment is calibrated' do
    expect(Leaderboard.leaderboard_heading(@questionnaire.id)).to eq("Test questionaire")
      it 'return true' do
  end
        allow(assignment).to receive(:is_calibrated).and_return(true)
        expect(assignment.calibrated?).to be(true)
      end


== Additional ==
    context 'when assignment is not calibrated' do
To explain our work in a further step, we did a video on Youtube to show the broken leaderboard page, why we did work in this way and our work for the unit tests and the refactor part.
      it 'return false' do
        allow(assignment).to receive(:is_calibrated).and_return(false)
        expect(assignment.calibrated?).to be(false)
      end


There are still some problems we need to figure out. 
For the code revising part, there are some methods with this problem: Assignment Branch Condition size for get_participants_score is too high.


I think we cannot make too many functions in one method so that we need to make some of these be executed outside of this method.
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'
Also, some of the code lines are too long so that it is not clear enough.


During the rspec testing we noticed that the methods get_participant_entries_in_assignment_list and find_by_qtype were being called but where not created in the program. To cover these tests we had to use stubs for these methods for the expected outputs.
  ~/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


There was also no declaration of ScoreCache anywhere in the program which caused other methods to fail. Trying to stub this was a problem since we were not sure of how it was laid out.


==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]