CSC/ECE 517 Spring 2013/OSS E608

Write-up of This Topic.

E608. Refactoring and Testing - Course Related Classes

Classes Involved:

• models/course.rb (76 lines)
• models/course_participant.rb (94 lines)
• models/course_team.rb (116 lines)
• models/course_node.rb (95 lines)

What it does: Involves creation and management of courses in Expertiza

What needs to be done:

• It is not clear what a method named 'create_node' is doing in the 'course' class. Refactor by renaming this method appropriately.
• In the course_team class the import method seems to be doing too many things. See if this method can be broken down into smaller methods with specific functionality.
• The 'get' method in course_node.rb appears to be doing a lot. Refactor this method by breaking it up into smaller methods.
• Look for any unused methods or variables in these files.
• Also apply other refactorings such as Rename variable, Rename method to give the variables and methods more meaningful names.
• Write unit tests for all the methods in each of the listed model classes.

Changes for models/course.rb

The following changes were made:

• Since the method create_node simply creates a course node associated with the newly created or copied course, we replaced its functionality by adding the line CourseNode.create(:node_object_id => course.id) in the course_controller's create method and copy method. The function of getting the CourseNode's parent id is extracted as a class method of CourseNode model, since this parent id is independent of the course. With all of its functionalities refactored to other places, this method is hereby removed.
• Added the line "has_many :assigments, :dependent => :destroy" to allow CourseTest.test_destroy unit test to pass.

Changes for models/course_participant.rb

• CourseParticipant.get_parent_name is removed
  • This method is never called in the project, plus method CourseParticipant.get_course_string provides the same functionality as CourseParticipant.get_parent_name. So we deleted this method following the principle of DRY.

Changes for models/course_team.rb

File Location: <Project Directory>/app/models/course_team.rb

import Method

Functionality: import a single course team for a course from an input array. The first element in the array optionally used to specify a name for the team that’s being imported.

Usage: the method is used by the import file controller to import course teams to a course.

Analysis and Refactor

Decomposition

The import method can be decompose into 5 separate parts

Part 1 – Preparation

Functionality: ensure that the array has enough elements also that the class that the use wishes to import the team to exist.

if row.length < 2
    raise ArgumentError, "Not enough items" 
end 
course = Course.find(id)
if course == nil
    raise ImportError, "The course with id \""+id.to_s+"\" was not found. <a href='/assignment/new'>Create</a> this assignment?"
end

Part 2 – Naming the New Team

Functionality: depending on the option has_column_name the method will either import the team name from the array if has_column_name is set to true or generate a new team name if has_column_name is set to false.

if options[:has_column_names] == "true"
    name = row[0].to_s.strip
    index = 1
else
    name = generate_team_name()
    index = 0
end 

Part 3 – Handling Duplicate Team

Functionality: if a team with the same team name exist this part of the method will handle the conflict depending on the option handle_duplicate.

currTeam = CourseTeam.find(:first, :conditions => ["name =? and parent_id =?",name,course.id])  
if options[:handle_dups] == "ignore" && currTeam != nil
    return
end  
if currTeam != nil && options[:handle_dups] == "rename"
    name = generate_team_name()
    currTeam = nil
end
if options[:handle_dups] == "replace" && teams.first != nil        
for teamsuser in TeamsUser.find(:all, :conditions => ["team_id =?", currTeam.id])
    teamsuser.destroy
    end    
    currTeam.destroy
    currTeam = nil
end     
   

Part 4 – Create the New Team and Node

Functionality: creates a new team and team node base on the information gather by the above parts.

if currTeam == nil
    currTeam = CourseTeam.new
    currTeam.name = name
    currTeam.parent_id = course.id
    currTeam.save
    parent = CourseNode.find_by_node_object_id(course.id)
    TeamNode.create(:parent_id => parent.id, :node_object_id => currTeam.id)
end

Part 5 – Import the Participants to the New Team

Functionality: verify that the users specified in the array exist than add the users to the newly created team.

while(index < row.length) 
    user = User.find_by_name(row[index].to_s.strip)
    if user == nil
        raise ImportError, "The user \""+row[index].to_s.strip+"\" was not found. <a href='/users/new'>Create</a> this user?"                           
    elsif currTeam != nil         
        currUser = TeamsUser.find(:first, :conditions => ["team_id =? and user_id =?", currTeam.id,user.id])          
        if currUser == nil
            currTeam.add_member(user)            
        end                      
    end
    index = index+1      
end

Refactor

At the first glance from the decomposition we can clearly see that part 5 can be separated into its own method

 def import_participants(starting_index, row)
   index = starting_index
   while(index < row.length)
     user = User.find_by_name(row[index].to_s.strip)
     if user.nil?
       raise ImportError, "The user \""+row[index].to_s.strip+"\" was not found. <a href='/users/new'>Create</a> this user?"
     else
       if TeamsUser.find(:first, :conditions => ["team_id =? and user_id =?", id, user.id]).nil?
         add_member(user)
       end
     end
     index = index + 1
   end
 end

