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]