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
• Complete tests in sign_up_sheet_controller_rspec.rb

Refactor

Refactor `add_to_delayed_queue` method

`add_to_delayed_queue` is long method with duplicate codes. So we split it into three methods, name the added two as `get_diff_from_now` and `add_delayed_job`. For example, we changed this method.

 def self.getIndependantAssignments(user_id)
   assignmentIds = AssignmentParticipant.where(user_id: user_id).pluck(:parent_id)
 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)
   assignment_ids = AssignmentParticipant.where(user_id: user_id).pluck(:parent_id)
 end

Rename method 'is_calibrated' to `calibrated?` and all other places it is used

We refactorer this method by using

Before:

noCourseAssignments = Assignment.where(id: assignmentIds, course_id: nil)

After:

 Assignment.where(id: assignmentIds, course_id: nil)

Use `find_by` instead of `where.first`

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

And the problem would be fixed.

aaa

Avoid more than 3 levels of block nesting.

 if qTypeHash.fetch(questionnaireType, {}).fetch(courseId, nil).nil?
    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.

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.

 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.

 it "getAssignmentsInCourses should return an assignment" do
   expect(Leaderboard.get_assignments_in_courses(1)).to have(1).items
 end

We also tested whether the code could handle invalid arguments like such.

 it "leaderboard_heading should return No Entry with invalid input" do    
   expect(Leaderboard.leaderboard_heading("Invalid")).to eq("No Entry")
 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.

 it "leaderboard_heading should return name" do
   allow(Leaderboard).to receive(:find_by_qtype).and_return(@questionnaire).with(@questionnaire.id)
   expect(Leaderboard.leaderboard_heading(@questionnaire.id)).to eq("Test questionaire")
 end

Additional

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.

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

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.