From the decomposition we notice that the logic of part 2 and part 3 can be simplified. Originally, part 4 gets executed regardless if the team name is generated or imported. However, if the team name is generated instead of imported there will not be another team with the same name. Therefore, we extracted part 4 into its own method and modified the logic of part 3.

 def self.handle_duplicate(name, course_id, handle_dups)
   team = find(:first, :conditions => ["name =? and parent_id =?", name, course_id])
   if team.nil? #no duplicate
     return name
   end
   if handle_dups == "ignore" #ignore: not create the new team
     return nil
   end
   if handle_dups == "rename" #rename: rename new team
     return generate_team_name
   end
   if handle_dups == "replace" #replace: delete old team
     team.delete
     return name
   end
 end

after modification part 3 and 4 in the import method becomes

   if options[:has_column_names] == "true"
     name = row[0].to_s.strip
     name = handle_duplicate(name, course_id, options[:handle_dups])
     index = 1
   else
     name = generate_team_name
     index = 0
   end

Miscellaneous Changes

Check in part 1 is wrong because number of element can be less than 2 if the array does not have a team name column.

if (row.length < 2 and options[:has_column_names] == "true") or (row.length < 1 and options[:has_column_names] != "true")
  raise ArgumentError, "Not enough items"
end

Part 4 of the method was simplified to improve the readability.

team = CourseTeam.create(:name => name, :parent_id => course_id)
course_node = CourseNode.find_by_node_object_id(course_id)
TeamNode.create(:parent_id => course_node.id, :node_object_id => team.id)

Other Issues

The input variable session was unused in the method therefore it should be removed, however due to the way that the method is being called in the import file controller this change was not made in this project.

Changes for models/course_node.rb

• models/course_node.rb
  • The "get" method was improved.
    • Added "get_courses_managed_by_user" method to hold logic extracted from the "get"method.
    • Added "get_course_query_conditions" method to hold logic extracted from the "get" method.
    • Modified any checks on if the user is a teaching assistant to use polymorphism to make the determination.
    • Removed the initial conditional for setting the "conditions" value since the remaining conditions was overwriting the value regardless of how the initial conditional was evaluated.
    • Renaming the "get" method was considered; however, the name of the method is necessary for polymorphism reasons related to Assignment nodes. As such, a comment was added to explain why the method name should remain.
    • The "sortorder" parameter is given a default value of "ASC" for ascending. This allowed for a related conditional to be removed from the method.
    • The "sortvar" parameter is given a default value of "name" for sorting by course name. This allowed for a related conditional to be removed from the method.
  • Renaming the "get_children" method was considered; however, the name of the method is necessary for polymorphism reasons related to Assignment Nodes. As such, a comment was added to explain why the method name should remain.
  • The "get_survey_distribution_id" method was removed after finding it to be dead code.
• models/ta.rb
  • Added "is_teaching_assistant?" method to override the inherited method from the User model. This supported usage of polymorphism to determine if the user is a teaching assistant.
• models/user.rb
  • Added "is_teaching_assistant?" method to enable usage of polymorphism to determine if the user is a teaching assistant.

Methods Found That Are Not Currently Being Used

The following methods were not found to be used currently by the project, but have been left in the code for potential future usages of the functionality:

• models/course.rb
  • Course.get_participant(user_id)
• models/course_participant.rb
  • CourseParticipant.copy(assignment_id)
  • CourseParticipant.get_parent_name
• models/course_node.rb
  • CourseNode.get_modified_date

List of New Unit Tests and Testcases

The following lists shows new unit files and/or new testcases:

• test/unit/course_node_test.rb (new test file)
  • test_get_children_with_show_true_and_non_TA_and_ascending_order_by_name
  • test_get_children_with_show_true_and_non_TA_and_descending_order_by_name
  • test_get_children_with_show_true_and_TA_and_ascending_order_by_name
  • test_get_children_with_show_true_and_TA_and_descending_order_by_name
  • test_get_children_with_show_false_and_non_TA_and_ascending_order_by_name
  • test_get_children_with_show_false_and_non_TA_and_descending_order_by_name
  • test_get_children_with_show_false_and_TA_and_ascending_order_by_name
  • test_get_children_with_show_false_and_TA_and_descending_order_by_name
  • test_get_children_with_show_false_and_non_TA_and_nil_order_by_nil
  • test_get_name
  • test_get_directory
  • test_get_creation_date
  • test_get_modified_date
  • test_get_teams
  • test_get_parent_id
• test/unit/course_test.rb (previously existing test file)
  • test_get_teams
  • test_get_path
  • test_get_participants
  • test_get_participant
  • test_add_participant
  • test_copy_participants
• test/unit/course_participant_test.rb (previously existing test file)
  • test_get_course_string
  • test_import
  • test_get_path
  • test_export
  • test_get_export_field

List of Modified Unit Tests

The following unit test files and/or testcases were modified:

• test/unit/course_test.rb
  • The line "fixtures :courses" was changed to "fixtures :courses,:teams,:users,:participants,:assignments,:nodes,:tree_folders".