CSC/ECE 517 Fall 2017/E1771 Refactor team.rb: Difference between revisions

From Expertiza_Wiki
Jump to navigation Jump to search
No edit summary
No edit summary
 
(4 intermediate revisions by 2 users not shown)
Line 1: Line 1:
== Introduction ==
== 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.
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 ==
== 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.
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 ==
== Issue ==
team.rb is a fairly complex file. It contains a lot of methods that are long and hard to understand.  
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
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 ==
== Tasks accomplished ==
     · Refactor self.randomize_all_by_parent method
     · Refactor self.randomize_all_by_parent method
         · Split into several simpler methods and assign reasonable names
         · Split into several simpler methods and assign reasonable names
Line 32: Line 29:
         · L166, L181  
         · 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.
     · 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:  
Elaborating the edits made:  
== Refactor self.randomize_all_by_parent method ==
== Refactor self.randomize_all_by_parent method ==
Previous code:
Previous code:
   def self.randomize_all_by_parent(parent, team_type, min_team_size)
   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 = Participant.where(parent_id: parent.id, type: parent.class.to_s + "Participant")
Line 90: Line 82:
     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: [https://github.com/expertiza/expertiza/pull/1043]

Latest revision as of 22:02, 5 November 2017

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]