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.

Mentor and Members

Mentor: Zhewei Hu (zhu6@ncsu.edu)

1. Darshil Patel (dpatel14@ncsu.edu)

2. Srujan Barai (sjbarai@ncsu.edu)

3. Shikhar Sharma (ssharm29@ncsu.edu)

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

create_team_from_single_users

def self.create_team_from_single_users(min_team_size, parent, team_type, users)
   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

Rename method and change all other places it is used

Renamed the methods has_user to user?, get_node_type → node_type and get_author_names → author_names and also updated the function calls so that they call the correct (newly renamed) methods

Pass &:destroy as an argument to each instead of a block

TeamsUser.where(team_id: self.id).each{ |teams_user| teams_user.destroy }

was refactored to

TeamsUser.where(team_id: self.id).each(&:destroy)

Use find_by instead of dynamic method

parent = TeamNode.find_by_node_object_id(self.id)

return team_name unless Team.find_by_name(team_name)

user = User.find_by_name(row[index].to_s.strip)

were changed to

parent = TeamNode.find_by(node_object_id: self.id)

return team_name unless Team.find_by(name: team_name)

user = User.find_by(name: row[index].to_s.strip)

Use find_by instead of where.first

participant = AssignmentParticipant.where(user_id: user.id, parent_id: self.parent_id).first

if AssignmentParticipant.where(parent_id: assignment_id, user_id: user.id).first.nil?

if CourseParticipant.where(parent_id: course_id, user_id: user.id).first.nil?

were updated to

participant = AssignmentParticipant.find_by(user_id: user.id, parent_id: self.parent_id)

if AssignmentParticipant.find_by(parent_id: assignment_id, user_id: user.id).nil?

if CourseParticipant.find_by(parent_id: course_id, user_id: user.id).nil?

Test Cases

We have completed all the pending tests in the file team_spec.rb and all the test cases are passing. This can be checked in the pull request which is linked below.

Pull Request

Github Link to Pull Request: [1]