CSC/ECE 517 Fall 2020 - E2067. Refactor student teams controller.rb

Introduction

Background

The student_teams_controller.rb controller used in Expertiza to manipulate teams that are created for assignments. Primary functions that this controller provides is create a new team, update team name and delete members from a team.

Motivation

The motivation of our work is to improve the maintainability and readability of student_teams_controller.rb Controller. Furthermore, we also fix several occasions where the student_teams_controller.rb contains code snippets that actually belongs to the model classes. This will help enforce single responsibility principle and model-view-controller pattern.

Tasks Identified

• Move return unless current_user_id? student.user_id into action_allowed method
• Refactor code from view function into may or may not be necessary to add a method to the DueDates class
• Remove variable current_team
• Add comment for @users_on_waiting_list and also simplify the condition
• Refactor @teammate_review_allowed into due_date.rb
• Rename existing_assignments? to existing_teams
• Add method comment for the update method
• Change (matching_teams[0].name <=> team.name).zero? to (matching_teams[0].name == team.name)?
• Add method comment for Advertise_for_partners and remove_advertisement
• Rename sign_up into signup
• Refactor code from remove_participant into waitlist.rb
• Add method comment for the review method

Affected Classes

• /controllers/student_teams_controller.rb
• /models/waitlist.rb
• /models/due_date.rb

Code Review

The purpose of this section is to provide the before and after change comparison so the developer can perform code review to ensure the correctness of the change.

Move return unless current_user_id? student.user_id into action_allowed method

• Objective

The action_allowed method determines whether an action is permissible, depending on the privileges of the user who is performing it. In the view method, there is a clause that says, return unless current_user_id? student.user_id. This needs to be moved to the when 'view' clause in the action_allowed method.

• Change Comparison

Student_teams_controller.rb:
action_allowed method

view method

• Change Explanation

Deleted 'return unless current_user_id? student.user_id' in the view method, and added it to a if clause under the when 'view' clause in the action_allowed method. So that every time before view method is called, this sentence in the action_allowed method will run first.

Refactor code from view function into may or may not be necessary to add a method to the DueDates class

• Objective

The code beginning at line 44 deals with due_dates, which are part of the “business logic” and should be moved to an appropriate model method. Thus, @student.assignment.due_dates.each do |due_date| should invoke an appropriate method in due_date.rb. It may or may not be necessary to add a method to the DueDates class.

• Change Comparison

Student_teams_controller.rb:

due_date.rb:

• Change Explanation

The code in the first image uses logic associated with due dates to assign a variable called '@current_due_date'. This logic was moved to a new method in the model due_dates.rb, shown in the second image. This logic was moved to to simplify the controller but also because the logic is specific to due dates and is more reasonable to be located in the due dates model.

Remove variable current_team

• Objective

The variable current_team seems unnecessary; it only saves one character vs. @student.team. Consider removing it.

• Change Comparison

• Change Explanation

In the red line 51 above, the variable 'current_team' is used. This variable is unnecessary as it is not significantly shorter or more descriptive. As shown in the changes between red line 53 and green line 49, the 'current_team' variable was removed and replaced with '@student.team'.

Add comment for @users_on_waiting_list and also simplify the condition

• Objective

The code on Line 53 (@users_on_waiting_list ...) is not clear at all. Needs at least a comment, and also the condition should not be so complicated.

• Change Comparison

• Change Explanation

The '@users_on_waiting_list' line of code needed a comment to clarify its function. The original line had a complex conditional that made the code hard to read. This conditional was moved to a new method in which it was refactored and commented to be more understandable and readable.

Refactor @teammate_review_allowed into due_date.rb

• Objective

In line 55, the @teammate_review_allowed: condition is way too complex, uses magic constants, and belongs in code in another model class (maybe due_date.rb); no way should it be in the controller!

• Change Comparison

Student_teams_controller.rb:

• Change Explanation

Moved @teammate_review_allowed 's condition to due_date.rb so the code is more readable.

Rename existing_assignments? to existing_teams

• Objective

Line 59: existing_assignments is evidently a team! So why is the variable name existing_assignments? It should perhaps be changed to existing_teams.

• Change Comparison

• Change Explanation

The variable 'existing_assignments' was actually a team. To better reflect the purpose of the variable and function of the code, the name was changed from 'existing_assignments' to 'existing_teams'.

Add method comment for the update method

• Objective

The update method needs comments about what it is doing.

• Change Comparison (commit 7a9e39)

• Change Explanation

The if condition checks if the new team name is NOT already exist, if so then update the team name. The elsif condition checks if the new team name is the same the current one, if so then nothing is changed. The else covers the case where the new team name is already in use, then print an error message and not changing the team name.

Change (matching_teams[0].name <=> team.name).zero? to (matching_teams[0].name == team.name)?

• Objective

Line 95: Why isn’t (matching_teams[0].name <=> team.name).zero? just (matching_teams[0].name == team.name)?

• Change Comparison (commit d60d92)

• Change Explanation

This change is straight forward to satisfy the objective of the task.

Add method comment for Advertise_for_partners and remove_advertisement

• Objective

Line 108: advertise_for_partners needs a method comment. Since its body is only 1 line, does it make sense to have a separate method for this? It makes sense only if having a separate method improves readability. Ditto for the remove_advertisement method at line 112.

• Change Comparison

• Change Explanation

Both advertise_for_partners and remove_advertisement are necessary in terms of readability.

Rename sign_up into signup

• Objective

The name sign_up needs to be signup; it is being used as a noun (sign_up would suggest a verb, the action of signing up).

• Change Comparison (commit e393ba)

• Change Explanation

I performed a file search on sign_up and it only appears in the places shown in the screenshot. I replaced them with signup according to the objective.

Refactor code from remove_participant into waitlist.rb

• Objective

remove_participant is generally good code, but handling of waitlists is again model activity, and should be moved to a model class, perhaps waitlist.rb.

• Change Comparison (commit 504f84)

• Change Explanation

Line 127~142 in student_teams_controller.rb are moved to Line 14~29 in waitlist.rb. The only input to the new function remove_from_waitlists in waitlist.rb requires params[:team_id] from student_teams_controller.rb.

Add method comment for the review method

• Objective

Line 169: The review method needs a method comment; its purpose is not clear.

• Change Comparison

• Change Explanation

Clarified what review method intends to do.

Test

File: spec/controllers/student_teams_controller_spec.rb

describe StudentTeamsController do
  let (:student_teams_controller) { StudentTeamsController.new }
  let(:student) { double "student" }
  describe '#view' do
    it 'sets the student' do
      allow(AssignmentParticipant).to receive(:find).with('12345').and_return student
      allow(student_teams_controller).to receive(:current_user_id?)
      allow(student_teams_controller).to receive(:params).and_return(student_id: '12345')
      allow(student).to receive(:user_id)
      student_teams_controller.view
    end
  end
end

Future Work

• In the update method of student_teams_controller.rb, the 'elsif' needs a comment to explain its purpose. It is unclear as to why this conditional is present.
• For student_teams_controller update method, simplify the condition logic by combining elsif and else into a single statement