CSC/ECE 517 Fall 2017/E1771 Refactor team.rb

Introduction

Expertiza is an open source webapp built on Ruby on Rails stack. It provides a platform to students with various features like peer-reviewing projects, submitting work, form teams, viewing grades etc. The project is being built and maintained by students and faculty at NCSU.

About team.rb

team.rb is the default class to interact with the team table. team.rb has 2 subclasses assignment_team.rb and course_team.rb.

Issue

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

Tasks accomplished

   · Refactor self.randomize_all_by_parent method
       · Split into several simpler methods and assign reasonable names
       · Extract duplicated code into separate methods
   · Rename method and change all other places it is used
       · has_user →  user?
       · get_node_type → node_type
       · get_author_names → author_names
   · Pass &:destroy as an argument to each instead of a block
       · L22
   · Use find_by instead of dynamic method
       · L63, L153, L162
   · Use find_by instead of where.first
       · L166, L181 
   · Complete the pending tests in team_spec.rb, and write integration tests for newly-created methods. Refactor corresponding methods, and only then finish pending tests.

Elaborating the edits made:

Refactor self.randomize_all_by_parent method

Previous code:

 def self.randomize_all_by_parent(parent, team_type, min_team_size)
   participants = Participant.where(parent_id: parent.id, type: parent.class.to_s + "Participant")
   participants = participants.sort { rand(3) - 1 }
   users = participants.map {|p| User.find(p.user_id) }.to_a
   # find teams still need team members and users who are not in any team
   teams = Team.where(parent_id: parent.id, type: parent.class.to_s + "Team").to_a
   teams_num = teams.size
   i = 0
   teams_num.times do
     teams_users = TeamsUser.where(team_id: teams[i].id)
     teams_users.each do |teams_user|
       users.delete(User.find(teams_user.user_id))
     end
     if Team.size(teams.first.id) >= min_team_size
       teams.delete(teams.first)
     else
       i += 1
     end
   end
   # sort teams by decreasing team size
   teams.sort_by {|team| Team.size(team.id) }.reverse!
   # insert users who are not in any team to teams still need team members
   if !users.empty? and !teams.empty?
     teams.each do |team|
       curr_team_size = Team.size(team.id)
       member_num_difference = min_team_size - curr_team_size
       for i in (1..member_num_difference).to_a
         team.add_member(users.first, parent.id)
         users.delete(users.first)
         break if users.empty?
       end
       break if users.empty?
     end
   end
   # If all the existing teams are fill to the min_team_size and we still have more users, create teams for them.
   unless users.empty?
     num_of_teams = users.length.fdiv(min_team_size).ceil
     nextTeamMemberIndex = 0
     for i in (1..num_of_teams).to_a
       team = Object.const_get(team_type + 'Team').create(name: "Team" + i.to_s, parent_id: parent.id)
       TeamNode.create(parent_id: parent.id, node_object_id: team.id)
       min_team_size.times do
         break if nextTeamMemberIndex >= users.length
         user = users[nextTeamMemberIndex]
         team.add_member(user, parent.id)
         nextTeamMemberIndex += 1
       end
     end
   end
 end

This method has been edited and divided into following methods:

sort_teams_by_members_reverse()

def self.sort_teams_by_members_reverse(teams)
    teams.sort_by { |team| Team.size(team.id) }.reverse!
 end

assign_single_users_to_teams

def self.assign_single_users_to_teams(min_team_size, parent, teams, users)
   teams.each do |team|
     curr_team_size = Team.size(team.id)
     member_num_difference = min_team_size - curr_team_size
     for i in (1..member_num_difference).to_a
       team.add_member(users.first, parent.id)
       users.delete(users.first)
       break if users.empty?
     end
     break if users.empty?
   end
 end