Expertiza_Wiki - User contributions [en]

CSC/ECE 517 E2024 Mentor management for assignments without topics

2020-04-14T00:13:36Z

Thwinter: /* Project Goal */

CSC/ECE 517 E2024 Mentor management for assignments without topics

== Problem Statement ==

Currently, Expertiza has no way to associate mentors with teams. For assignments with topics, like the OSS project, mentors are associated with topics, and then whichever team is assigned to the topic inherits the mentor for that topic, However, for assignments without topics (like Program 2), there is no good way to “automatically” assign mentors to projects. The instructor needs to watch teams being formed, and every time a new team is formed, a new mentor needs to be assigned, outside of Expertiza. This leads to a lot of work for the instructor, as well as sometimes long delays before a team is assigned a mentor.

== Project Goal ==

Develop a trigger that: 
# Is activated when any team has been formed that has k members, where k is greater than 50% of the maximum team capacity 
#* ex. max members = 4, trigger activated when the team size reaches 3
# Assign a mentor to the team. Mentors should be evenly assigned to teams, so a good strategy is to assign the mentor who has the fewest teams to mentor so far.
# Notify the mentor via email that they are now assigned to a specific team, and provide the email addresses of the team members.
# Possibly notify the team members that they have been assigned the mentor with contact information.

== Previous Work ==
=== Design Pattern ===

Since the trigger they implemented would need multiple handlers and each of the responses in different actions, they decided to use Chain of Responsibility as the design pattern. Chain of Responsibility is a behavioural design pattern that lets you pass requests along a chain of handlers. Upon receiving a request, each handler decides either to process the request or to pass it to the next handler in the chain.
[[File:Cor.PNG]]

=== Work Flow Diagram ===
[[File:Orig flow mentor.jpg]]
[[File:Orig flow mentor1.jpg]]

== Improvement on the Previous Work ==
=== Change in Work Flow Diagram ===
[[File:New flow mentor1.jpg]]

'''Refactor Code to follow good coding practices''' 
Example: 
# Rename method names more meaningfully and intuitive like, from able_to_review to can_review.
# Reduce the number of conditional statements checking for mentor.
# Rename variable names to reduce confusion like changing lowest_team_no to lowest_number_of_teams.

'''Correct Previous Design'''
# Remove mentors from being included in a team's number of members count

'''Code placed in the wrong locations to be moved to the desired locations.''' 
Example: 
# The email code has to be moved to the email module.
# Code written in team file has to be moved to the assignment team file.

'''Remove Duplicate Functions''' 
Example: 
Though there is a method "team" to list the members of a team, a new method has been created, which is to be removed.

[[File:Check.png]]

== Testing ==
# Some existing test cases have been modified, which has to be reverted. 
# RSpec tests for the email functionality will be added. 
# Run and pass the existing RSpec Tests. 
# Develop New RSpec Tests for the new code additions (additional features). 
# Test the UI for the deployed project. 

== Functionalities to be added ==

# The mentor should be able to check submissions of his team.

CSC/ECE 517 E2024 Mentor management for assignments without topics

2020-04-14T00:13:25Z

Thwinter: /* Project Goal */

CSC/ECE 517 E2024 Mentor management for assignments without topics

== Problem Statement ==

Currently, Expertiza has no way to associate mentors with teams. For assignments with topics, like the OSS project, mentors are associated with topics, and then whichever team is assigned to the topic inherits the mentor for that topic, However, for assignments without topics (like Program 2), there is no good way to “automatically” assign mentors to projects. The instructor needs to watch teams being formed, and every time a new team is formed, a new mentor needs to be assigned, outside of Expertiza. This leads to a lot of work for the instructor, as well as sometimes long delays before a team is assigned a mentor.

== Project Goal ==

Develop a trigger that: 
# Is activated when any team has been formed that has k members, where k is greater than 50% of the maximum team capacity 
** ex. max members = 4, trigger activated when the team size reaches 3
# Assign a mentor to the team. Mentors should be evenly assigned to teams, so a good strategy is to assign the mentor who has the fewest teams to mentor so far.
# Notify the mentor via email that they are now assigned to a specific team, and provide the email addresses of the team members.
# Possibly notify the team members that they have been assigned the mentor with contact information.

== Previous Work ==
=== Design Pattern ===

Since the trigger they implemented would need multiple handlers and each of the responses in different actions, they decided to use Chain of Responsibility as the design pattern. Chain of Responsibility is a behavioural design pattern that lets you pass requests along a chain of handlers. Upon receiving a request, each handler decides either to process the request or to pass it to the next handler in the chain.
[[File:Cor.PNG]]

=== Work Flow Diagram ===
[[File:Orig flow mentor.jpg]]
[[File:Orig flow mentor1.jpg]]

== Improvement on the Previous Work ==
=== Change in Work Flow Diagram ===
[[File:New flow mentor1.jpg]]

'''Refactor Code to follow good coding practices''' 
Example: 
# Rename method names more meaningfully and intuitive like, from able_to_review to can_review.
# Reduce the number of conditional statements checking for mentor.
# Rename variable names to reduce confusion like changing lowest_team_no to lowest_number_of_teams.

'''Correct Previous Design'''
# Remove mentors from being included in a team's number of members count

'''Code placed in the wrong locations to be moved to the desired locations.''' 
Example: 
# The email code has to be moved to the email module.
# Code written in team file has to be moved to the assignment team file.

'''Remove Duplicate Functions''' 
Example: 
Though there is a method "team" to list the members of a team, a new method has been created, which is to be removed.

[[File:Check.png]]

== Testing ==
# Some existing test cases have been modified, which has to be reverted. 
# RSpec tests for the email functionality will be added. 
# Run and pass the existing RSpec Tests. 
# Develop New RSpec Tests for the new code additions (additional features). 
# Test the UI for the deployed project. 

== Functionalities to be added ==

# The mentor should be able to check submissions of his team.

CSC/ECE 517 E2024 Mentor management for assignments without topics

2020-04-14T00:13:16Z

Thwinter: /* Project Goal */

CSC/ECE 517 E2024 Mentor management for assignments without topics

== Problem Statement ==

Currently, Expertiza has no way to associate mentors with teams. For assignments with topics, like the OSS project, mentors are associated with topics, and then whichever team is assigned to the topic inherits the mentor for that topic, However, for assignments without topics (like Program 2), there is no good way to “automatically” assign mentors to projects. The instructor needs to watch teams being formed, and every time a new team is formed, a new mentor needs to be assigned, outside of Expertiza. This leads to a lot of work for the instructor, as well as sometimes long delays before a team is assigned a mentor.

== Project Goal ==

Develop a trigger that: 
# Is activated when any team has been formed that has k members, where k is greater than 50% of the maximum team capacity 
* ex. max members = 4, trigger activated when the team size reaches 3
# Assign a mentor to the team. Mentors should be evenly assigned to teams, so a good strategy is to assign the mentor who has the fewest teams to mentor so far.
# Notify the mentor via email that they are now assigned to a specific team, and provide the email addresses of the team members.
# Possibly notify the team members that they have been assigned the mentor with contact information.

== Previous Work ==
=== Design Pattern ===

Since the trigger they implemented would need multiple handlers and each of the responses in different actions, they decided to use Chain of Responsibility as the design pattern. Chain of Responsibility is a behavioural design pattern that lets you pass requests along a chain of handlers. Upon receiving a request, each handler decides either to process the request or to pass it to the next handler in the chain.
[[File:Cor.PNG]]

=== Work Flow Diagram ===
[[File:Orig flow mentor.jpg]]
[[File:Orig flow mentor1.jpg]]

== Improvement on the Previous Work ==
=== Change in Work Flow Diagram ===
[[File:New flow mentor1.jpg]]

'''Refactor Code to follow good coding practices''' 
Example: 
# Rename method names more meaningfully and intuitive like, from able_to_review to can_review.
# Reduce the number of conditional statements checking for mentor.
# Rename variable names to reduce confusion like changing lowest_team_no to lowest_number_of_teams.

'''Correct Previous Design'''
# Remove mentors from being included in a team's number of members count

'''Code placed in the wrong locations to be moved to the desired locations.''' 
Example: 
# The email code has to be moved to the email module.
# Code written in team file has to be moved to the assignment team file.

'''Remove Duplicate Functions''' 
Example: 
Though there is a method "team" to list the members of a team, a new method has been created, which is to be removed.

[[File:Check.png]]

== Testing ==
# Some existing test cases have been modified, which has to be reverted. 
# RSpec tests for the email functionality will be added. 
# Run and pass the existing RSpec Tests. 
# Develop New RSpec Tests for the new code additions (additional features). 
# Test the UI for the deployed project. 

== Functionalities to be added ==

# The mentor should be able to check submissions of his team.

CSC/ECE 517 E2024 Mentor management for assignments without topics

2020-04-14T00:12:57Z

Thwinter: /* Project Goal */

CSC/ECE 517 E2024 Mentor management for assignments without topics

== Problem Statement ==

Currently, Expertiza has no way to associate mentors with teams. For assignments with topics, like the OSS project, mentors are associated with topics, and then whichever team is assigned to the topic inherits the mentor for that topic, However, for assignments without topics (like Program 2), there is no good way to “automatically” assign mentors to projects. The instructor needs to watch teams being formed, and every time a new team is formed, a new mentor needs to be assigned, outside of Expertiza. This leads to a lot of work for the instructor, as well as sometimes long delays before a team is assigned a mentor.

== Project Goal ==

Develop a trigger that: 
# Is activated when any team has been formed that has k members, where k is greater than 50% of the maximum team capacity 
ex. max members = 4, trigger activated when the team size reaches 3
# Assign a mentor to the team. Mentors should be evenly assigned to teams, so a good strategy is to assign the mentor who has the fewest teams to mentor so far.
# Notify the mentor via email that they are now assigned to a specific team, and provide the email addresses of the team members.
# Possibly notify the team members that they have been assigned the mentor with contact information.

== Previous Work ==
=== Design Pattern ===

Since the trigger they implemented would need multiple handlers and each of the responses in different actions, they decided to use Chain of Responsibility as the design pattern. Chain of Responsibility is a behavioural design pattern that lets you pass requests along a chain of handlers. Upon receiving a request, each handler decides either to process the request or to pass it to the next handler in the chain.
[[File:Cor.PNG]]

=== Work Flow Diagram ===
[[File:Orig flow mentor.jpg]]
[[File:Orig flow mentor1.jpg]]

== Improvement on the Previous Work ==
=== Change in Work Flow Diagram ===
[[File:New flow mentor1.jpg]]

'''Refactor Code to follow good coding practices''' 
Example: 
# Rename method names more meaningfully and intuitive like, from able_to_review to can_review.
# Reduce the number of conditional statements checking for mentor.
# Rename variable names to reduce confusion like changing lowest_team_no to lowest_number_of_teams.

'''Correct Previous Design'''
# Remove mentors from being included in a team's number of members count

'''Code placed in the wrong locations to be moved to the desired locations.''' 
Example: 
# The email code has to be moved to the email module.
# Code written in team file has to be moved to the assignment team file.

'''Remove Duplicate Functions''' 
Example: 
Though there is a method "team" to list the members of a team, a new method has been created, which is to be removed.

[[File:Check.png]]

== Testing ==
# Some existing test cases have been modified, which has to be reverted. 
# RSpec tests for the email functionality will be added. 
# Run and pass the existing RSpec Tests. 
# Develop New RSpec Tests for the new code additions (additional features). 
# Test the UI for the deployed project. 

== Functionalities to be added ==

# The mentor should be able to check submissions of his team.

CSC/ECE 517 E2024 Mentor management for assignments without topics

2020-04-14T00:12:30Z

Thwinter: /* Project Goal */

CSC/ECE 517 E2024 Mentor management for assignments without topics

== Problem Statement ==

Currently, Expertiza has no way to associate mentors with teams. For assignments with topics, like the OSS project, mentors are associated with topics, and then whichever team is assigned to the topic inherits the mentor for that topic, However, for assignments without topics (like Program 2), there is no good way to “automatically” assign mentors to projects. The instructor needs to watch teams being formed, and every time a new team is formed, a new mentor needs to be assigned, outside of Expertiza. This leads to a lot of work for the instructor, as well as sometimes long delays before a team is assigned a mentor.

== Project Goal ==

Develop a trigger that: 
# Is activated when any team has been formed that has k members, where k is greater than 50% of the maximum team capacity 
ex. max members = 4, trigger activated when the team size reaches 3
# Assign a mentor to the team. Mentors should be evenly assigned to teams, so a good strategy is to assign the mentor who has the fewest teams to mentor so far.
# Notify the mentor via email that they are now assigned to a specific team, and provide the email addresses of the team members.
# Possibly notify the team members that they have been assigned the mentor with contact information.

== Previous Work ==
=== Design Pattern ===

Since the trigger they implemented would need multiple handlers and each of the responses in different actions, they decided to use Chain of Responsibility as the design pattern. Chain of Responsibility is a behavioural design pattern that lets you pass requests along a chain of handlers. Upon receiving a request, each handler decides either to process the request or to pass it to the next handler in the chain.
[[File:Cor.PNG]]

=== Work Flow Diagram ===
[[File:Orig flow mentor.jpg]]
[[File:Orig flow mentor1.jpg]]

== Improvement on the Previous Work ==
=== Change in Work Flow Diagram ===
[[File:New flow mentor1.jpg]]

'''Refactor Code to follow good coding practices''' 
Example: 
# Rename method names more meaningfully and intuitive like, from able_to_review to can_review.
# Reduce the number of conditional statements checking for mentor.
# Rename variable names to reduce confusion like changing lowest_team_no to lowest_number_of_teams.

'''Correct Previous Design'''
# Remove mentors from being included in a team's number of members count

'''Code placed in the wrong locations to be moved to the desired locations.''' 
Example: 
# The email code has to be moved to the email module.
# Code written in team file has to be moved to the assignment team file.

'''Remove Duplicate Functions''' 
Example: 
Though there is a method "team" to list the members of a team, a new method has been created, which is to be removed.

[[File:Check.png]]

== Testing ==
# Some existing test cases have been modified, which has to be reverted. 
# RSpec tests for the email functionality will be added. 
# Run and pass the existing RSpec Tests. 
# Develop New RSpec Tests for the new code additions (additional features). 
# Test the UI for the deployed project. 

== Functionalities to be added ==

# The mentor should be able to check submissions of his team.

CSC/ECE 517 E2024 Mentor management for assignments without topics

2020-04-14T00:11:52Z

Thwinter: /* Project Goal */

CSC/ECE 517 E2024 Mentor management for assignments without topics

== Problem Statement ==

Currently, Expertiza has no way to associate mentors with teams. For assignments with topics, like the OSS project, mentors are associated with topics, and then whichever team is assigned to the topic inherits the mentor for that topic, However, for assignments without topics (like Program 2), there is no good way to “automatically” assign mentors to projects. The instructor needs to watch teams being formed, and every time a new team is formed, a new mentor needs to be assigned, outside of Expertiza. This leads to a lot of work for the instructor, as well as sometimes long delays before a team is assigned a mentor.

== Project Goal ==

Develop a trigger that: 
# Is activated when any team has been formed that has k members, where k is greater than 50% of the maximum team capacity 
ex. max members = 4, trigger activated when the team size reaches 3
# Assign a mentor to the team. Mentors should be evenly assigned to teams, so a good strategy is to assign the mentor who has the fewest teams to mentor so far.
# Notify the mentor via email that they are now assigned to a specific team, and provide the email addresses of the team members.
# Possibly notify the team members that they have been assigned the mentor with contact information.

== Previous Work ==
=== Design Pattern ===

Since the trigger they implemented would need multiple handlers and each of the responses in different actions, they decided to use Chain of Responsibility as the design pattern. Chain of Responsibility is a behavioural design pattern that lets you pass requests along a chain of handlers. Upon receiving a request, each handler decides either to process the request or to pass it to the next handler in the chain.
[[File:Cor.PNG]]

=== Work Flow Diagram ===
[[File:Orig flow mentor.jpg]]
[[File:Orig flow mentor1.jpg]]

== Improvement on the Previous Work ==
=== Change in Work Flow Diagram ===
[[File:New flow mentor1.jpg]]

'''Refactor Code to follow good coding practices''' 
Example: 
# Rename method names more meaningfully and intuitive like, from able_to_review to can_review.
# Reduce the number of conditional statements checking for mentor.
# Rename variable names to reduce confusion like changing lowest_team_no to lowest_number_of_teams.

'''Correct Previous Design'''
# Remove mentors from being included in a team's number of members count

'''Code placed in the wrong locations to be moved to the desired locations.''' 
Example: 
# The email code has to be moved to the email module.
# Code written in team file has to be moved to the assignment team file.

'''Remove Duplicate Functions''' 
Example: 
Though there is a method "team" to list the members of a team, a new method has been created, which is to be removed.

[[File:Check.png]]

== Testing ==
# Some existing test cases have been modified, which has to be reverted. 
# RSpec tests for the email functionality will be added. 
# Run and pass the existing RSpec Tests. 
# Develop New RSpec Tests for the new code additions (additional features). 
# Test the UI for the deployed project. 

== Functionalities to be added ==

# The mentor should be able to check submissions of his team.

CSC/ECE 517 E2024 Mentor management for assignments without topics

2020-04-14T00:11:08Z

Thwinter: /* Functionalities to be added */

CSC/ECE 517 E2024 Mentor management for assignments without topics

== Problem Statement ==

Currently, Expertiza has no way to associate mentors with teams. For assignments with topics, like the OSS project, mentors are associated with topics, and then whichever team is assigned to the topic inherits the mentor for that topic, However, for assignments without topics (like Program 2), there is no good way to “automatically” assign mentors to projects. The instructor needs to watch teams being formed, and every time a new team is formed, a new mentor needs to be assigned, outside of Expertiza. This leads to a lot of work for the instructor, as well as sometimes long delays before a team is assigned a mentor.

== Project Goal ==

Develop a trigger that: 
1) Is activated when any team has been formed that has k members, where k is greater than 50% of the maximum team capacity 
ex. max members = 4, trigger activated when the team size reaches 3 
2) Assign a mentor to the team. Mentors should be evenly assigned to teams, so a good strategy is to assign the mentor who has the fewest teams to mentor so far. 
3) Notify the mentor via email that they are now assigned to a specific team, and provide the email addresses of the team members. 
4) Possibly notify the team members that they have been assigned the mentor with contact information. 

== Previous Work ==
=== Design Pattern ===

Since the trigger they implemented would need multiple handlers and each of the responses in different actions, they decided to use Chain of Responsibility as the design pattern. Chain of Responsibility is a behavioural design pattern that lets you pass requests along a chain of handlers. Upon receiving a request, each handler decides either to process the request or to pass it to the next handler in the chain.
[[File:Cor.PNG]]

=== Work Flow Diagram ===
[[File:Orig flow mentor.jpg]]
[[File:Orig flow mentor1.jpg]]

== Improvement on the Previous Work ==
=== Change in Work Flow Diagram ===
[[File:New flow mentor1.jpg]]

'''Refactor Code to follow good coding practices''' 
Example: 
# Rename method names more meaningfully and intuitive like, from able_to_review to can_review.
# Reduce the number of conditional statements checking for mentor.
# Rename variable names to reduce confusion like changing lowest_team_no to lowest_number_of_teams.

'''Correct Previous Design'''
# Remove mentors from being included in a team's number of members count

'''Code placed in the wrong locations to be moved to the desired locations.''' 
Example: 
# The email code has to be moved to the email module.
# Code written in team file has to be moved to the assignment team file.

'''Remove Duplicate Functions''' 
Example: 
Though there is a method "team" to list the members of a team, a new method has been created, which is to be removed.

[[File:Check.png]]

== Testing ==
# Some existing test cases have been modified, which has to be reverted. 
# RSpec tests for the email functionality will be added. 
# Run and pass the existing RSpec Tests. 
# Develop New RSpec Tests for the new code additions (additional features). 
# Test the UI for the deployed project. 

== Functionalities to be added ==

# The mentor should be able to check submissions of his team.

CSC/ECE 517 E2024 Mentor management for assignments without topics

2020-04-14T00:10:55Z

Thwinter: /* Testing */

CSC/ECE 517 E2024 Mentor management for assignments without topics

== Problem Statement ==

Currently, Expertiza has no way to associate mentors with teams. For assignments with topics, like the OSS project, mentors are associated with topics, and then whichever team is assigned to the topic inherits the mentor for that topic, However, for assignments without topics (like Program 2), there is no good way to “automatically” assign mentors to projects. The instructor needs to watch teams being formed, and every time a new team is formed, a new mentor needs to be assigned, outside of Expertiza. This leads to a lot of work for the instructor, as well as sometimes long delays before a team is assigned a mentor.

== Project Goal ==

Develop a trigger that: 
1) Is activated when any team has been formed that has k members, where k is greater than 50% of the maximum team capacity 
ex. max members = 4, trigger activated when the team size reaches 3 
2) Assign a mentor to the team. Mentors should be evenly assigned to teams, so a good strategy is to assign the mentor who has the fewest teams to mentor so far. 
3) Notify the mentor via email that they are now assigned to a specific team, and provide the email addresses of the team members. 
4) Possibly notify the team members that they have been assigned the mentor with contact information. 

== Previous Work ==
=== Design Pattern ===

Since the trigger they implemented would need multiple handlers and each of the responses in different actions, they decided to use Chain of Responsibility as the design pattern. Chain of Responsibility is a behavioural design pattern that lets you pass requests along a chain of handlers. Upon receiving a request, each handler decides either to process the request or to pass it to the next handler in the chain.
[[File:Cor.PNG]]

=== Work Flow Diagram ===
[[File:Orig flow mentor.jpg]]
[[File:Orig flow mentor1.jpg]]

== Improvement on the Previous Work ==
=== Change in Work Flow Diagram ===
[[File:New flow mentor1.jpg]]

'''Refactor Code to follow good coding practices''' 
Example: 
# Rename method names more meaningfully and intuitive like, from able_to_review to can_review.
# Reduce the number of conditional statements checking for mentor.
# Rename variable names to reduce confusion like changing lowest_team_no to lowest_number_of_teams.

'''Correct Previous Design'''
# Remove mentors from being included in a team's number of members count

'''Code placed in the wrong locations to be moved to the desired locations.''' 
Example: 
# The email code has to be moved to the email module.
# Code written in team file has to be moved to the assignment team file.

'''Remove Duplicate Functions''' 
Example: 
Though there is a method "team" to list the members of a team, a new method has been created, which is to be removed.

[[File:Check.png]]

== Testing ==
# Some existing test cases have been modified, which has to be reverted. 
# RSpec tests for the email functionality will be added. 
# Run and pass the existing RSpec Tests. 
# Develop New RSpec Tests for the new code additions (additional features). 
# Test the UI for the deployed project. 

== Functionalities to be added ==

1) The mentor should be able to check submissions of his team.

CSC/ECE 517 E2024 Mentor management for assignments without topics

2020-04-14T00:10:21Z

Thwinter: /* Improvement on the Previous Work */

CSC/ECE 517 E2024 Mentor management for assignments without topics

== Problem Statement ==

Currently, Expertiza has no way to associate mentors with teams. For assignments with topics, like the OSS project, mentors are associated with topics, and then whichever team is assigned to the topic inherits the mentor for that topic, However, for assignments without topics (like Program 2), there is no good way to “automatically” assign mentors to projects. The instructor needs to watch teams being formed, and every time a new team is formed, a new mentor needs to be assigned, outside of Expertiza. This leads to a lot of work for the instructor, as well as sometimes long delays before a team is assigned a mentor.

== Project Goal ==

Develop a trigger that: 
1) Is activated when any team has been formed that has k members, where k is greater than 50% of the maximum team capacity 
ex. max members = 4, trigger activated when the team size reaches 3 
2) Assign a mentor to the team. Mentors should be evenly assigned to teams, so a good strategy is to assign the mentor who has the fewest teams to mentor so far. 
3) Notify the mentor via email that they are now assigned to a specific team, and provide the email addresses of the team members. 
4) Possibly notify the team members that they have been assigned the mentor with contact information. 

== Previous Work ==
=== Design Pattern ===

Since the trigger they implemented would need multiple handlers and each of the responses in different actions, they decided to use Chain of Responsibility as the design pattern. Chain of Responsibility is a behavioural design pattern that lets you pass requests along a chain of handlers. Upon receiving a request, each handler decides either to process the request or to pass it to the next handler in the chain.
[[File:Cor.PNG]]

=== Work Flow Diagram ===
[[File:Orig flow mentor.jpg]]
[[File:Orig flow mentor1.jpg]]

== Improvement on the Previous Work ==
=== Change in Work Flow Diagram ===
[[File:New flow mentor1.jpg]]

'''Refactor Code to follow good coding practices''' 
Example: 
# Rename method names more meaningfully and intuitive like, from able_to_review to can_review.
# Reduce the number of conditional statements checking for mentor.
# Rename variable names to reduce confusion like changing lowest_team_no to lowest_number_of_teams.

'''Correct Previous Design'''
# Remove mentors from being included in a team's number of members count

'''Code placed in the wrong locations to be moved to the desired locations.''' 
Example: 
# The email code has to be moved to the email module.
# Code written in team file has to be moved to the assignment team file.

'''Remove Duplicate Functions''' 
Example: 
Though there is a method "team" to list the members of a team, a new method has been created, which is to be removed.

[[File:Check.png]]

== Testing ==
1)Some existing test cases have been modified, which has to be reverted. 
2)RSpec tests for the email functionality will be added. 
3)Run and pass the existing RSpec Tests. 
4)Develop New RSpec Tests for the new code additions (additional features). 
5)Test the UI for the deployed project. 

== Functionalities to be added ==

1) The mentor should be able to check submissions of his team.

CSC/ECE 517 Spring 2020/E2012. refactor lottery controller.rb

2020-03-30T23:56:55Z

Thwinter: /* Test Cases */

= Introduction =

== Expertiza Background ==

Expertiza is an open-source project written using the Ruby on Rails framework. The Expertiza platform aims to improve student learning by using active and cooperative learning, allowing distance education students to participate in active learning, and by discouraging plagiarism through the use of multiple deadlines. Expertiza helps to improve teaching and resource utilization by having students contribute to the creation of course materials and through peer-graded assignments. Expertiza manages objects related to a course such as instructors, TAs, students, and other users. It also manages assignments, teams, signup topics, rubrics, peer grading, and peer evaluations.

== Lottery Controller Background ==

An assignment can have a list of signup topics. The teams associated with the assignment can bid on the signup topics to try and get assigned one they find more favorable. At a high level, the lottery controller assigns teams to topics based on the priorities the team gave to each signup topic during the bidding process.

In more detail, each student starts off on a team of size 1. They can bid on signup topics by arranging them in priority order. A team can invite other users to join their team. If student2 from Team B joins Team A which includes student1, student2 will lose their bids and take on the bids of TeamA and student1.
When the lottery controller is called to run its method run_intelligent_assignment, a web service takes the bidding data from each team and returns a new list of teams, each of which is closer to the maximum team size specified for the assignment. The web service combines teams together that have similar
bid data and then assigns those new teams to topics, giving each team their top bid on a topic that hasn't been assigned yet. Teams with larger team sizes and more bids are assigned their topics first.

== Problem Statement ==

Code Climate currently grades <code>lottery_controller.rb</code> at a C level, finding issues with the controller's long methods and complex statements. Most methods are around 40 lines and hard to decipher with a quick look. Also, the file currently has only 10% test coverage. Our goal is to resolve the Code Climate issues, make methods simpler to understand, and write new tests to improve test coverage.

== Tasks Identified ==

# Cut down on code in run_intelligent_assignment method by moving some code to helper methods
# Cut down on code in create_new_teams_for_bidding_response by moving some code to helper methods
# Cut down on code in match_new_teams_to_topics by moving some code to helper methods
# Reduce cognitive complexity of merge_bids_from_different_previous_teams method
# Add additional comments to clarify newly written methods
# Add additional RSpec tests to increase the coverage percentage

= Implementation =

We did the following refactoring to the <code>LotteryController</code> class:

# Added 6 helper methods (<code>construct_users_bidding_info</code>, <code>construct_teams_bidding_info</code>, <code>remove_user_from_previous_team</code>, <code>remove_empty_teams</code>, <code>assign_available_slots</code>) that each represent a specific and meaningful step
# Replaced large chunks of code with a small number of method calls
# Reduced the cognitive complexity by reducing the level of nested loops
# Optimized the performance by moving conditional checking and input filtering outside of loops, where appropriate
# Renamed some variable names to make their purpose clearer
# Refactored the parameter list to give each method the least amount of information needed to complete its task
# Added addition comments and fixed some typos

Here are some examples of the problems we identified and the solutions we found to these problems.

=== Problem 1: Long Methods ===

The code climate analysis tool identified two long methods exceeding 25 lines of code per method, <code>run_intelligent_assignment</code> and <code>match_new_teams_to_topics</code>. While some chunks of code are short and intuitive, others can be pulled out and put into helper methods with more meaningful method names.

====Solutions ====

=====run_intelligent_assignment=====

1. Put lines 19-29 into a separate method called <code>construct_users_bidding_info</code> that is responsible for generating user bidding information hash for students on a team who haven't signed up for a topic yet

[[Media:E2012_1.png| view code changes]]

2. Place the code closest to where it is needed. e.g. place line 32 (the <code>url</code> variable initialization) inside the <code>begin/rescue</code> block because it is considered an integral part of the REST API call.

[[Media:E2012_2.png| view code changes]]

===== match_new_teams_to_topics =====

1. Conclude lines 147-157 with a new method <code>construct_teams_bidding_info</code> that is responsible for generating team bidding information needed for the <code>assign_available_slots</code> method.

2. Conclude lines 161-169 with a new method <code>assign_available_slots</code> that uses the result returned from <code>construct_teams_bidding_info</code> to match unassigned teams to available topics.

These two steps greatly shorten the <code>match_new_teams_to_topics</code> method and make the workflow more intuitive to code maintainers.

[[Media:E2012_3.png| view code changes]]

=== Problem 2: Verbose Code ===

Verbosity contributes to more lines of codes and increases the cognitive complexity, which is the measure of how difficult a unit of code is to intuitively understand. Method extraction can't be applied to every long method as some methods only do one thing. For these methods, one can keep them simple with alternative language syntax and other ways of thinking of the behavior.

==== Solutions ====

===== Example 1 =====

# Before
bidding_matrix = {}
assignment.sign_up_topics.each_with_index do |topic, index|
bidding_matrix[topic.id] = [] unless bidding_matrix[topic.id]
bidding_matrix[topic.id] << bids[index]
end

The above code segment ensures that <code>bidding_matrix[topic.id]</code> returns an array object before pushing a new value to it. It is inelegant both in its readability and the time wasted to visit each <code>bidding_matrix[topic.id]</code> twice. The same behavior can be accomplished by constructing a Hash object that knows what default value to return when the key has not been seen before.

# After
bidding_matrix = Hash.new {|hash, key| hash[key] = [] }
sign_up_topics.each_with_index do |topic, index|
bidding_matrix[topic.id] << bids[index]
end

=====Example 2=====

# Before
teams.each do |team|
next if SignedUpTeam.where(team_id: team.id, is_waitlisted: 0).any?
... (rest of code omitted)
end

This block is extracted from the original implementation that, like many places in the <code>LotteryController</code>, uses <code>next</code> to skip any elements that don't fulfill the imposed condition. It's more reasonable to eliminate out unwanted elements outside of the loop using either <code>select</code> or <code>reject</code> call. Below is the modification made to this block.

# After
teams_not_signed_up = teams.reject {|team| SignedUpTeam.where(team_id: team.id, is_waitlisted: 0).any? }
teams_not_signed_up.each do |team|
... (rest of code omitted)
end

===== Example 3 =====

Same as example 1, the following block uses <code>next</code> to skip unwanted <code>team_user</code>s until the one that meets the condition, that is, who belongs to the specified assignment. This code does the filtering twice, which can be combined with one query.

# Before
TeamsUser.where(user_id: user_id).each do |team_user|
next unless team_user.team.parent_id == assignment.id
team_user.team_user_node.destroy rescue nil
team_user.destroy rescue nil
break
end

We first tried to use <code>find_by</code> with two parameters. <code>find_by</code> works by pulling column values from the model object. Since the <code>TeamsUser</code> model does not have a <code>team</code> attribute (it only stores <code>team_id</code>), this attempt is syntactically illegal.

team_user = TeamsUser.find_by(user_id: user_id, team.parent_id: assignment_id)
team_user.team_user_node.destroy rescue nil
team_user.destroy rescue nil

The way we refactored the code is to use the <code>find</code> method from the <code>Enumerable</code> mixin to get the first element for which the block is not false. In this way, the loop will no longer be necessary since the only thing it does is finding the first matching object.

# After
team_user = TeamsUser.where(user_id: user_id).find{|team_user| team_user.team.parent_id == assignment_id }
team_user.team_user_node.destroy rescue nil
team_user.destroy rescue nil

===== Example 4 =====

Besides pulling unwanted elements out before entering the loop, one can further reduce the verbosity of the code by replacing <code>next if</code> with <code>unless</code> and adding this checking step to the only statement that's inside the loop.

# Before
bidding_matrix.each do |topic_id, value|
next if value.inject(:+).zero?
bidding_matrix_summary << [value.count {|i| i != 0 }, value.inject(:+), topic_id]
end

# After
bidding_matrix.each do |topic_id, value|
bidding_matrix_summary << [value.count {|i| i != 0 }, value.inject(:+), topic_id] unless value.inject(:+).zero?
end

=== Problem 3: Ambiguous Identifiers ===

There were many puzzling variable names in the original <code>lottery_controller.rb</code> file. For example, copied from line 30, the slot with the label <code>users</code> accepts an input of <code>priority_info</code>.
bidding_data = {users: priority_info, max_team_size: assignment.max_team_size}

The <code>priority_info</code> is an array of hashes, each hash stores a pair of user id and bids that the user has made so far. This variable name is not descriptive enough to visualize its content. It is refactored into a new name, <code>users_bidding_info</code>, and the corresponding construction method is therefore named <code>construct_users_bidding_info</code>.

There is another array that has a very similar structure to the <code>users_bidding_info</code>, except that it holds bidding information for teams rather than users. To follow a good naming consistency, this array is granted a new name called <code>teams_bidding_info</code> (previously named <code>team_bids</code>) and the name of the corresponding construction method is changed to <code>construct_teams_bidding_info</code>.

'''Notes'''

We originally considered to change the label <code>ranks</code> to <code>bids</code> because the same identity has been presented in three different forms throughout the file: <code>rank</code>, <code>bid</code>, and <code>priority</code>.
|user| priority_info << {pid: user.id, ranks: bids}
However, such a change could have an impact on the later REST API call to the web service that uses students' bidding data to build teams automatically. We are uncertain about how the bidding data is actually used at the other end so it is not safe to apply such a change.

=== Problem 4: Multi-Purposed Methods ===

The <code>create_new_teams_for_bidding_response</code> method does more than 1 thing, both creating new teams and deleting residual teams that none of their team members remained in their teams. Since it is doing more beyond just what the name implies, it is reasonable to extract the removing part of the code to a new method and place the method call after the call to the <code>create_new_teams_for_bidding_response</code> method.

# Before
create_new_teams_for_bidding_response(teams, assignment, priority_info)
# ^^ <code>create_new_teams_for_bidding_response</code> is responsible for both creation and deletion of AssignmentTeam objects
match_new_teams_to_topics(assignment)

# After
create_new_teams_for_bidding_response(teams, assignment, users_bidding_info)
remove_empty_teams(assignment)
# ^^ <code>remove_empty_teams</code> becomes a separate method
match_new_teams_to_topics(assignment)

=== Problem 5: Unnecessary Looping ===

Many unnecessary looping can be avoided by moving the conditional check out. For example, consider this piece of code.

#Before
bids = []
topics.each do |topic|
bid_record = Bid.find_by(team_id: team.id, topic_id: topic.id)
bids << (bid_record.nil? ? 0 : bid_record.priority ||= 0)
end
team.users.each {|user| priority_info << {pid: user.id, ranks: bids} if bids.uniq != [0] }

The last line shows a looping on the team users that for each user in the team, it pushes its user id along with the generated bids to the <code>priority_info</code> array. If there are 3 users in a team, then the comparison <code>if bids.uniq != [0]</code> always gets executed 3 times while it should only be executed once because at this point the <code>bids</code> variable will not be changed anymore. The solution to this performance drawback would be to change the last line to the code below:

# After
team.users.each {|user| users_bidding_info << {pid: user.id, ranks: bids} } unless bids.uniq == [0]
# Note that priority_info has been renamed to users_bidding_info

Other fixes on the unnecessary looping are demonstrated in examples 2 and 3 of Problem 2. Instead of moving the checking condition out, they each move the filter out so the loop runs in a minimum number of times.

= Testing =

== Test Plan ==

The main objective of improving the current Lottery Controller testing is to increase the code coverage as much as possible. To achieve this object, tests will be written for each newly added method in the controller. The initial tests for these methods will assume expected normal conditions that the controller would face.

The next step to bolster the testing section is to make sure that the tests catch unexpected inputs, so additional tests to account for edge cases and invalid inputs will be implemented. These edge cases include an unintelligent assignment being passed to match_new_teams_to_topics, no teams being passed to the different methods that require teams, and no topics available when there are users still unassigned.

Finally, the last goal is to allow other users to be able to manually test the Lottery Controller themselves. This will be accomplished my implementing User Interface Testing. Users will be able to create their own testing scenarios by creating their testable objects.

Using these steps, the goal is to achieve total test coverage or as much coverage as possible and provide a testable framework for other users to test their own scenarios.

=== Test Case Prerequisites ===

Testing the Lottery Controller requires setting up Factory objects that will be used for each of the test cases. The following steps were taken to set up the test objects:

# Created an intelligent assignment for the students to complete.
# Created 6 students that were assigned this assignment.
# Created 4 topics for the assignment that will be bid on by the students.
# Created 4 teams that the students may form to work cooperatively and bid as one team.
# Created a bid based on a topic each student wants to work on.

=== Test Cases ===

The following test cases were made for each respective method in the Lottery Controller:

# Run_intelligent_assignment:
#* Create an intelligent assignment and expect a successful redirection to the controller’s tree display.
#* Input an invalid case where there are no signed-up teams and expect an error message
# Action_allowed:
#* Login in as an Instructor, Teaching Assistant, and Administrator and expect them to be able to run the bidding operation.
#* Login as a student and visitor and expect them not to be able to run the bidding operation.
# Construct_users_bidding_info:
#* Set up topics for teams to bid on. Set up teams with bids for those topics. Execute the method. Expect to return the bidding information of the teams with bids.
#* Setup teams without any bids. Setup teams that are not signed up. Execute the method. Expect to exclude the teams that are not signed-up.
# Construct_teams_bidding_info:
#* Setup unassigned teams. Setup topics for those teams. Execute the method. Expect that information to be stored in a hash table.
# Create_new_teams_for_bidding_response:
#* Set up 3 students. Set up bids for those students. Set those students to two separate teams. Execute the method. Expect the user count to remain the same. Expect team count to increase by 2.
# Match_new_teams_to_topics:
#* Set up an unintelligent assignment. Set up a team. Set up that team’s bidding information. Execute the method. Expect the assignment to become intelligent.
#* Set up an unintelligent assignment. Set up a team. Set up that team’s bidding information. Set up an invalid path. Execute the method. Expect an error message and assignment to remain unintelligent.
# Remove_user_from_previous_team:
#* Set up users. Set up a team with those users. Execute the method. Expect the team’s user count to decrease. Expect the removed user’s path to return nil. Expect the remaining users’ paths to return their path.
# Remove_empty_teams:
#* Create 1 team that has no users. Execute the method. Expect the team to be deleted. Expect the team count to decrease.
# Assign_available_slots:
#* Set up a topic. Set up a team with bidding information. Execute the method. Expect the number of signed up teams to increase.
# Merge_bids_from_different_previous_teams:
#* Set up topics. Set up teams. Set up users on those teams. Set up user bids for each created user. Execute the method. Expect bid count to increase. Expect the new bid information to be a combined bid of the new team.

== RSpec Testing ==

=== Existing Tests ===
There were existing RSpec tests for the Lottery Controller, but the tests only provided about 10% coverage of the code in the controller. The test coverage was determined by the Coveralls gem. Most of the existing tests actually didn't even test the functionality of the lottery controller. The existing tests included the following:

# Testing an http get request to google.com and expecting a 200 response code with the content-type of application/json This was an attempt at testing the web service used by the lottery controller
# Checking if an assignment had a correctly set is_intelligent attribute, but it didn't call any methods of the lottery controller
# Creating new team for a bid
# Sorting unassigned teams, but it didn't call any methods of the lottery controller

=== After Refactoring ===

After refactoring the Lottery Controller, the long methods were broken down into smaller, more simpler methods. This required a new RSpec test for each of the new methods, but it allowed for increased code coverage. In order to effectively test the rest of the controller, testing objects that mimic the expected application objects were created. The testing objects were created from the factories.rb file using the factory_bot_rails gem and the double feature of the RSpec gem. The objects are also recreated before each test so that the tests are independent from one another to truly validate the different testing sections. The added tests were able to expand the code coverage from about 10% to 99%. The new tests that were added for each of the newly condensed methods include testing for the following:

# Constructing the hash table for the users' bidding information
# A more exhaustive running Intelligent Assignment method
# Generate the hash for a whole team's bidding information
# Match new teams to a topic based on their bids
# Remove a user from a team
# Destroying teams that have no users
# Merging the bids from teams that have merged together so that they have a weighted uniform bid
# Assigning open topics to teams that do not have a topic yet
# Matching teams to topics based on their weighted bids

=== Running the Tests ===
These tests can be executed by utilizing the RSpec command in the Command Prompt window by entering:

<code> user:expertiza $rspec spec/controllers/lottery_controller_spec.rb </code>

== UI Testing (For Reviewers) ==

By following the below steps, you can manually test our refactored <code>LotteryController</code> class.

'''Note:''' The previous implementation does the intelligent assignment on a team basis. That is, students who do not belong to any team will not get assigned to topics (see method <code>construct_users_bidding_info</code>). We intended not to disrupt this behavior since it's beyond the scope of this refactoring.

# Visit [xxx]. Enter the credentials: '''Username:''' instructor6 '''Password:''' password
# From the menubar, hover on the '''Manage''' tab, and select '''Assignments'''.
# Scroll down the page and find the assignment with the name '''New test assignment'''. The orange book icon on the right links to the <code>run_intelligent_assignment</code> method of the <code>LotteryController</code> class. [[File:E2012_4.png]] When you click on that, you should receive an error message claiming "500 Internal Server Error". This is because this assignment has not properly set up its participants. To get the intelligent assignment to run, you should click on the pencil icon, navigate to "Other stuff -> Add participant", and add the student by entering its user_id to the textbox next to the label "Enter a user login:".

'''Additional configuring:'''

# Under the '''General''' tab, mark the checkbox '''Has teams''' as true and set the maximum number of members per team to number that makes sense (e.g. our demonstration uses 3).
# You may also want to login to the account of each student you add (they all have "**password**" as their passwords), and help that student bid the topics just like how you did with our OSS project! The algorithm works more effectively when there are bids presented in the system. And by signing up topics, you indirectly create teams for them. We know this process can be very tedious, so thank you ahead for your patience!

Here are some data we used in our recording, you are free to come up with your own!

| User id | First Choice | Second Choice | Third Choice |
| ---------------------------------- | ------------ | ------------- | ------------ |
| student558 | 1 | | |
| student559, student562, student563 | 2 | 3 | 1 |
| student560, student561 | 2 | 1 | |
| student564 | 1 | 2 | |
| student565 | 3 | | |
| student566 | 2 | | |

Return to the '''Assignments''' page after you set up the New test assignment, and retry the orange book icon. You should be indicated that the intelligent assignment has run successfully. You can check the teaming result under the '''Topics''' tab of the edit page. The above data generate the teaming result of the following.

[[File:E2012_5.png]]

= Future Improvements =

# Improving the RSpec test for assigning open topics for teams without topics. The new test only checks for 1 open assignment and 1 unassigned team. The method is capable of assigning multiple open topics for multiple unassigned teams, but testing for this would require more Factory objects for assignments and teams. While the new test provides code coverage for the entire method, the nested loops currently only run through a single time before completion, which is not always the case in a realistic setting.
# Improving the time-space complexity for the method that merges bids from teams that are merging together. The existing method requires iterating through each member's bidding information to create a hash map. Then there is a nest iteration for the included topics. Afterwards there is an additional iterations through to exclude topics without bids.

CSC/ECE 517 Spring 2020/E2012. refactor lottery controller.rb

2020-03-30T23:51:49Z

Thwinter: /* Test Plan */

= Introduction =

== Expertiza Background ==

Expertiza is an open-source project written using the Ruby on Rails framework. The Expertiza platform aims to improve student learning by using active and cooperative learning, allowing distance education students to participate in active learning, and by discouraging plagiarism through the use of multiple deadlines. Expertiza helps to improve teaching and resource utilization by having students contribute to the creation of course materials and through peer-graded assignments. Expertiza manages objects related to a course such as instructors, TAs, students, and other users. It also manages assignments, teams, signup topics, rubrics, peer grading, and peer evaluations.

== Lottery Controller Background ==

An assignment can have a list of signup topics. The teams associated with the assignment can bid on the signup topics to try and get assigned one they find more favorable. At a high level, the lottery controller assigns teams to topics based on the priorities the team gave to each signup topic during the bidding process.

In more detail, each student starts off on a team of size 1. They can bid on signup topics by arranging them in priority order. A team can invite other users to join their team. If student2 from Team B joins Team A which includes student1, student2 will lose their bids and take on the bids of TeamA and student1.
When the lottery controller is called to run its method run_intelligent_assignment, a web service takes the bidding data from each team and returns a new list of teams, each of which is closer to the maximum team size specified for the assignment. The web service combines teams together that have similar
bid data and then assigns those new teams to topics, giving each team their top bid on a topic that hasn't been assigned yet. Teams with larger team sizes and more bids are assigned their topics first.

== Problem Statement ==

Code Climate currently grades <code>lottery_controller.rb</code> at a C level, finding issues with the controller's long methods and complex statements. Most methods are around 40 lines and hard to decipher with a quick look. Also, the file currently has only 10% test coverage. Our goal is to resolve the Code Climate issues, make methods simpler to understand, and write new tests to improve test coverage.

== Tasks Identified ==

# Cut down on code in run_intelligent_assignment method by moving some code to helper methods
# Cut down on code in create_new_teams_for_bidding_response by moving some code to helper methods
# Cut down on code in match_new_teams_to_topics by moving some code to helper methods
# Reduce cognitive complexity of merge_bids_from_different_previous_teams method
# Add additional comments to clarify newly written methods
# Add additional RSpec tests to increase the coverage percentage

= Implementation =

We did the following refactoring to the <code>LotteryController</code> class:

# Added 6 helper methods (<code>construct_users_bidding_info</code>, <code>construct_teams_bidding_info</code>, <code>remove_user_from_previous_team</code>, <code>remove_empty_teams</code>, <code>assign_available_slots</code>) that each represent a specific and meaningful step
# Replaced large chunks of code with a small number of method calls
# Reduced the cognitive complexity by reducing the level of nested loops
# Optimized the performance by moving conditional checking and input filtering outside of loops, where appropriate
# Renamed some variable names to make their purpose clearer
# Refactored the parameter list to give each method the least amount of information needed to complete its task
# Added addition comments and fixed some typos

Here are some examples of the problems we identified and the solutions we found to these problems.

=== Problem 1: Long Methods ===

The code climate analysis tool identified two long methods exceeding 25 lines of code per method, <code>run_intelligent_assignment</code> and <code>match_new_teams_to_topics</code>. While some chunks of code are short and intuitive, others can be pulled out and put into helper methods with more meaningful method names.

====Solutions ====

=====run_intelligent_assignment=====

1. Put lines 19-29 into a separate method called <code>construct_users_bidding_info</code> that is responsible for generating user bidding information hash for students on a team who haven't signed up for a topic yet

[[Media:E2012_1.png| view code changes]]

2. Place the code closest to where it is needed. e.g. place line 32 (the <code>url</code> variable initialization) inside the <code>begin/rescue</code> block because it is considered an integral part of the REST API call.

[[Media:E2012_2.png| view code changes]]

===== match_new_teams_to_topics =====

1. Conclude lines 147-157 with a new method <code>construct_teams_bidding_info</code> that is responsible for generating team bidding information needed for the <code>assign_available_slots</code> method.

2. Conclude lines 161-169 with a new method <code>assign_available_slots</code> that uses the result returned from <code>construct_teams_bidding_info</code> to match unassigned teams to available topics.

These two steps greatly shorten the <code>match_new_teams_to_topics</code> method and make the workflow more intuitive to code maintainers.

[[Media:E2012_3.png| view code changes]]

=== Problem 2: Verbose Code ===

Verbosity contributes to more lines of codes and increases the cognitive complexity, which is the measure of how difficult a unit of code is to intuitively understand. Method extraction can't be applied to every long method as some methods only do one thing. For these methods, one can keep them simple with alternative language syntax and other ways of thinking of the behavior.

==== Solutions ====

===== Example 1 =====

# Before
bidding_matrix = {}
assignment.sign_up_topics.each_with_index do |topic, index|
bidding_matrix[topic.id] = [] unless bidding_matrix[topic.id]
bidding_matrix[topic.id] << bids[index]
end

The above code segment ensures that <code>bidding_matrix[topic.id]</code> returns an array object before pushing a new value to it. It is inelegant both in its readability and the time wasted to visit each <code>bidding_matrix[topic.id]</code> twice. The same behavior can be accomplished by constructing a Hash object that knows what default value to return when the key has not been seen before.

# After
bidding_matrix = Hash.new {|hash, key| hash[key] = [] }
sign_up_topics.each_with_index do |topic, index|
bidding_matrix[topic.id] << bids[index]
end

=====Example 2=====

# Before
teams.each do |team|
next if SignedUpTeam.where(team_id: team.id, is_waitlisted: 0).any?
... (rest of code omitted)
end

This block is extracted from the original implementation that, like many places in the <code>LotteryController</code>, uses <code>next</code> to skip any elements that don't fulfill the imposed condition. It's more reasonable to eliminate out unwanted elements outside of the loop using either <code>select</code> or <code>reject</code> call. Below is the modification made to this block.

# After
teams_not_signed_up = teams.reject {|team| SignedUpTeam.where(team_id: team.id, is_waitlisted: 0).any? }
teams_not_signed_up.each do |team|
... (rest of code omitted)
end

===== Example 3 =====

Same as example 1, the following block uses <code>next</code> to skip unwanted <code>team_user</code>s until the one that meets the condition, that is, who belongs to the specified assignment. This code does the filtering twice, which can be combined with one query.

# Before
TeamsUser.where(user_id: user_id).each do |team_user|
next unless team_user.team.parent_id == assignment.id
team_user.team_user_node.destroy rescue nil
team_user.destroy rescue nil
break
end

We first tried to use <code>find_by</code> with two parameters. <code>find_by</code> works by pulling column values from the model object. Since the <code>TeamsUser</code> model does not have a <code>team</code> attribute (it only stores <code>team_id</code>), this attempt is syntactically illegal.

team_user = TeamsUser.find_by(user_id: user_id, team.parent_id: assignment_id)
team_user.team_user_node.destroy rescue nil
team_user.destroy rescue nil

The way we refactored the code is to use the <code>find</code> method from the <code>Enumerable</code> mixin to get the first element for which the block is not false. In this way, the loop will no longer be necessary since the only thing it does is finding the first matching object.

# After
team_user = TeamsUser.where(user_id: user_id).find{|team_user| team_user.team.parent_id == assignment_id }
team_user.team_user_node.destroy rescue nil
team_user.destroy rescue nil

===== Example 4 =====

Besides pulling unwanted elements out before entering the loop, one can further reduce the verbosity of the code by replacing <code>next if</code> with <code>unless</code> and adding this checking step to the only statement that's inside the loop.

# Before
bidding_matrix.each do |topic_id, value|
next if value.inject(:+).zero?
bidding_matrix_summary << [value.count {|i| i != 0 }, value.inject(:+), topic_id]
end

# After
bidding_matrix.each do |topic_id, value|
bidding_matrix_summary << [value.count {|i| i != 0 }, value.inject(:+), topic_id] unless value.inject(:+).zero?
end

=== Problem 3: Ambiguous Identifiers ===

There were many puzzling variable names in the original <code>lottery_controller.rb</code> file. For example, copied from line 30, the slot with the label <code>users</code> accepts an input of <code>priority_info</code>.
bidding_data = {users: priority_info, max_team_size: assignment.max_team_size}

The <code>priority_info</code> is an array of hashes, each hash stores a pair of user id and bids that the user has made so far. This variable name is not descriptive enough to visualize its content. It is refactored into a new name, <code>users_bidding_info</code>, and the corresponding construction method is therefore named <code>construct_users_bidding_info</code>.

There is another array that has a very similar structure to the <code>users_bidding_info</code>, except that it holds bidding information for teams rather than users. To follow a good naming consistency, this array is granted a new name called <code>teams_bidding_info</code> (previously named <code>team_bids</code>) and the name of the corresponding construction method is changed to <code>construct_teams_bidding_info</code>.

'''Notes'''

We originally considered to change the label <code>ranks</code> to <code>bids</code> because the same identity has been presented in three different forms throughout the file: <code>rank</code>, <code>bid</code>, and <code>priority</code>.
|user| priority_info << {pid: user.id, ranks: bids}
However, such a change could have an impact on the later REST API call to the web service that uses students' bidding data to build teams automatically. We are uncertain about how the bidding data is actually used at the other end so it is not safe to apply such a change.

=== Problem 4: Multi-Purposed Methods ===

The <code>create_new_teams_for_bidding_response</code> method does more than 1 thing, both creating new teams and deleting residual teams that none of their team members remained in their teams. Since it is doing more beyond just what the name implies, it is reasonable to extract the removing part of the code to a new method and place the method call after the call to the <code>create_new_teams_for_bidding_response</code> method.

# Before
create_new_teams_for_bidding_response(teams, assignment, priority_info)
# ^^ <code>create_new_teams_for_bidding_response</code> is responsible for both creation and deletion of AssignmentTeam objects
match_new_teams_to_topics(assignment)

# After
create_new_teams_for_bidding_response(teams, assignment, users_bidding_info)
remove_empty_teams(assignment)
# ^^ <code>remove_empty_teams</code> becomes a separate method
match_new_teams_to_topics(assignment)

=== Problem 5: Unnecessary Looping ===

Many unnecessary looping can be avoided by moving the conditional check out. For example, consider this piece of code.

#Before
bids = []
topics.each do |topic|
bid_record = Bid.find_by(team_id: team.id, topic_id: topic.id)
bids << (bid_record.nil? ? 0 : bid_record.priority ||= 0)
end
team.users.each {|user| priority_info << {pid: user.id, ranks: bids} if bids.uniq != [0] }

The last line shows a looping on the team users that for each user in the team, it pushes its user id along with the generated bids to the <code>priority_info</code> array. If there are 3 users in a team, then the comparison <code>if bids.uniq != [0]</code> always gets executed 3 times while it should only be executed once because at this point the <code>bids</code> variable will not be changed anymore. The solution to this performance drawback would be to change the last line to the code below:

# After
team.users.each {|user| users_bidding_info << {pid: user.id, ranks: bids} } unless bids.uniq == [0]
# Note that priority_info has been renamed to users_bidding_info

Other fixes on the unnecessary looping are demonstrated in examples 2 and 3 of Problem 2. Instead of moving the checking condition out, they each move the filter out so the loop runs in a minimum number of times.

= Testing =

== Test Plan ==

The main objective of improving the current Lottery Controller testing is to increase the code coverage as much as possible. To achieve this object, tests will be written for each newly added method in the controller. The initial tests for these methods will assume expected normal conditions that the controller would face.

The next step to bolster the testing section is to make sure that the tests catch unexpected inputs, so additional tests to account for edge cases and invalid inputs will be implemented. These edge cases include an unintelligent assignment being passed to match_new_teams_to_topics, no teams being passed to the different methods that require teams, and no topics available when there are users still unassigned.

Finally, the last goal is to allow other users to be able to manually test the Lottery Controller themselves. This will be accomplished my implementing User Interface Testing. Users will be able to create their own testing scenarios by creating their testable objects.

Using these steps, the goal is to achieve total test coverage or as much coverage as possible and provide a testable framework for other users to test their own scenarios.

=== Test Case Prerequisites ===

Testing the Lottery Controller requires setting up Factory objects that will be used for each of the test cases. The following steps were taken to set up the test objects:

# Created an intelligent assignment for the students to complete.
# Created 6 students that were assigned this assignment.
# Created 4 topics for the assignment that will be bid on by the students.
# Created 4 teams that the students may form to work cooperatively and bid as one team.
# Created a bid based on a topic each student wants to work on.

=== Test Cases ===

== RSpec Testing ==

=== Existing Tests ===
There were existing RSpec tests for the Lottery Controller, but the tests only provided about 10% coverage of the code in the controller. The test coverage was determined by the Coveralls gem. Most of the existing tests actually didn't even test the functionality of the lottery controller. The existing tests included the following:

# Testing an http get request to google.com and expecting a 200 response code with the content-type of application/json This was an attempt at testing the web service used by the lottery controller
# Checking if an assignment had a correctly set is_intelligent attribute, but it didn't call any methods of the lottery controller
# Creating new team for a bid
# Sorting unassigned teams, but it didn't call any methods of the lottery controller

=== After Refactoring ===

After refactoring the Lottery Controller, the long methods were broken down into smaller, more simpler methods. This required a new RSpec test for each of the new methods, but it allowed for increased code coverage. In order to effectively test the rest of the controller, testing objects that mimic the expected application objects were created. The testing objects were created from the factories.rb file using the factory_bot_rails gem and the double feature of the RSpec gem. The objects are also recreated before each test so that the tests are independent from one another to truly validate the different testing sections. The added tests were able to expand the code coverage from about 10% to 99%. The new tests that were added for each of the newly condensed methods include testing for the following:

# Constructing the hash table for the users' bidding information
# A more exhaustive running Intelligent Assignment method
# Generate the hash for a whole team's bidding information
# Match new teams to a topic based on their bids
# Remove a user from a team
# Destroying teams that have no users
# Merging the bids from teams that have merged together so that they have a weighted uniform bid
# Assigning open topics to teams that do not have a topic yet
# Matching teams to topics based on their weighted bids

=== Running the Tests ===
These tests can be executed by utilizing the RSpec command in the Command Prompt window by entering:

<code> user:expertiza $rspec spec/controllers/lottery_controller_spec.rb </code>

== UI Testing (For Reviewers) ==

By following the below steps, you can manually test our refactored <code>LotteryController</code> class.

'''Note:''' The previous implementation does the intelligent assignment on a team basis. That is, students who do not belong to any team will not get assigned to topics (see method <code>construct_users_bidding_info</code>). We intended not to disrupt this behavior since it's beyond the scope of this refactoring.

# Visit [xxx]. Enter the credentials: '''Username:''' instructor6 '''Password:''' password
# From the menubar, hover on the '''Manage''' tab, and select '''Assignments'''.
# Scroll down the page and find the assignment with the name '''New test assignment'''. The orange book icon on the right links to the <code>run_intelligent_assignment</code> method of the <code>LotteryController</code> class. [[File:E2012_4.png]] When you click on that, you should receive an error message claiming "500 Internal Server Error". This is because this assignment has not properly set up its participants. To get the intelligent assignment to run, you should click on the pencil icon, navigate to "Other stuff -> Add participant", and add the student by entering its user_id to the textbox next to the label "Enter a user login:".

'''Additional configuring:'''

# Under the '''General''' tab, mark the checkbox '''Has teams''' as true and set the maximum number of members per team to number that makes sense (e.g. our demonstration uses 3).
# You may also want to login to the account of each student you add (they all have "**password**" as their passwords), and help that student bid the topics just like how you did with our OSS project! The algorithm works more effectively when there are bids presented in the system. And by signing up topics, you indirectly create teams for them. We know this process can be very tedious, so thank you ahead for your patience!

Here are some data we used in our recording, you are free to come up with your own!

| User id | First Choice | Second Choice | Third Choice |
| ---------------------------------- | ------------ | ------------- | ------------ |
| student558 | 1 | | |
| student559, student562, student563 | 2 | 3 | 1 |
| student560, student561 | 2 | 1 | |
| student564 | 1 | 2 | |
| student565 | 3 | | |
| student566 | 2 | | |

Return to the '''Assignments''' page after you set up the New test assignment, and retry the orange book icon. You should be indicated that the intelligent assignment has run successfully. You can check the teaming result under the '''Topics''' tab of the edit page. The above data generate the teaming result of the following.

[[File:E2012_5.png]]

= Future Improvements =

# Improving the RSpec test for assigning open topics for teams without topics. The new test only checks for 1 open assignment and 1 unassigned team. The method is capable of assigning multiple open topics for multiple unassigned teams, but testing for this would require more Factory objects for assignments and teams. While the new test provides code coverage for the entire method, the nested loops currently only run through a single time before completion, which is not always the case in a realistic setting.
# Improving the time-space complexity for the method that merges bids from teams that are merging together. The existing method requires iterating through each member's bidding information to create a hash map. Then there is a nest iteration for the included topics. Afterwards there is an additional iterations through to exclude topics without bids.

CSC/ECE 517 Spring 2020/E2012. refactor lottery controller.rb

2020-03-30T20:20:18Z

Thwinter: /* Existing Tests */

= Introduction =

== Expertiza Background ==

Expertiza is an open-source project written using the Ruby on Rails framework. The Expertiza platform aims to improve student learning by using active and cooperative learning, allowing distance education students to participate in active learning, and by discouraging plagiarism through the use of multiple deadlines. Expertiza helps to improve teaching and resource utilization by having students contribute to the creation of course materials and through peer-graded assignments. Expertiza manages objects related to a course such as instructors, TAs, students, and other users. It also manages assignments, teams, signup topics, rubrics, peer grading, and peer evaluations.

== Lottery Controller Background ==

An assignment can have a list of signup topics. The teams associated with the assignment can bid on the signup topics to try and get assigned one they find more favorable. At a high level, the lottery controller assigns teams to topics based on the priorities the team gave to each signup topic during the bidding process.

In more detail, each student starts off on a team of size 1. They can bid on signup topics by arranging them in priority order. A team can invite other users to join their team. If student2 from Team B joins Team A which includes student1, student2 will lose their bids and take on the bids of TeamA and student1.
When the lottery controller is called to run its method run_intelligent_assignment, a web service takes the bidding data from each team and returns a new list of teams, each of which is closer to the maximum team size specified for the assignment. The web service combines teams together that have similar
bid data and then assigns those new teams to topics, giving each team their top bid on a topic that hasn't been assigned yet. Teams with larger team sizes and more bids are assigned their topics first.

== Problem Statement ==

Code Climate currently grades <code>lottery_controller.rb</code> at a C level, finding issues with the controller's long methods and complex statements. Most methods are around 40 lines and hard to decipher with a quick look. Also, the file currently has only 10% test coverage. Our goal is to resolve the Code Climate issues, make methods simpler to understand, and write new tests to improve test coverage.

== Tasks Identified ==

# Cut down on code in run_intelligent_assignment method by moving some code to helper methods
# Cut down on code in create_new_teams_for_bidding_response by moving some code to helper methods
# Cut down on code in match_new_teams_to_topics by moving some code to helper methods
# Reduce cognitive complexity of merge_bids_from_different_previous_teams method
# Add additional comments to clarify newly written methods
# Add additional RSpec tests to increase the coverage percentage

= Implementation =

We did the following refactoring to the <code>LotteryController</code> class:

# Added 6 helper methods (<code>construct_users_bidding_info</code>, <code>construct_teams_bidding_info</code>, <code>remove_user_from_previous_team</code>, <code>remove_empty_teams</code>, <code>assign_available_slots</code>) that each represent a specific and meaningful step
# Replaced large chunks of code with a small number of method calls
# Reduced the cognitive complexity by reducing the level of nested loops
# Optimized the performance by moving conditional checking and input filtering outside of loops, where appropriate
# Renamed some variable names to make their purpose clearer
# Refactored the parameter list to give each method the least amount of information needed to complete its task
# Added addition comments and fixed some typos

Here are some examples of the problems we identified and the solutions we found to these problems.

=== Problem 1: Long Methods ===

The code climate analysis tool identified two long methods exceeding 25 lines of code per method, <code>run_intelligent_assignment</code> and <code>match_new_teams_to_topics</code>. While some chunks of code are short and intuitive, others can be pulled out and put into helper methods with more meaningful method names.

====Solutions ====

=====run_intelligent_assignment=====

1. Put lines 19-29 into a separate method called <code>construct_users_bidding_info</code> that is responsible for generating user bidding information hash for students on a team who haven't signed up for a topic yet

[[Media:E2012_1.png| view code changes]]

2. Place the code closest to where it is needed. e.g. place line 32 (the <code>url</code> variable initialization) inside the <code>begin/rescue</code> block because it is considered an integral part of the REST API call.

[[Media:E2012_2.png| view code changes]]

===== match_new_teams_to_topics =====

1. Conclude lines 147-157 with a new method <code>construct_teams_bidding_info</code> that is responsible for generating team bidding information needed for the <code>assign_available_slots</code> method.

2. Conclude lines 161-169 with a new method <code>assign_available_slots</code> that uses the result returned from <code>construct_teams_bidding_info</code> to match unassigned teams to available topics.

These two steps greatly shorten the <code>match_new_teams_to_topics</code> method and make the workflow more intuitive to code maintainers.

[[Media:E2012_3.png| view code changes]]

=== Problem 2: Verbose Code ===

Verbosity contributes to more lines of codes and increases the cognitive complexity, which is the measure of how difficult a unit of code is to intuitively understand. Method extraction can't be applied to every long method as some methods only do one thing. For these methods, one can keep them simple with alternative language syntax and other ways of thinking of the behavior.

==== Solutions ====

===== Example 1 =====

# Before
bidding_matrix = {}
assignment.sign_up_topics.each_with_index do |topic, index|
bidding_matrix[topic.id] = [] unless bidding_matrix[topic.id]
bidding_matrix[topic.id] << bids[index]
end

The above code segment ensures that <code>bidding_matrix[topic.id]</code> returns an array object before pushing a new value to it. It is inelegant both in its readability and the time wasted to visit each <code>bidding_matrix[topic.id]</code> twice. The same behavior can be accomplished by constructing a Hash object that knows what default value to return when the key has not been seen before.

# After
bidding_matrix = Hash.new {|hash, key| hash[key] = [] }
sign_up_topics.each_with_index do |topic, index|
bidding_matrix[topic.id] << bids[index]
end

=====Example 2=====

# Before
teams.each do |team|
next if SignedUpTeam.where(team_id: team.id, is_waitlisted: 0).any?
... (rest of code omitted)
end

This block is extracted from the original implementation that, like many places in the <code>LotteryController</code>, uses <code>next</code> to skip any elements that don't fulfill the imposed condition. It's more reasonable to eliminate out unwanted elements outside of the loop using either <code>select</code> or <code>reject</code> call. Below is the modification made to this block.

# After
teams_not_signed_up = teams.reject {|team| SignedUpTeam.where(team_id: team.id, is_waitlisted: 0).any? }
teams_not_signed_up.each do |team|
... (rest of code omitted)
end

===== Example 3 =====

Same as example 1, the following block uses <code>next</code> to skip unwanted <code>team_user</code>s until the one that meets the condition, that is, who belongs to the specified assignment. This code does the filtering twice, which can be combined with one query.

# Before
TeamsUser.where(user_id: user_id).each do |team_user|
next unless team_user.team.parent_id == assignment.id
team_user.team_user_node.destroy rescue nil
team_user.destroy rescue nil
break
end

We first tried to use <code>find_by</code> with two parameters. <code>find_by</code> works by pulling column values from the model object. Since the <code>TeamsUser</code> model does not have a <code>team</code> attribute (it only stores <code>team_id</code>), this attempt is syntactically illegal.

team_user = TeamsUser.find_by(user_id: user_id, team.parent_id: assignment_id)
team_user.team_user_node.destroy rescue nil
team_user.destroy rescue nil

The way we refactored the code is to use the <code>find</code> method from the <code>Enumerable</code> mixin to get the first element for which the block is not false. In this way, the loop will no longer be necessary since the only thing it does is finding the first matching object.

# After
team_user = TeamsUser.where(user_id: user_id).find{|team_user| team_user.team.parent_id == assignment_id }
team_user.team_user_node.destroy rescue nil
team_user.destroy rescue nil

===== Example 4 =====

Besides pulling unwanted elements out before entering the loop, one can further reduce the verbosity of the code by replacing <code>next if</code> with <code>unless</code> and adding this checking step to the only statement that's inside the loop.

# Before
bidding_matrix.each do |topic_id, value|
next if value.inject(:+).zero?
bidding_matrix_summary << [value.count {|i| i != 0 }, value.inject(:+), topic_id]
end

# After
bidding_matrix.each do |topic_id, value|
bidding_matrix_summary << [value.count {|i| i != 0 }, value.inject(:+), topic_id] unless value.inject(:+).zero?
end

=== Problem 3: Ambiguous Identifiers ===

There were many puzzling variable names in the original <code>lottery_controller.rb</code> file. For example, copied from line 30, the slot with the label <code>users</code> accepts an input of <code>priority_info</code>.
bidding_data = {users: priority_info, max_team_size: assignment.max_team_size}

The <code>priority_info</code> is an array of hashes, each hash stores a pair of user id and bids that the user has made so far. This variable name is not descriptive enough to visualize its content. It is refactored into a new name, <code>users_bidding_info</code>, and the corresponding construction method is therefore named <code>construct_users_bidding_info</code>.

There is another array that has a very similar structure to the <code>users_bidding_info</code>, except that it holds bidding information for teams rather than users. To follow a good naming consistency, this array is granted a new name called <code>teams_bidding_info</code> (previously named <code>team_bids</code>) and the name of the corresponding construction method is changed to <code>construct_teams_bidding_info</code>.

'''Notes'''

We originally considered to change the label <code>ranks</code> to <code>bids</code> because the same identity has been presented in three different forms throughout the file: <code>rank</code>, <code>bid</code>, and <code>priority</code>.
|user| priority_info << {pid: user.id, ranks: bids}
However, such a change could have an impact on the later REST API call to the web service that uses students' bidding data to build teams automatically. We are uncertain about how the bidding data is actually used at the other end so it is not safe to apply such a change.

=== Problem 4: Multi-Purposed Methods ===

The <code>create_new_teams_for_bidding_response</code> method does more than 1 thing, both creating new teams and deleting residual teams that none of their team members remained in their teams. Since it is doing more beyond just what the name implies, it is reasonable to extract the removing part of the code to a new method and place the method call after the call to the <code>create_new_teams_for_bidding_response</code> method.

# Before
create_new_teams_for_bidding_response(teams, assignment, priority_info)
# ^^ <code>create_new_teams_for_bidding_response</code> is responsible for both creation and deletion of AssignmentTeam objects
match_new_teams_to_topics(assignment)

# After
create_new_teams_for_bidding_response(teams, assignment, users_bidding_info)
remove_empty_teams(assignment)
# ^^ <code>remove_empty_teams</code> becomes a separate method
match_new_teams_to_topics(assignment)

=== Problem 5: Unnecessary Looping ===

Many unnecessary looping can be avoided by moving the conditional check out. For example, consider this piece of code.

#Before
bids = []
topics.each do |topic|
bid_record = Bid.find_by(team_id: team.id, topic_id: topic.id)
bids << (bid_record.nil? ? 0 : bid_record.priority ||= 0)
end
team.users.each {|user| priority_info << {pid: user.id, ranks: bids} if bids.uniq != [0] }

The last line shows a looping on the team users that for each user in the team, it pushes its user id along with the generated bids to the <code>priority_info</code> array. If there are 3 users in a team, then the comparison <code>if bids.uniq != [0]</code> always gets executed 3 times while it should only be executed once because at this point the <code>bids</code> variable will not be changed anymore. The solution to this performance drawback would be to change the last line to the code below:

# After
team.users.each {|user| users_bidding_info << {pid: user.id, ranks: bids} } unless bids.uniq == [0]
# Note that priority_info has been renamed to users_bidding_info

Other fixes on the unnecessary looping are demonstrated in examples 2 and 3 of Problem 2. Instead of moving the checking condition out, they each move the filter out so the loop runs in a minimum number of times.

= Testing =

== Test Plan ==

The main objective of improving the current Lottery Controller testing is to increase the code coverage as much as possible. To achieve this object, tests will be written for each newly added method in the controller. The initial tests for these methods will assume expected normal conditions that the controller would face.

The next step to bolster the testing section is to make sure that the tests catch unexpected inputs, so additional tests to account for edge cases and invalid inputs will be implemented. These edge cases include an unintelligent assignment being passed to match_new_teams_to_topics, no teams being passed to the different methods that require teams, and no topics available when there are users still unassigned.

Finally, the last goal is to allow other users to be able to manually test the Lottery Controller themselves. This will be accomplished my implementing User Interface Testing. Users will be able to create their own testing scenarios by creating their testable objects.

Using these steps, the goal is to achieve total test coverage or as much coverage as possible and provide a testable framework for other users to test their own scenarios.

== RSpec Testing ==

=== Existing Tests ===
There were existing RSpec tests for the Lottery Controller, but the tests only provided about 10% coverage of the code in the controller. The test coverage was determined by the Coveralls gem. Most of the existing tests actually didn't even test the functionality of the lottery controller. The existing tests included the following:

# Testing an http get request to google.com and expecting a 200 response code with the content-type of application/json This was an attempt at testing the web service used by the lottery controller
# Checking if an assignment had a correctly set is_intelligent attribute, but it didn't call any methods of the lottery controller
# Creating new team for a bid
# Sorting unassigned teams, but it didn't call any methods of the lottery controller

=== After Refactoring ===

After refactoring the Lottery Controller, the long methods were broken down into smaller, more simpler methods. This required a new RSpec test for each of the new methods, but it allowed for increased code coverage. In order to effectively test the rest of the controller, testing objects that mimic the expected application objects were created. The testing objects were created from the factories.rb file using the factory_bot_rails gem and the double feature of the RSpec gem. The objects are also recreated before each test so that the tests are independent from one another to truly validate the different testing sections. The added tests were able to expand the code coverage from about 10% to 99%. The new tests that were added for each of the newly condensed methods include testing for the following:

# Constructing the hash table for the users' bidding information
# A more exhaustive running Intelligent Assignment method
# Generate the hash for a whole team's bidding information
# Match new teams to a topic based on their bids
# Remove a user from a team
# Destroying teams that have no users
# Merging the bids from teams that have merged together so that they have a weighted uniform bid
# Assigning open topics to teams that do not have a topic yet
# Matching teams to topics based on their weighted bids

=== Running the Tests ===
These tests can be executed by utilizing the RSpec command in the Command Prompt window by entering:

<code> user:expertiza $rspec spec/controllers/lottery_controller_spec.rb </code>

== UI Testing (For Reviewers) ==

By following the below steps, you can manually test our refactored <code>LotteryController</code> class.

'''Note:''' The previous implementation does the intelligent assignment on a team basis. That is, students who do not belong to any team will not get assigned to topics (see method <code>construct_users_bidding_info</code>). We intended not to disrupt this behavior since it's beyond the scope of this refactoring.

# Visit [xxx]. Enter the credentials: '''Username:''' instructor6 '''Password:''' password
# From the menubar, hover on the '''Manage''' tab, and select '''Assignments'''.
# Scroll down the page and find the assignment with the name '''New test assignment'''. The orange book icon on the right links to the <code>run_intelligent_assignment</code> method of the <code>LotteryController</code> class. [[File:E2012_4.png]] When you click on that, you should receive an error message claiming "500 Internal Server Error". This is because this assignment has not properly set up its participants. To get the intelligent assignment to run, you should click on the pencil icon, navigate to "Other stuff -> Add participant", and add the student by entering its user_id to the textbox next to the label "Enter a user login:".

'''Additional configuring:'''

# Under the '''General''' tab, mark the checkbox '''Has teams''' as true and set the maximum number of members per team to number that makes sense (e.g. our demonstration uses 3).
# You may also want to login to the account of each student you add (they all have "**password**" as their passwords), and help that student bid the topics just like how you did with our OSS project! The algorithm works more effectively when there are bids presented in the system. And by signing up topics, you indirectly create teams for them. We know this process can be very tedious, so thank you ahead for your patience!

Here are some data we used in our recording, you are free to come up with your own!

| User id | First Choice | Second Choice | Third Choice |
| ---------------------------------- | ------------ | ------------- | ------------ |
| student558 | 1 | | |
| student559, student562, student563 | 2 | 3 | 1 |
| student560, student561 | 2 | 1 | |
| student564 | 1 | 2 | |
| student565 | 3 | | |
| student566 | 2 | | |

Return to the '''Assignments''' page after you set up the New test assignment, and retry the orange book icon. You should be indicated that the intelligent assignment has run successfully. You can check the teaming result under the '''Topics''' tab of the edit page. The above data generate the teaming result of the following.

[[File:E2012_5.png]]

= Future Improvements =

# Improving the RSpec test for assigning open topics for teams without topics. The new test only checks for 1 open assignment and 1 unassigned team. The method is capable of assigning multiple open topics for multiple unassigned teams, but testing for this would require more Factory objects for assignments and teams. While the new test provides code coverage for the entire method, the nested loops currently only run through a single time before completion, which is not always the case in a realistic setting.
# Improving the time-space complexity for the method that merges bids from teams that are merging together. The existing method requires iterating through each member's bidding information to create a hash map. Then there is a nest iteration for the included topics. Afterwards there is an additional iterations through to exclude topics without bids.

CSC/ECE 517 Spring 2020/E2012. refactor lottery controller.rb

2020-03-30T18:14:21Z

Thwinter: /* Test Plan */

= Introduction =

== Expertiza Background ==

Expertiza is an open-source project written using the Ruby on Rails framework. The Expertiza platform aims to improve student learning by using active and cooperative learning, allowing distance education students to participate in active learning, and by discouraging plagiarism through the use of multiple deadlines. Expertiza helps to improve teaching and resource utilization by having students contribute to the creation of course materials and through peer-graded assignments. Expertiza manages objects related to a course such as instructors, TAs, students, and other users. It also manages assignments, teams, signup topics, rubrics, peer grading, and peer evaluations.

== Lottery Controller Background ==

An assignment can have a list of signup topics. The teams associated with the assignment can bid on the signup topics to try and get assigned one they find more favorable. At a high level, the lottery controller assigns teams to topics based on the priorities the team gave to each signup topic during the bidding process.

In more detail, each student starts off on a team of size 1. They can bid on signup topics by arranging them in priority order. A team can invite other users to join their team. If student2 from Team B joins Team A which includes student1, student2 will lose their bids and take on the bids of TeamA and student1.
When the lottery controller is called to run its method run_intelligent_assignment, a web service takes the bidding data from each team and returns a new list of teams, each of which is closer to the maximum team size specified for the assignment. The web service combines teams together that have similar
bid data and then assigns those new teams to topics, giving each team their top bid on a topic that hasn't been assigned yet. Teams with larger team sizes and more bids are assigned their topics first.

== Problem Statement ==

Code Climate currently grades <code>lottery_controller.rb</code> at a C level, finding issues with the controller's long methods and complex statements. Most methods are around 40 lines and hard to decipher with a quick look. Also, the file currently has only 10% test coverage. Our goal is to resolve the Code Climate issues, make methods simpler to understand, and write new tests to improve test coverage.

== Tasks Identified ==

# Cut down on code in run_intelligent_assignment method by moving some code to helper methods
# Cut down on code in create_new_teams_for_bidding_response by moving some code to helper methods
# Cut down on code in match_new_teams_to_topics by moving some code to helper methods
# Reduce cognitive complexity of merge_bids_from_different_previous_teams method
# Add additional comments to clarify newly written methods
# Add additional RSpec tests to increase the coverage percentage

= Implementation =

We did the following refactoring to the <code>LotteryController</code> class:

# Added 6 helper methods (<code>construct_users_bidding_info</code>, <code>construct_teams_bidding_info</code>, <code>remove_user_from_previous_team</code>, <code>remove_empty_teams</code>, <code>assign_available_slots</code>) that each represent a specific and meaningful step
# Replaced large chunks of code with a small number of method calls
# Reduced the cognitive complexity by reducing the level of nested loops
# Optimized the performance by moving conditional checking and input filtering outside of loops, where appropriate
# Renamed some variable names to make their purpose clearer
# Refactored the parameter list to give each method the least amount of information needed to complete its task
# Added addition comments and fixed some typos

Here are some examples of the problems we identified and the solutions we found to these problems.

=== Problem 1: Long Methods ===

The code climate analysis tool identified two long methods exceeding 25 lines of code per method, <code>run_intelligent_assignment</code> and <code>match_new_teams_to_topics</code>. While some chunks of code are short and intuitive, others can be pulled out and put into helper methods with more meaningful method names.

====Solutions ====

=====run_intelligent_assignment=====

1. Put lines 19-29 into a separate method called <code>construct_users_bidding_info</code> that is responsible for generating user bidding information hash for students on a team who haven't signed up for a topic yet

[[Media:E2012_1.png| view code changes]]

2. Place the code closest to where it is needed. e.g. place line 32 (the <code>url</code> variable initialization) inside the <code>begin/rescue</code> block because it is considered an integral part of the REST API call.

[[Media:E2012_2.png| view code changes]]

===== match_new_teams_to_topics =====

1. Conclude lines 147-157 with a new method <code>construct_teams_bidding_info</code> that is responsible for generating team bidding information needed for the <code>assign_available_slots</code> method.

2. Conclude lines 161-169 with a new method <code>assign_available_slots</code> that uses the result returned from <code>construct_teams_bidding_info</code> to match unassigned teams to available topics.

These two steps greatly shorten the <code>match_new_teams_to_topics</code> method and make the workflow more intuitive to code maintainers.

[[Media:E2012_3.png| view code changes]]

=== Problem 2: Verbose Code ===

Verbosity contributes to more lines of codes and increases the cognitive complexity, which is the measure of how difficult a unit of code is to intuitively understand. Method extraction can't be applied to every long method as some methods only do one thing. For these methods, one can keep them simple with alternative language syntax and other ways of thinking of the behavior.

==== Solutions ====

===== Example 1 =====

# Before
bidding_matrix = {}
assignment.sign_up_topics.each_with_index do |topic, index|
bidding_matrix[topic.id] = [] unless bidding_matrix[topic.id]
bidding_matrix[topic.id] << bids[index]
end

The above code segment ensures that <code>bidding_matrix[topic.id]</code> returns an array object before pushing a new value to it. It is inelegant both in its readability and the time wasted to visit each <code>bidding_matrix[topic.id]</code> twice. The same behavior can be accomplished by constructing a Hash object that knows what default value to return when the key has not been seen before.

# After
bidding_matrix = Hash.new {|hash, key| hash[key] = [] }
sign_up_topics.each_with_index do |topic, index|
bidding_matrix[topic.id] << bids[index]
end

=====Example 2=====

# Before
teams.each do |team|
next if SignedUpTeam.where(team_id: team.id, is_waitlisted: 0).any?
... (rest of code omitted)
end

This block is extracted from the original implementation that, like many places in the <code>LotteryController</code>, uses <code>next</code> to skip any elements that don't fulfill the imposed condition. It's more reasonable to eliminate out unwanted elements outside of the loop using either <code>select</code> or <code>reject</code> call. Below is the modification made to this block.

# After
teams_not_signed_up = teams.reject {|team| SignedUpTeam.where(team_id: team.id, is_waitlisted: 0).any? }
teams_not_signed_up.each do |team|
... (rest of code omitted)
end

===== Example 3 =====

Same as example 1, the following block uses <code>next</code> to skip unwanted <code>team_user</code>s until the one that meets the condition, that is, who belongs to the specified assignment. This code does the filtering twice, which can be combined with one query.

# Before
TeamsUser.where(user_id: user_id).each do |team_user|
next unless team_user.team.parent_id == assignment.id
team_user.team_user_node.destroy rescue nil
team_user.destroy rescue nil
break
end

We first tried to use <code>find_by</code> with two parameters. <code>find_by</code> works by pulling column values from the model object. Since the <code>TeamsUser</code> model does not have a <code>team</code> attribute (it only stores <code>team_id</code>), this attempt is syntactically illegal.

team_user = TeamsUser.find_by(user_id: user_id, team.parent_id: assignment_id)
team_user.team_user_node.destroy rescue nil
team_user.destroy rescue nil

The way we refactored the code is to use the <code>find</code> method from the <code>Enumerable</code> mixin to get the first element for which the block is not false. In this way, the loop will no longer be necessary since the only thing it does is finding the first matching object.

# After
team_user = TeamsUser.where(user_id: user_id).find{|team_user| team_user.team.parent_id == assignment_id }
team_user.team_user_node.destroy rescue nil
team_user.destroy rescue nil

===== Example 4 =====

Besides pulling unwanted elements out before entering the loop, one can further reduce the verbosity of the code by replacing <code>next if</code> with <code>unless</code> and adding this checking step to the only statement that's inside the loop.

# Before
bidding_matrix.each do |topic_id, value|
next if value.inject(:+).zero?
bidding_matrix_summary << [value.count {|i| i != 0 }, value.inject(:+), topic_id]
end

# After
bidding_matrix.each do |topic_id, value|
bidding_matrix_summary << [value.count {|i| i != 0 }, value.inject(:+), topic_id] unless value.inject(:+).zero?
end

=== Problem 3: Ambiguous Identifiers ===

There were many puzzling variable names in the original <code>lottery_controller.rb</code> file. For example, copied from line 30, the slot with the label <code>users</code> accepts an input of <code>priority_info</code>.
bidding_data = {users: priority_info, max_team_size: assignment.max_team_size}

The <code>priority_info</code> is an array of hashes, each hash stores a pair of user id and bids that the user has made so far. This variable name is not descriptive enough to visualize its content. It is refactored into a new name, <code>users_bidding_info</code>, and the corresponding construction method is therefore named <code>construct_users_bidding_info</code>.

There is another array that has a very similar structure to the <code>users_bidding_info</code>, except that it holds bidding information for teams rather than users. To follow a good naming consistency, this array is granted a new name called <code>teams_bidding_info</code> (previously named <code>team_bids</code>) and the name of the corresponding construction method is changed to <code>construct_teams_bidding_info</code>.

'''Notes'''

We originally considered to change the label <code>ranks</code> to <code>bids</code> because the same identity has been presented in three different forms throughout the file: <code>rank</code>, <code>bid</code>, and <code>priority</code>.
|user| priority_info << {pid: user.id, ranks: bids}
However, such a change could have an impact on the later REST API call to the web service that uses students' bidding data to build teams automatically. We are uncertain about how the bidding data is actually used at the other end so it is not safe to apply such a change.

=== Problem 4: Multi-Purposed Methods ===

The <code>create_new_teams_for_bidding_response</code> method does more than 1 thing, both creating new teams and deleting residual teams that none of their team members remained in their teams. Since it is doing more beyond just what the name implies, it is reasonable to extract the removing part of the code to a new method and place the method call after the call to the <code>create_new_teams_for_bidding_response</code> method.

# Before
create_new_teams_for_bidding_response(teams, assignment, priority_info)
# ^^ <code>create_new_teams_for_bidding_response</code> is responsible for both creation and deletion of AssignmentTeam objects
match_new_teams_to_topics(assignment)

# After
create_new_teams_for_bidding_response(teams, assignment, users_bidding_info)
remove_empty_teams(assignment)
# ^^ <code>remove_empty_teams</code> becomes a separate method
match_new_teams_to_topics(assignment)

=== Problem 5: Unnecessary Looping ===

Many unnecessary looping can be avoided by moving the conditional check out. For example, consider this piece of code.

#Before
bids = []
topics.each do |topic|
bid_record = Bid.find_by(team_id: team.id, topic_id: topic.id)
bids << (bid_record.nil? ? 0 : bid_record.priority ||= 0)
end
team.users.each {|user| priority_info << {pid: user.id, ranks: bids} if bids.uniq != [0] }

The last line shows a looping on the team users that for each user in the team, it pushes its user id along with the generated bids to the <code>priority_info</code> array. If there are 3 users in a team, then the comparison <code>if bids.uniq != [0]</code> always gets executed 3 times while it should only be executed once because at this point the <code>bids</code> variable will not be changed anymore. The solution to this performance drawback would be to change the last line to the code below:

# After
team.users.each {|user| users_bidding_info << {pid: user.id, ranks: bids} } unless bids.uniq == [0]
# Note that priority_info has been renamed to users_bidding_info

Other fixes on the unnecessary looping are demonstrated in examples 2 and 3 of Problem 2. Instead of moving the checking condition out, they each move the filter out so the loop runs in a minimum number of times.

= Testing =

== Test Plan ==

The main objective of improving the current Lottery Controller testing is to increase the code coverage as much as possible. To achieve this object, tests will be written for each newly added method in the controller. The initial tests for these methods will assume expected normal conditions that the controller would face.

The next step to bolster the testing section is to make sure that the tests catch unexpected inputs, so additional tests to account for edge cases and invalid inputs will be implemented. These edge cases include an unintelligent assignment being passed to match_new_teams_to_topics, no teams being passed to the different methods that require teams, and no topics available when there are users still unassigned.

Finally, the last goal is to allow other users to be able to manually test the Lottery Controller themselves. This will be accomplished my implementing User Interface Testing. Users will be able to create their own testing scenarios by creating their testable objects.

Using these steps, the goal is to achieve total test coverage or as much coverage as possible and provide a testable framework for other users to test their own scenarios.

== RSpec Testing ==

=== Existing Tests ===
There were existing RSpec tests for the Lottery Controller, but the tests only provided about 10% coverage of the code in the controller. The test coverage was determined by the Coveralls gem. Most of the existing tests actually didn't even test the functionality of the lottery controller. The existing tests included the following:

# Testing an http get request to google.com and expecting a 200 response code with the content-type of application/json This was an attempt at testing the web service used by the lottery controller
# Checking if an assignment had a correctly set is_intelligent attribute Didn't call any methods of the lottery controller
# Creating new team for a bid
# Sorting unassigned teams Didn't call any methods of the lottery controller

=== After Refactoring ===

After refactoring the Lottery Controller, the long methods were broken down into smaller, more simpler methods. This required a new RSpec test for each of the new methods, but it allowed for increased code coverage. In order to effectively test the rest of the controller, testing objects that mimic the expected application objects were created. The testing objects were created from the factories.rb file using the factory_bot_rails gem and the double feature of the RSpec gem. The objects are also recreated before each test so that the tests are independent from one another to truly validate the different testing sections. The added tests were able to expand the code coverage from about 10% to 99%. The new tests that were added for each of the newly condensed methods include testing for the following:

# Constructing the hash table for the users' bidding information
# A more exhaustive running Intelligent Assignment method
# Generate the hash for a whole team's bidding information
# Match new teams to a topic based on their bids
# Remove a user from a team
# Destroying teams that have no users
# Merging the bids from teams that have merged together so that they have a weighted uniform bid
# Assigning open topics to teams that do not have a topic yet
# Matching teams to topics based on their weighted bids

=== Running the Tests ===
These tests can be executed by utilizing the RSpec command in the Command Prompt window by entering:

<code> user:expertiza $rspec spec/controllers/lottery_controller_spec.rb </code>

== UI Testing (For Reviewers) ==

By following the below steps, you can manually test our refactored <code>LotteryController</code> class.

'''Note:''' The previous implementation does the intelligent assignment on a team basis. That is, students who do not belong to any team will not get assigned to topics (see method <code>construct_users_bidding_info</code>). We intended not to disrupt this behavior since it's beyond the scope of this refactoring.

# Visit [xxx]. Enter the credentials: '''Username:''' instructor6 '''Password:''' password
# From the menubar, hover on the '''Manage''' tab, and select '''Assignments'''.
# Scroll down the page and find the assignment with the name '''New test assignment'''. The orange book icon on the right links to the <code>run_intelligent_assignment</code> method of the <code>LotteryController</code> class. [[File:E2012_4.png]] When you click on that, you should receive an error message claiming "500 Internal Server Error". This is because this assignment has not properly set up its participants. To get the intelligent assignment to run, you should click on the pencil icon, navigate to "Other stuff -> Add participant", and add the student by entering its user_id to the textbox next to the label "Enter a user login:".

'''Additional configuring:'''

# Under the '''General''' tab, mark the checkbox '''Has teams''' as true and set the maximum number of members per team to number that makes sense (e.g. our demonstration uses 3).
# You may also want to login to the account of each student you add (they all have "**password**" as their passwords), and help that student bid the topics just like how you did with our OSS project! The algorithm works more effectively when there are bids presented in the system. And by signing up topics, you indirectly create teams for them. We know this process can be very tedious, so thank you ahead for your patience!

Here are some data we used in our recording, you are free to come up with your own!

| User id | First Choice | Second Choice | Third Choice |
| ---------------------------------- | ------------ | ------------- | ------------ |
| student558 | 1 | | |
| student559, student562, student563 | 2 | 3 | 1 |
| student560, student561 | 2 | 1 | |
| student564 | 1 | 2 | |
| student565 | 3 | | |
| student566 | 2 | | |

Return to the '''Assignments''' page after you set up the New test assignment, and retry the orange book icon. You should be indicated that the intelligent assignment has run successfully. You can check the teaming result under the '''Topics''' tab of the edit page. The above data generate the teaming result of the following.

[[File:E2012_5.png]]

= Future Improvements =

# Improving the RSpec test for assigning open topics for teams without topics. The new test only checks for 1 open assignment and 1 unassigned team. The method is capable of assigning multiple open topics for multiple unassigned teams, but testing for this would require more Factory objects for assignments and teams. While the new test provides code coverage for the entire method, the nested loops currently only run through a single time before completion, which is not always the case in a realistic setting.
# Improving the time-space complexity for the method that merges bids from teams that are merging together. The existing method requires iterating through each member's bidding information to create a hash map. Then there is a nest iteration for the included topics. Afterwards there is an additional iterations through to exclude topics without bids.

CSC/ECE 517 Spring 2020/E2012. refactor lottery controller.rb

2020-03-30T18:09:48Z

Thwinter: /* Test Plan */

= Introduction =

== Expertiza Background ==

Expertiza is an open-source project written using the Ruby on Rails framework. The Expertiza platform aims to improve student learning by using active and cooperative learning, allowing distance education students to participate in active learning, and by discouraging plagiarism through the use of multiple deadlines. Expertiza helps to improve teaching and resource utilization by having students contribute to the creation of course materials and through peer-graded assignments. Expertiza manages objects related to a course such as instructors, TAs, students, and other users. It also manages assignments, teams, signup topics, rubrics, peer grading, and peer evaluations.

== Lottery Controller Background ==

An assignment can have a list of signup topics. The teams associated with the assignment can bid on the signup topics to try and get assigned one they find more favorable. At a high level, the lottery controller assigns teams to topics based on the priorities the team gave to each signup topic during the bidding process.

In more detail, each student starts off on a team of size 1. They can bid on signup topics by arranging them in priority order. A team can invite other users to join their team. If student2 from Team B joins Team A which includes student1, student2 will lose their bids and take on the bids of TeamA and student1.
When the lottery controller is called to run its method run_intelligent_assignment, a web service takes the bidding data from each team and returns a new list of teams, each of which is closer to the maximum team size specified for the assignment. The web service combines teams together that have similar
bid data and then assigns those new teams to topics, giving each team their top bid on a topic that hasn't been assigned yet. Teams with larger team sizes and more bids are assigned their topics first.

== Problem Statement ==

Code Climate currently grades <code>lottery_controller.rb</code> at a C level, finding issues with the controller's long methods and complex statements. Most methods are around 40 lines and hard to decipher with a quick look. Also, the file currently has only 10% test coverage. Our goal is to resolve the Code Climate issues, make methods simpler to understand, and write new tests to improve test coverage.

== Tasks Identified ==

# Cut down on code in run_intelligent_assignment method by moving some code to helper methods
# Cut down on code in create_new_teams_for_bidding_response by moving some code to helper methods
# Cut down on code in match_new_teams_to_topics by moving some code to helper methods
# Reduce cognitive complexity of merge_bids_from_different_previous_teams method
# Add additional comments to clarify newly written methods
# Add additional RSpec tests to increase the coverage percentage

= Implementation =

We did the following refactoring to the <code>LotteryController</code> class:

# Added 6 helper methods (<code>construct_users_bidding_info</code>, <code>construct_teams_bidding_info</code>, <code>remove_user_from_previous_team</code>, <code>remove_empty_teams</code>, <code>assign_available_slots</code>) that each represent a specific and meaningful step
# Replaced large chunks of code with a small number of method calls
# Reduced the cognitive complexity by reducing the level of nested loops
# Optimized the performance by moving conditional checking and input filtering outside of loops, where appropriate
# Renamed some variable names to make their purpose clearer
# Refactored the parameter list to give each method the least amount of information needed to complete its task
# Added addition comments and fixed some typos

Here are some examples of the problems we identified and the solutions we found to these problems.

=== Problem 1: Long Methods ===

The code climate analysis tool identified two long methods exceeding 25 lines of code per method, <code>run_intelligent_assignment</code> and <code>match_new_teams_to_topics</code>. While some chunks of code are short and intuitive, others can be pulled out and put into helper methods with more meaningful method names.

====Solutions ====

=====run_intelligent_assignment=====

1. Put lines 19-29 into a separate method called <code>construct_users_bidding_info</code> that is responsible for generating user bidding information hash for students on a team who haven't signed up for a topic yet

[[Media:E2012_1.png| view code changes]]

2. Place the code closest to where it is needed. e.g. place line 32 (the <code>url</code> variable initialization) inside the <code>begin/rescue</code> block because it is considered an integral part of the REST API call.

[[Media:E2012_2.png| view code changes]]

===== match_new_teams_to_topics =====

1. Conclude lines 147-157 with a new method <code>construct_teams_bidding_info</code> that is responsible for generating team bidding information needed for the <code>assign_available_slots</code> method.

2. Conclude lines 161-169 with a new method <code>assign_available_slots</code> that uses the result returned from <code>construct_teams_bidding_info</code> to match unassigned teams to available topics.

These two steps greatly shorten the <code>match_new_teams_to_topics</code> method and make the workflow more intuitive to code maintainers.

[[Media:E2012_3.png| view code changes]]

=== Problem 2: Verbose Code ===

Verbosity contributes to more lines of codes and increases the cognitive complexity, which is the measure of how difficult a unit of code is to intuitively understand. Method extraction can't be applied to every long method as some methods only do one thing. For these methods, one can keep them simple with alternative language syntax and other ways of thinking of the behavior.

==== Solutions ====

===== Example 1 =====

# Before
bidding_matrix = {}
assignment.sign_up_topics.each_with_index do |topic, index|
bidding_matrix[topic.id] = [] unless bidding_matrix[topic.id]
bidding_matrix[topic.id] << bids[index]
end

The above code segment ensures that <code>bidding_matrix[topic.id]</code> returns an array object before pushing a new value to it. It is inelegant both in its readability and the time wasted to visit each <code>bidding_matrix[topic.id]</code> twice. The same behavior can be accomplished by constructing a Hash object that knows what default value to return when the key has not been seen before.

# After
bidding_matrix = Hash.new {|hash, key| hash[key] = [] }
sign_up_topics.each_with_index do |topic, index|
bidding_matrix[topic.id] << bids[index]
end

=====Example 2=====

# Before
teams.each do |team|
next if SignedUpTeam.where(team_id: team.id, is_waitlisted: 0).any?
... (rest of code omitted)
end

This block is extracted from the original implementation that, like many places in the <code>LotteryController</code>, uses <code>next</code> to skip any elements that don't fulfill the imposed condition. It's more reasonable to eliminate out unwanted elements outside of the loop using either <code>select</code> or <code>reject</code> call. Below is the modification made to this block.

# After
teams_not_signed_up = teams.reject {|team| SignedUpTeam.where(team_id: team.id, is_waitlisted: 0).any? }
teams_not_signed_up.each do |team|
... (rest of code omitted)
end

===== Example 3 =====

Same as example 1, the following block uses <code>next</code> to skip unwanted <code>team_user</code>s until the one that meets the condition, that is, who belongs to the specified assignment. This code does the filtering twice, which can be combined with one query.

# Before
TeamsUser.where(user_id: user_id).each do |team_user|
next unless team_user.team.parent_id == assignment.id
team_user.team_user_node.destroy rescue nil
team_user.destroy rescue nil
break
end

We first tried to use <code>find_by</code> with two parameters. <code>find_by</code> works by pulling column values from the model object. Since the <code>TeamsUser</code> model does not have a <code>team</code> attribute (it only stores <code>team_id</code>), this attempt is syntactically illegal.

team_user = TeamsUser.find_by(user_id: user_id, team.parent_id: assignment_id)
team_user.team_user_node.destroy rescue nil
team_user.destroy rescue nil

The way we refactored the code is to use the <code>find</code> method from the <code>Enumerable</code> mixin to get the first element for which the block is not false. In this way, the loop will no longer be necessary since the only thing it does is finding the first matching object.

# After
team_user = TeamsUser.where(user_id: user_id).find{|team_user| team_user.team.parent_id == assignment_id }
team_user.team_user_node.destroy rescue nil
team_user.destroy rescue nil

===== Example 4 =====

Besides pulling unwanted elements out before entering the loop, one can further reduce the verbosity of the code by replacing <code>next if</code> with <code>unless</code> and adding this checking step to the only statement that's inside the loop.

# Before
bidding_matrix.each do |topic_id, value|
next if value.inject(:+).zero?
bidding_matrix_summary << [value.count {|i| i != 0 }, value.inject(:+), topic_id]
end

# After
bidding_matrix.each do |topic_id, value|
bidding_matrix_summary << [value.count {|i| i != 0 }, value.inject(:+), topic_id] unless value.inject(:+).zero?
end

=== Problem 3: Ambiguous Identifiers ===

There were many puzzling variable names in the original <code>lottery_controller.rb</code> file. For example, copied from line 30, the slot with the label <code>users</code> accepts an input of <code>priority_info</code>.
bidding_data = {users: priority_info, max_team_size: assignment.max_team_size}

The <code>priority_info</code> is an array of hashes, each hash stores a pair of user id and bids that the user has made so far. This variable name is not descriptive enough to visualize its content. It is refactored into a new name, <code>users_bidding_info</code>, and the corresponding construction method is therefore named <code>construct_users_bidding_info</code>.

There is another array that has a very similar structure to the <code>users_bidding_info</code>, except that it holds bidding information for teams rather than users. To follow a good naming consistency, this array is granted a new name called <code>teams_bidding_info</code> (previously named <code>team_bids</code>) and the name of the corresponding construction method is changed to <code>construct_teams_bidding_info</code>.

'''Notes'''

We originally considered to change the label <code>ranks</code> to <code>bids</code> because the same identity has been presented in three different forms throughout the file: <code>rank</code>, <code>bid</code>, and <code>priority</code>.
|user| priority_info << {pid: user.id, ranks: bids}
However, such a change could have an impact on the later REST API call to the web service that uses students' bidding data to build teams automatically. We are uncertain about how the bidding data is actually used at the other end so it is not safe to apply such a change.

=== Problem 4: Multi-Purposed Methods ===

The <code>create_new_teams_for_bidding_response</code> method does more than 1 thing, both creating new teams and deleting residual teams that none of their team members remained in their teams. Since it is doing more beyond just what the name implies, it is reasonable to extract the removing part of the code to a new method and place the method call after the call to the <code>create_new_teams_for_bidding_response</code> method.

# Before
create_new_teams_for_bidding_response(teams, assignment, priority_info)
# ^^ <code>create_new_teams_for_bidding_response</code> is responsible for both creation and deletion of AssignmentTeam objects
match_new_teams_to_topics(assignment)

# After
create_new_teams_for_bidding_response(teams, assignment, users_bidding_info)
remove_empty_teams(assignment)
# ^^ <code>remove_empty_teams</code> becomes a separate method
match_new_teams_to_topics(assignment)

=== Problem 5: Unnecessary Looping ===

Many unnecessary looping can be avoided by moving the conditional check out. For example, consider this piece of code.

#Before
bids = []
topics.each do |topic|
bid_record = Bid.find_by(team_id: team.id, topic_id: topic.id)
bids << (bid_record.nil? ? 0 : bid_record.priority ||= 0)
end
team.users.each {|user| priority_info << {pid: user.id, ranks: bids} if bids.uniq != [0] }

The last line shows a looping on the team users that for each user in the team, it pushes its user id along with the generated bids to the <code>priority_info</code> array. If there are 3 users in a team, then the comparison <code>if bids.uniq != [0]</code> always gets executed 3 times while it should only be executed once because at this point the <code>bids</code> variable will not be changed anymore. The solution to this performance drawback would be to change the last line to the code below:

# After
team.users.each {|user| users_bidding_info << {pid: user.id, ranks: bids} } unless bids.uniq == [0]
# Note that priority_info has been renamed to users_bidding_info

Other fixes on the unnecessary looping are demonstrated in examples 2 and 3 of Problem 2. Instead of moving the checking condition out, they each move the filter out so the loop runs in a minimum number of times.

= Testing =

== Test Plan ==

The main objective of improving the current Lottery Controller testing is to increase the code coverage as much as possible. To achieve this object, tests will be written for each newly added method in the controller. The initial tests for these methods will assume expected normal conditions that the controller would face.

The next step to bolster the testing section is to make sure that the tests catch unexpected inputs, so additional tests to account for edge cases and invalid inputs will be implemented. These edge cases include an unintelligent assignment being passed to match_new_teams_to_topics, no teams being passed to the different methods that require teams, and no topics available when there are users still unassigned.

Using these steps, the goal is to achieve total test coverage or as much coverage as possible.

== RSpec Testing ==

=== Existing Tests ===
There were existing RSpec tests for the Lottery Controller, but the tests only provided about 10% coverage of the code in the controller. The test coverage was determined by the Coveralls gem. Most of the existing tests actually didn't even test the functionality of the lottery controller. The existing tests included the following:

# Testing an http get request to google.com and expecting a 200 response code with the content-type of application/json This was an attempt at testing the web service used by the lottery controller
# Checking if an assignment had a correctly set is_intelligent attribute Didn't call any methods of the lottery controller
# Creating new team for a bid
# Sorting unassigned teams Didn't call any methods of the lottery controller

=== After Refactoring ===

After refactoring the Lottery Controller, the long methods were broken down into smaller, more simpler methods. This required a new RSpec test for each of the new methods, but it allowed for increased code coverage. In order to effectively test the rest of the controller, testing objects that mimic the expected application objects were created. The testing objects were created from the factories.rb file using the factory_bot_rails gem and the double feature of the RSpec gem. The objects are also recreated before each test so that the tests are independent from one another to truly validate the different testing sections. The added tests were able to expand the code coverage from about 10% to 99%. The new tests that were added for each of the newly condensed methods include testing for the following:

# Constructing the hash table for the users' bidding information
# A more exhaustive running Intelligent Assignment method
# Generate the hash for a whole team's bidding information
# Match new teams to a topic based on their bids
# Remove a user from a team
# Destroying teams that have no users
# Merging the bids from teams that have merged together so that they have a weighted uniform bid
# Assigning open topics to teams that do not have a topic yet
# Matching teams to topics based on their weighted bids

=== Running the Tests ===
These tests can be executed by utilizing the RSpec command in the Command Prompt window by entering:

<code> user:expertiza $rspec spec/controllers/lottery_controller_spec.rb </code>

== UI Testing (For Reviewers) ==

By following the below steps, you can manually test our refactored <code>LotteryController</code> class.

'''Note:''' The previous implementation does the intelligent assignment on a team basis. That is, students who do not belong to any team will not get assigned to topics (see method <code>construct_users_bidding_info</code>). We intended not to disrupt this behavior since it's beyond the scope of this refactoring.

# Visit [xxx]. Enter the credentials: '''Username:''' instructor6 '''Password:''' password
# From the menubar, hover on the '''Manage''' tab, and select '''Assignments'''.
# Scroll down the page and find the assignment with the name '''New test assignment'''. The orange book icon on the right links to the <code>run_intelligent_assignment</code> method of the <code>LotteryController</code> class. [[File:E2012_4.png]] When you click on that, you should receive an error message claiming "500 Internal Server Error". This is because this assignment has not properly set up its participants. To get the intelligent assignment to run, you should click on the pencil icon, navigate to "Other stuff -> Add participant", and add the student by entering its user_id to the textbox next to the label "Enter a user login:".

'''Additional configuring:'''

# Under the '''General''' tab, mark the checkbox '''Has teams''' as true and set the maximum number of members per team to number that makes sense (e.g. our demonstration uses 3).
# You may also want to login to the account of each student you add (they all have "**password**" as their passwords), and help that student bid the topics just like how you did with our OSS project! The algorithm works more effectively when there are bids presented in the system. And by signing up topics, you indirectly create teams for them. We know this process can be very tedious, so thank you ahead for your patience!

Here are some data we used in our recording, you are free to come up with your own!

| User id | First Choice | Second Choice | Third Choice |
| ---------------------------------- | ------------ | ------------- | ------------ |
| student558 | 1 | | |
| student559, student562, student563 | 2 | 3 | 1 |
| student560, student561 | 2 | 1 | |
| student564 | 1 | 2 | |
| student565 | 3 | | |
| student566 | 2 | | |

Return to the '''Assignments''' page after you set up the New test assignment, and retry the orange book icon. You should be indicated that the intelligent assignment has run successfully. You can check the teaming result under the '''Topics''' tab of the edit page. The above data generate the teaming result of the following.

[[File:E2012_5.png]]

= Future Improvements =

# Improving the RSpec test for assigning open topics for teams without topics. The new test only checks for 1 open assignment and 1 unassigned team. The method is capable of assigning multiple open topics for multiple unassigned teams, but testing for this would require more Factory objects for assignments and teams. While the new test provides code coverage for the entire method, the nested loops currently only run through a single time before completion, which is not always the case in a realistic setting.
# Improving the time-space complexity for the method that merges bids from teams that are merging together. The existing method requires iterating through each member's bidding information to create a hash map. Then there is a nest iteration for the included topics. Afterwards there is an additional iterations through to exclude topics without bids.

CSC/ECE 517 Spring 2020/E2012. refactor lottery controller.rb

2020-03-30T16:15:32Z

Thwinter: /* Test Plan */

= Introduction =

== Expertiza Background ==

Expertiza is an open-source project written using the Ruby on Rails framework. The Expertiza platform aims to improve student learning by using active and cooperative learning, allowing distance education students to participate in active learning, and by discouraging plagiarism through the use of multiple deadlines. Expertiza helps to improve teaching and resource utilization by having students contribute to the creation of course materials and through peer-graded assignments. Expertiza manages objects related to a course such as instructors, TAs, students, and other users. It also manages assignments, teams, signup topics, rubrics, peer grading, and peer evaluations.

== Lottery Controller Background ==

An assignment can have a list of signup topics. The teams associated with the assignment can bid on the signup topics to try and get assigned one they find more favorable. At a high level, the lottery controller assigns teams to topics based on the priorities the team gave to each signup topic during the bidding process.

In more detail, each student starts off on a team of size 1. They can bid on signup topics by arranging them in priority order. A team can invite other users to join their team. If student2 from Team B joins Team A which includes student1, student2 will lose their bids and take on the bids of TeamA and student1.
When the lottery controller is called to run its method run_intelligent_assignment, a web service takes the bidding data from each team and returns a new list of teams, each of which is closer to the maximum team size specified for the assignment. The web service combines teams together that have similar
bid data and then assigns those new teams to topics, giving each team their top bid on a topic that hasn't been assigned yet. Teams with larger team sizes and more bids are assigned their topics first.

== Problem Statement ==

Code Climate currently grades <code>lottery_controller.rb</code> at a C level, finding issues with the controller's long methods and complex statements. Most methods are around 40 lines and hard to decipher with a quick look. Also, the file currently has only 10% test coverage. Our goal is to resolve the Code Climate issues, make methods simpler to understand, and write new tests to improve test coverage.

== Tasks Identified ==

# Cut down on code in run_intelligent_assignment method by moving some code to helper methods
# Cut down on code in create_new_teams_for_bidding_response by moving some code to helper methods
# Cut down on code in match_new_teams_to_topics by moving some code to helper methods
# Reduce cognitive complexity of merge_bids_from_different_previous_teams method
# Add additional comments to clarify newly written methods
# Add additional RSpec tests to increase the coverage percentage

= Implementation =

We did the following refactoring to the <code>LotteryController</code> class:

# Added 6 helper methods (<code>construct_users_bidding_info</code>, <code>construct_teams_bidding_info</code>, <code>remove_user_from_previous_team</code>, <code>remove_empty_teams</code>, <code>assign_available_slots</code>) that each represent a specific and meaningful step
# Replaced large chunks of code with a small number of method calls
# Reduced the cognitive complexity by reducing the level of nested loops
# Optimized the performance by moving conditional checking and input filtering outside of loops, where appropriate
# Renamed some variable names to make their purpose clearer
# Refactored the parameter list to give each method the least amount of information needed to complete its task
# Added addition comments and fixed some typos

Here are some examples of the problems we identified and the solutions we found to these problems.

=== Problem 1: Long Methods ===

The code climate analysis tool identified two long methods exceeding 25 lines of code per method, <code>run_intelligent_assignment</code> and <code>match_new_teams_to_topics</code>. While some chunks of code are short and intuitive, others can be pulled out and put into helper methods with more meaningful method names.

====Solutions ====

=====run_intelligent_assignment=====

1. Put lines 19-29 into a separate method called <code>construct_users_bidding_info</code> that is responsible for generating user bidding information hash for students on a team who haven't signed up for a topic yet

[[Media:E2012_1.png| view code changes]]

2. Place the code closest to where it is needed. e.g. place line 32 (the <code>url</code> variable initialization) inside the <code>begin/rescue</code> block because it is considered an integral part of the REST API call.

[[Media:E2012_2.png| view code changes]]

===== match_new_teams_to_topics =====

1. Conclude lines 147-157 with a new method <code>construct_teams_bidding_info</code> that is responsible for generating team bidding information needed for the <code>assign_available_slots</code> method.

2. Conclude lines 161-169 with a new method <code>assign_available_slots</code> that uses the result returned from <code>construct_teams_bidding_info</code> to match unassigned teams to available topics.

These two steps greatly shorten the <code>match_new_teams_to_topics</code> method and make the workflow more intuitive to code maintainers.

[[Media:E2012_3.png| view code changes]]

=== Problem 2: Verbose Code ===

Verbosity contributes to more lines of codes and increases the cognitive complexity, which is the measure of how difficult a unit of code is to intuitively understand. Method extraction can't be applied to every long method as some methods only do one thing. For these methods, one can keep them simple with alternative language syntax and other ways of thinking of the behavior.

==== Solutions ====

===== Example 1 =====

# Before
bidding_matrix = {}
assignment.sign_up_topics.each_with_index do |topic, index|
bidding_matrix[topic.id] = [] unless bidding_matrix[topic.id]
bidding_matrix[topic.id] << bids[index]
end

The above code segment ensures that <code>bidding_matrix[topic.id]</code> returns an array object before pushing a new value to it. It is inelegant both in its readability and the time wasted to visit each <code>bidding_matrix[topic.id]</code> twice. The same behavior can be accomplished by constructing a Hash object that knows what default value to return when the key has not been seen before.

# After
bidding_matrix = Hash.new {|hash, key| hash[key] = [] }
sign_up_topics.each_with_index do |topic, index|
bidding_matrix[topic.id] << bids[index]
end

=====Example 2=====

# Before
teams.each do |team|
next if SignedUpTeam.where(team_id: team.id, is_waitlisted: 0).any?
... (rest of code omitted)
end

This block is extracted from the original implementation that, like many places in the <code>LotteryController</code>, uses <code>next</code> to skip any elements that don't fulfill the imposed condition. It's more reasonable to eliminate out unwanted elements outside of the loop using either <code>select</code> or <code>reject</code> call. Below is the modification made to this block.

# After
teams_not_signed_up = teams.reject {|team| SignedUpTeam.where(team_id: team.id, is_waitlisted: 0).any? }
teams_not_signed_up.each do |team|
... (rest of code omitted)
end

===== Example 3 =====

Same as example 1, the following block uses <code>next</code> to skip unwanted <code>team_user</code>s until the one that meets the condition, that is, who belongs to the specified assignment. This code does the filtering twice, which can be combined with one query.

# Before
TeamsUser.where(user_id: user_id).each do |team_user|
next unless team_user.team.parent_id == assignment.id
team_user.team_user_node.destroy rescue nil
team_user.destroy rescue nil
break
end

We first tried to use <code>find_by</code> with two parameters. <code>find_by</code> works by pulling column values from the model object. Since the <code>TeamsUser</code> model does not have a <code>team</code> attribute (it only stores <code>team_id</code>), this attempt is syntactically illegal.

team_user = TeamsUser.find_by(user_id: user_id, team.parent_id: assignment_id)
team_user.team_user_node.destroy rescue nil
team_user.destroy rescue nil

The way we refactored the code is to use the <code>find</code> method from the <code>Enumerable</code> mixin to get the first element for which the block is not false. In this way, the loop will no longer be necessary since the only thing it does is finding the first matching object.

# After
team_user = TeamsUser.where(user_id: user_id).find{|team_user| team_user.team.parent_id == assignment_id }
team_user.team_user_node.destroy rescue nil
team_user.destroy rescue nil

===== Example 4 =====

Besides pulling unwanted elements out before entering the loop, one can further reduce the verbosity of the code by replacing <code>next if</code> with <code>unless</code> and adding this checking step to the only statement that's inside the loop.

# Before
bidding_matrix.each do |topic_id, value|
next if value.inject(:+).zero?
bidding_matrix_summary << [value.count {|i| i != 0 }, value.inject(:+), topic_id]
end

# After
bidding_matrix.each do |topic_id, value|
bidding_matrix_summary << [value.count {|i| i != 0 }, value.inject(:+), topic_id] unless value.inject(:+).zero?
end

=== Problem 3: Ambiguous Identifiers ===

There were many puzzling variable names in the original <code>lottery_controller.rb</code> file. For example, copied from line 30, the slot with the label <code>users</code> accepts an input of <code>priority_info</code>.
bidding_data = {users: priority_info, max_team_size: assignment.max_team_size}

The <code>priority_info</code> is an array of hashes, each hash stores a pair of user id and bids that the user has made so far. This variable name is not descriptive enough to visualize its content. It is refactored into a new name, <code>users_bidding_info</code>, and the corresponding construction method is therefore named <code>construct_users_bidding_info</code>.

There is another array that has a very similar structure to the <code>users_bidding_info</code>, except that it holds bidding information for teams rather than users. To follow a good naming consistency, this array is granted a new name called <code>teams_bidding_info</code> (previously named <code>team_bids</code>) and the name of the corresponding construction method is changed to <code>construct_teams_bidding_info</code>.

'''Notes'''

We originally considered to change the label <code>ranks</code> to <code>bids</code> because the same identity has been presented in three different forms throughout the file: <code>rank</code>, <code>bid</code>, and <code>priority</code>.
|user| priority_info << {pid: user.id, ranks: bids}
However, such a change could have an impact on the later REST API call to the web service that uses students' bidding data to build teams automatically. We are uncertain about how the bidding data is actually used at the other end so it is not safe to apply such a change.

=== Problem 4: Multi-Purposed Methods ===

The <code>create_new_teams_for_bidding_response</code> method does more than 1 thing, both creating new teams and deleting residual teams that none of their team members remained in their teams. Since it is doing more beyond just what the name implies, it is reasonable to extract the removing part of the code to a new method and place the method call after the call to the <code>create_new_teams_for_bidding_response</code> method.

# Before
create_new_teams_for_bidding_response(teams, assignment, priority_info)
# ^^ <code>create_new_teams_for_bidding_response</code> is responsible for both creation and deletion of AssignmentTeam objects
match_new_teams_to_topics(assignment)

# After
create_new_teams_for_bidding_response(teams, assignment, users_bidding_info)
remove_empty_teams(assignment)
# ^^ <code>remove_empty_teams</code> becomes a separate method
match_new_teams_to_topics(assignment)

=== Problem 5: Unnecessary Looping ===

Many unnecessary looping can be avoided by moving the conditional check out. For example, consider this piece of code.

#Before
bids = []
topics.each do |topic|
bid_record = Bid.find_by(team_id: team.id, topic_id: topic.id)
bids << (bid_record.nil? ? 0 : bid_record.priority ||= 0)
end
team.users.each {|user| priority_info << {pid: user.id, ranks: bids} if bids.uniq != [0] }

The last line shows a looping on the team users that for each user in the team, it pushes its user id along with the generated bids to the <code>priority_info</code> array. If there are 3 users in a team, then the comparison <code>if bids.uniq != [0]</code> always gets executed 3 times while it should only be executed once because at this point the <code>bids</code> variable will not be changed anymore. The solution to this performance drawback would be to change the last line to the code below:

# After
team.users.each {|user| users_bidding_info << {pid: user.id, ranks: bids} } unless bids.uniq == [0]
# Note that priority_info has been renamed to users_bidding_info

Other fixes on the unnecessary looping are demonstrated in examples 2 and 3 of Problem 2. Instead of moving the checking condition out, they each move the filter out so the loop runs in a minimum number of times.

= Testing =

== Test Plan ==

The main objective of improving the current Lottery Controller testing is to increase the code coverage as much as possible. To achieve this object, tests will be written for each newly added method in the controller. The initial tests for these methods will assume expected normal conditions that the controller would face.

The next step to bolster the testing section is to make sure that the tests catch unexpected inputs, so additional tests to account for edge cases and invalid inputs will be implemented. These invalid inputs include an unintelligent assignment being passed to match_new_teams_to_topics,

== RSpec Testing ==

=== Existing Tests ===
There were existing RSpec tests for the Lottery Controller, but the tests only provided about 10% coverage of the code in the controller. The test coverage was determined by the Coveralls gem. Most of the existing tests actually didn't even test the functionality of the lottery controller. The existing tests included the following:

# Testing an http get request to google.com and expecting a 200 response code with the content-type of application/json This was an attempt at testing the web service used by the lottery controller
# Checking if an assignment had a correctly set is_intelligent attribute Didn't call any methods of the lottery controller
# Creating new team for a bid
# Sorting unassigned teams Didn't call any methods of the lottery controller

=== After Refactoring ===

After refactoring the Lottery Controller, the long methods were broken down into smaller, more simpler methods. This required a new RSpec test for each of the new methods, but it allowed for increased code coverage. In order to effectively test the rest of the controller, testing objects that mimic the expected application objects were created. The testing objects were created from the factories.rb file using the factory_bot_rails gem and the double feature of the RSpec gem. The objects are also recreated before each test so that the tests are independent from one another to truly validate the different testing sections. The added tests were able to expand the code coverage from about 10% to 99%. The new tests that were added for each of the newly condensed methods include testing for the following:

# Constructing the hash table for the users' bidding information
# A more exhaustive running Intelligent Assignment method
# Generate the hash for a whole team's bidding information
# Match new teams to a topic based on their bids
# Remove a user from a team
# Destroying teams that have no users
# Merging the bids from teams that have merged together so that they have a weighted uniform bid
# Assigning open topics to teams that do not have a topic yet
# Matching teams to topics based on their weighted bids

=== Running the Tests ===
These tests can be executed by utilizing the RSpec command in the Command Prompt window by entering:

<code> user:expertiza $rspec spec/controllers/lottery_controller_spec.rb </code>

== UI Testing (For Reviewers) ==

By following the below steps, you can manually test our refactored <code>LotteryController</code> class.

'''Note:''' The previous implementation does the intelligent assignment on a team basis. That is, students who do not belong to any team will not get assigned to topics (see method <code>construct_users_bidding_info</code>). We intended not to disrupt this behavior since it's beyond the scope of this refactoring.

# Visit [xxx]. Enter the credentials: '''Username:''' instructor6 '''Password:''' password
# From the menubar, hover on the '''Manage''' tab, and select '''Assignments'''.
# Scroll down the page and find the assignment with the name '''New test assignment'''. The orange book icon on the right links to the <code>run_intelligent_assignment</code> method of the <code>LotteryController</code> class. When you click on that, you should receive an error message claiming "500 Internal Server Error". This is because this assignment has not properly set up its participants. To get the intelligent assignment to run, you should click on the pencil icon, navigate to "Other stuff -> Add participant", and add the student by entering its user_id to the textbox next to the label "Enter a user login:".

'''Additional configuring:'''

# Under the '''General''' tab, mark the checkbox '''Has teams''' as true and set the maximum number of members per team to number that makes sense (e.g. our demonstration uses 3).
# You may also want to login to the account of each student you add (they all have "**password**" as their passwords), and help that student bid the topics just like how you did with our OSS project! The algorithm works more effectively when there are bids presented in the system. And by signing up topics, you indirectly create teams for them. We know this process can be very tedious, so thank you ahead for your patience!

Here are some data we used in our recording, you are free to come up with your own!

| User id | First Choice | Second Choice | Third Choice |
| ---------------------------------- | ------------ | ------------- | ------------ |
| student558 | 1 | | |
| student559, student562, student563 | 2 | 3 | 1 |
| student560, student561 | 2 | 1 | |
| student564 | 1 | 2 | |
| student565 | 3 | | |
| student566 | 2 | | |

Return to the '''Assignments''' page after you set up the New test assignment, and retry the orange book icon. You should be indicated that the intelligent assignment has run successfully. You can check the teaming result under the '''Topics''' tab of the edit page. The above data generate the teaming result of the following.

= Future Improvements =

# Improving the RSpec test for assigning open topics for teams without topics. The new test only checks for 1 open assignment and 1 unassigned team. The method is capable of assigning multiple open topics for multiple unassigned teams, but testing for this would require more Factory objects for assignments and teams. While the new test provides code coverage for the entire method, the nested loops currently only run through a single time before completion, which is not always the case in a realistic setting.
# Improving the time-space complexity for the method that merges bids from teams that are merging together. The existing method requires iterating through each member's bidding information to create a hash map. Then there is a nest iteration for the included topics. Afterwards there is an additional iterations through to exclude topics without bids.

CSC/ECE 517 Spring 2020/E2012. refactor lottery controller.rb

2020-03-30T15:44:19Z

Thwinter: /* Testing */

= Introduction =

== Expertiza Background ==

Expertiza is an open-source project written using the Ruby on Rails framework. The Expertiza platform aims to improve student learning by using active and cooperative learning, allowing distance education students to participate in active learning, and by discouraging plagiarism through the use of multiple deadlines. Expertiza helps to improve teaching and resource utilization by having students contribute to the creation of course materials and through peer-graded assignments. Expertiza manages objects related to a course such as instructors, TAs, students, and other users. It also manages assignments, teams, signup topics, rubrics, peer grading, and peer evaluations.

== Lottery Controller Background ==

An assignment can have a list of signup topics. The teams associated with the assignment can bid on the signup topics to try and get assigned one they find more favorable. At a high level, the lottery controller assigns teams to topics based on the priorities the team gave to each signup topic during the bidding process.

In more detail, each student starts off on a team of size 1. They can bid on signup topics by arranging them in priority order. A team can invite other users to join their team. If student2 from Team B joins Team A which includes student1, student2 will lose their bids and take on the bids of TeamA and student1.
When the lottery controller is called to run its method run_intelligent_assignment, a web service takes the bidding data from each team and returns a new list of teams, each of which is closer to the maximum team size specified for the assignment. The web service combines teams together that have similar
bid data and then assigns those new teams to topics, giving each team their top bid on a topic that hasn't been assigned yet. Teams with larger team sizes and more bids are assigned their topics first.

== Problem Statement ==

Code Climate currently grades <code>lottery_controller.rb</code> at a C level, finding issues with the controller's long methods and complex statements. Most methods are around 40 lines and hard to decipher with a quick look. Also, the file currently has only 10% test coverage. Our goal is to resolve the Code Climate issues, make methods simpler to understand, and write new tests to improve test coverage.

== Tasks Identified ==

# Cut down on code in run_intelligent_assignment method by moving some code to helper methods
# Cut down on code in create_new_teams_for_bidding_response by moving some code to helper methods
# Cut down on code in match_new_teams_to_topics by moving some code to helper methods
# Reduce cognitive complexity of merge_bids_from_different_previous_teams method
# Add additional comments to clarify newly written methods
# Add additional RSpec tests to increase the coverage percentage

= Implementation =

We did the following refactoring to the <code>LotteryController</code> class:

# Added 6 helper methods (<code>construct_users_bidding_info</code>, <code>construct_teams_bidding_info</code>, <code>remove_user_from_previous_team</code>, <code>remove_empty_teams</code>, <code>assign_available_slots</code>) that each represent a specific and meaningful step
# Replaced large chunks of code with a small number of method calls
# Reduced the cognitive complexity by reducing the level of nested loops
# Optimized the performance by moving conditional checking and input filtering outside of loops, where appropriate
# Renamed some variable names to make their purpose clearer
# Refactored the parameter list to give each method the least amount of information needed to complete its task
# Added addition comments and fixed some typos

Here are some examples of the problems we identified and the solutions we found to these problems.

=== Problem 1: Long Methods ===

The code climate analysis tool identified two long methods exceeding 25 lines of code per method, <code>run_intelligent_assignment</code> and <code>match_new_teams_to_topics</code>. While some chunks of code are short and intuitive, others can be pulled out and put into helper methods with more meaningful method names.

====Solutions ====

=====run_intelligent_assignment=====

1. Put lines 19-29 into a separate method called <code>construct_users_bidding_info</code> that is responsible for generating user bidding information hash for students on a team who haven't signed up for a topic yet

[[Media:E2012_1.png| view code changes]]

2. Place the code closest to where it is needed. e.g. place line 32 (the <code>url</code> variable initialization) inside the <code>begin/rescue</code> block because it is considered an integral part of the REST API call.

[[Media:E2012_2.png| view code changes]]

===== match_new_teams_to_topics =====

1. Conclude lines 147-157 with a new method <code>construct_teams_bidding_info</code> that is responsible for generating team bidding information needed for the <code>assign_available_slots</code> method.

2. Conclude lines 161-169 with a new method <code>assign_available_slots</code> that uses the result returned from <code>construct_teams_bidding_info</code> to match unassigned teams to available topics.

These two steps greatly shorten the <code>match_new_teams_to_topics</code> method and make the workflow more intuitive to code maintainers.

[[Media:E2012_3.png| view code changes]]

=== Problem 2: Verbose Code ===

Verbosity contributes to more lines of codes and increases the cognitive complexity, which is the measure of how difficult a unit of code is to intuitively understand. Method extraction can't be applied to every long method as some methods only do one thing. For these methods, one can keep them simple with alternative language syntax and other ways of thinking of the behavior.

==== Solutions ====

===== Example 1 =====

# Before
bidding_matrix = {}
assignment.sign_up_topics.each_with_index do |topic, index|
bidding_matrix[topic.id] = [] unless bidding_matrix[topic.id]
bidding_matrix[topic.id] << bids[index]
end

The above code segment ensures that <code>bidding_matrix[topic.id]</code> returns an array object before pushing a new value to it. It is inelegant both in its readability and the time wasted to visit each <code>bidding_matrix[topic.id]</code> twice. The same behavior can be accomplished by constructing a Hash object that knows what default value to return when the key has not been seen before.

# After
bidding_matrix = Hash.new {|hash, key| hash[key] = [] }
sign_up_topics.each_with_index do |topic, index|
bidding_matrix[topic.id] << bids[index]
end

=====Example 2=====

# Before
teams.each do |team|
next if SignedUpTeam.where(team_id: team.id, is_waitlisted: 0).any?
... (rest of code omitted)
end

This block is extracted from the original implementation that, like many places in the <code>LotteryController</code>, uses <code>next</code> to skip any elements that don't fulfill the imposed condition. It's more reasonable to eliminate out unwanted elements outside of the loop using either <code>select</code> or <code>reject</code> call. Below is the modification made to this block.

# After
teams_not_signed_up = teams.reject {|team| SignedUpTeam.where(team_id: team.id, is_waitlisted: 0).any? }
teams_not_signed_up.each do |team|
... (rest of code omitted)
end

===== Example 3 =====

Same as example 1, the following block uses <code>next</code> to skip unwanted <code>team_user</code>s until the one that meets the condition, that is, who belongs to the specified assignment. This code does the filtering twice, which can be combined with one query.

# Before
TeamsUser.where(user_id: user_id).each do |team_user|
next unless team_user.team.parent_id == assignment.id
team_user.team_user_node.destroy rescue nil
team_user.destroy rescue nil
break
end

We first tried to use <code>find_by</code> with two parameters. <code>find_by</code> works by pulling column values from the model object. Since the <code>TeamsUser</code> model does not have a <code>team</code> attribute (it only stores <code>team_id</code>), this attempt is syntactically illegal.

team_user = TeamsUser.find_by(user_id: user_id, team.parent_id: assignment_id)
team_user.team_user_node.destroy rescue nil
team_user.destroy rescue nil

The way we refactored the code is to use the <code>find</code> method from the <code>Enumerable</code> mixin to get the first element for which the block is not false. In this way, the loop will no longer be necessary since the only thing it does is finding the first matching object.

# After
team_user = TeamsUser.where(user_id: user_id).find{|team_user| team_user.team.parent_id == assignment_id }
team_user.team_user_node.destroy rescue nil
team_user.destroy rescue nil

===== Example 4 =====

Besides pulling unwanted elements out before entering the loop, one can further reduce the verbosity of the code by replacing <code>next if</code> with <code>unless</code> and adding this checking step to the only statement that's inside the loop.

# Before
bidding_matrix.each do |topic_id, value|
next if value.inject(:+).zero?
bidding_matrix_summary << [value.count {|i| i != 0 }, value.inject(:+), topic_id]
end

# After
bidding_matrix.each do |topic_id, value|
bidding_matrix_summary << [value.count {|i| i != 0 }, value.inject(:+), topic_id] unless value.inject(:+).zero?
end

=== Problem 3: Ambiguous Identifiers ===

There were many puzzling variable names in the original <code>lottery_controller.rb</code> file. For example, copied from line 30, the slot with the label <code>users</code> accepts an input of <code>priority_info</code>.
bidding_data = {users: priority_info, max_team_size: assignment.max_team_size}

The <code>priority_info</code> is an array of hashes, each hash stores a pair of user id and bids that the user has made so far. This variable name is not descriptive enough to visualize its content. It is refactored into a new name, <code>users_bidding_info</code>, and the corresponding construction method is therefore named <code>construct_users_bidding_info</code>.

There is another array that has a very similar structure to the <code>users_bidding_info</code>, except that it holds bidding information for teams rather than users. To follow a good naming consistency, this array is granted a new name called <code>teams_bidding_info</code> (previously named <code>team_bids</code>) and the name of the corresponding construction method is changed to <code>construct_teams_bidding_info</code>.

'''Notes'''

We originally considered to change the label <code>ranks</code> to <code>bids</code> because the same identity has been presented in three different forms throughout the file: <code>rank</code>, <code>bid</code>, and <code>priority</code>.
|user| priority_info << {pid: user.id, ranks: bids}
However, such a change could have an impact on the later REST API call to the web service that uses students' bidding data to build teams automatically. We are uncertain about how the bidding data is actually used at the other end so it is not safe to apply such a change.

=== Problem 4: Multi-Purposed Methods ===

The <code>create_new_teams_for_bidding_response</code> method does more than 1 thing, both creating new teams and deleting residual teams that none of their team members remained in their teams. Since it is doing more beyond just what the name implies, it is reasonable to extract the removing part of the code to a new method and place the method call after the call to the <code>create_new_teams_for_bidding_response</code> method.

# Before
create_new_teams_for_bidding_response(teams, assignment, priority_info)
# ^^ <code>create_new_teams_for_bidding_response</code> is responsible for both creation and deletion of AssignmentTeam objects
match_new_teams_to_topics(assignment)

# After
create_new_teams_for_bidding_response(teams, assignment, users_bidding_info)
remove_empty_teams(assignment)
# ^^ <code>remove_empty_teams</code> becomes a separate method
match_new_teams_to_topics(assignment)

=== Problem 5: Unnecessary Looping ===

Many unnecessary looping can be avoided by moving the conditional check out. For example, consider this piece of code.

#Before
bids = []
topics.each do |topic|
bid_record = Bid.find_by(team_id: team.id, topic_id: topic.id)
bids << (bid_record.nil? ? 0 : bid_record.priority ||= 0)
end
team.users.each {|user| priority_info << {pid: user.id, ranks: bids} if bids.uniq != [0] }

The last line shows a looping on the team users that for each user in the team, it pushes its user id along with the generated bids to the <code>priority_info</code> array. If there are 3 users in a team, then the comparison <code>if bids.uniq != [0]</code> always gets executed 3 times while it should only be executed once because at this point the <code>bids</code> variable will not be changed anymore. The solution to this performance drawback would be to change the last line to the code below:

# After
team.users.each {|user| users_bidding_info << {pid: user.id, ranks: bids} } unless bids.uniq == [0]
# Note that priority_info has been renamed to users_bidding_info

Other fixes on the unnecessary looping are demonstrated in examples 2 and 3 of Problem 2. Instead of moving the checking condition out, they each move the filter out so the loop runs in a minimum number of times.

= Testing =

== Test Plan ==

== RSpec Testing ==

=== Existing Tests ===
There were existing RSpec tests for the Lottery Controller, but the tests only provided about 10% coverage of the code in the controller. The test coverage was determined by the Coveralls gem. Most of the existing tests actually didn't even test the functionality of the lottery controller. The existing tests included the following:

# Testing an http get request to google.com and expecting a 200 response code with the content-type of application/json This was an attempt at testing the web service used by the lottery controller
# Checking if an assignment had a correctly set is_intelligent attribute Didn't call any methods of the lottery controller
# Creating new team for a bid
# Sorting unassigned teams Didn't call any methods of the lottery controller

=== After Refactoring ===

After refactoring the Lottery Controller, the long methods were broken down into smaller, more simpler methods. This required a new RSpec test for each of the new methods, but it allowed for increased code coverage. In order to effectively test the rest of the controller, testing objects that mimic the expected application objects were created. The testing objects were created from the factories.rb file using the factory_bot_rails gem and the double feature of the RSpec gem. The objects are also recreated before each test so that the tests are independent from one another to truly validate the different testing sections. The added tests were able to expand the code coverage from about 10% to 99%. The new tests that were added for each of the newly condensed methods include testing for the following:

# Constructing the hash table for the users' bidding information
# A more exhaustive running Intelligent Assignment method
# Generate the hash for a whole team's bidding information
# Match new teams to a topic based on their bids
# Remove a user from a team
# Destroying teams that have no users
# Merging the bids from teams that have merged together so that they have a weighted uniform bid
# Assigning open topics to teams that do not have a topic yet
# Matching teams to topics based on their weighted bids

=== Running the Tests ===
These tests can be executed by utilizing the RSpec command in the Command Prompt window by entering:

<code> user:expertiza $rspec spec/controllers/lottery_controller_spec.rb </code>

== UI Testing (For Reviewers) ==

By following the below steps, you can manually test our refactored <code>LotteryController</code> class.

'''Note:''' The previous implementation does the intelligent assignment on a team basis. That is, students who do not belong to any team will not get assigned to topics (see method <code>construct_users_bidding_info</code>). We intended not to disrupt this behavior since it's beyond the scope of this refactoring.

# Visit [xxx]. Enter the credentials: '''Username:''' instructor6 '''Password:''' password
# From the menubar, hover on the '''Manage''' tab, and select '''Assignments'''.
# Scroll down the page and find the assignment with the name '''New test assignment'''. The orange book icon on the right links to the <code>run_intelligent_assignment</code> method of the <code>LotteryController</code> class. When you click on that, you should receive an error message claiming "500 Internal Server Error". This is because this assignment has not properly set up its participants. To get the intelligent assignment to run, you should click on the pencil icon, navigate to "Other stuff -> Add participant", and add the student by entering its user_id to the textbox next to the label "Enter a user login:".

'''Additional configuring:'''

# Under the '''General''' tab, mark the checkbox '''Has teams''' as true and set the maximum number of members per team to number that makes sense (e.g. our demonstration uses 3).
# You may also want to login to the account of each student you add (they all have "**password**" as their passwords), and help that student bid the topics just like how you did with our OSS project! The algorithm works more effectively when there are bids presented in the system. And by signing up topics, you indirectly create teams for them. We know this process can be very tedious, so thank you ahead for your patience!

Here are some data we used in our recording, you are free to come up with your own!

| User id | First Choice | Second Choice | Third Choice |
| ---------------------------------- | ------------ | ------------- | ------------ |
| student558 | 1 | | |
| student559, student562, student563 | 2 | 3 | 1 |
| student560, student561 | 2 | 1 | |
| student564 | 1 | 2 | |
| student565 | 3 | | |
| student566 | 2 | | |

Return to the '''Assignments''' page after you set up the New test assignment, and retry the orange book icon. You should be indicated that the intelligent assignment has run successfully. You can check the teaming result under the '''Topics''' tab of the edit page. The above data generate the teaming result of the following.

= Future Improvements =

# Improving the RSpec test for assigning open topics for teams without topics. The new test only checks for 1 open assignment and 1 unassigned team. The method is capable of assigning multiple open topics for multiple unassigned teams, but testing for this would require more Factory objects for assignments and teams. While the new test provides code coverage for the entire method, the nested loops currently only run through a single time before completion, which is not always the case in a realistic setting.
# Improving the time-space complexity for the method that merges bids from teams that are merging together. The existing method requires iterating through each member's bidding information to create a hash map. Then there is a nest iteration for the included topics. Afterwards there is an additional iterations through to exclude topics without bids.

CSC/ECE 517 Spring 2020/E2012. refactor lottery controller.rb

2020-03-30T15:38:25Z

Thwinter: /* Testing */

= Introduction =

== Expertiza Background ==

Expertiza is an open-source project written using the Ruby on Rails framework. The Expertiza platform aims to improve student learning by using active and cooperative learning, allowing distance education students to participate in active learning, and by discouraging plagiarism through the use of multiple deadlines. Expertiza helps to improve teaching and resource utilization by having students contribute to the creation of course materials and through peer-graded assignments. Expertiza manages objects related to a course such as instructors, TAs, students, and other users. It also manages assignments, teams, signup topics, rubrics, peer grading, and peer evaluations.

== Lottery Controller Background ==

An assignment can have a list of signup topics. The teams associated with the assignment can bid on the signup topics to try and get assigned one they find more favorable. At a high level, the lottery controller assigns teams to topics based on the priorities the team gave to each signup topic during the bidding process.

In more detail, each student starts off on a team of size 1. They can bid on signup topics by arranging them in priority order. A team can invite other users to join their team. If student2 from Team B joins Team A which includes student1, student2 will lose their bids and take on the bids of TeamA and student1.
When the lottery controller is called to run its method run_intelligent_assignment, a web service takes the bidding data from each team and returns a new list of teams, each of which is closer to the maximum team size specified for the assignment. The web service combines teams together that have similar
bid data and then assigns those new teams to topics, giving each team their top bid on a topic that hasn't been assigned yet. Teams with larger team sizes and more bids are assigned their topics first.

== Problem Statement ==

Code Climate currently grades <code>lottery_controller.rb</code> at a C level, finding issues with the controller's long methods and complex statements. Most methods are around 40 lines and hard to decipher with a quick look. Also, the file currently has only 10% test coverage. Our goal is to resolve the Code Climate issues, make methods simpler to understand, and write new tests to improve test coverage.

== Tasks Identified ==

# Cut down on code in run_intelligent_assignment method by moving some code to helper methods
# Cut down on code in create_new_teams_for_bidding_response by moving some code to helper methods
# Cut down on code in match_new_teams_to_topics by moving some code to helper methods
# Reduce cognitive complexity of merge_bids_from_different_previous_teams method
# Add additional comments to clarify newly written methods
# Add additional RSpec tests to increase the coverage percentage

= Implementation =

We did the following refactoring to the <code>LotteryController</code> class:

# Added 6 helper methods (<code>construct_users_bidding_info</code>, <code>construct_teams_bidding_info</code>, <code>remove_user_from_previous_team</code>, <code>remove_empty_teams</code>, <code>assign_available_slots</code>) that each represent a specific and meaningful step
# Replaced large chunks of code with a small number of method calls
# Reduced the cognitive complexity by reducing the level of nested loops
# Optimized the performance by moving conditional checking and input filtering outside of loops, where appropriate
# Renamed some variable names to make their purpose clearer
# Refactored the parameter list to give each method the least amount of information needed to complete its task
# Added addition comments and fixed some typos

Here are some examples of the problems we identified and the solutions we found to these problems.

=== Problem 1: Long Methods ===

The code climate analysis tool identified two long methods exceeding 25 lines of code per method, <code>run_intelligent_assignment</code> and <code>match_new_teams_to_topics</code>. While some chunks of code are short and intuitive, others can be pulled out and put into helper methods with more meaningful method names.

====Solutions ====

=====run_intelligent_assignment=====

1. Put lines 19-29 into a separate method called <code>construct_users_bidding_info</code> that is responsible for generating user bidding information hash for students on a team who haven't signed up for a topic yet

[[Media:E2012_1.png| view code changes]]

2. Place the code closest to where it is needed. e.g. place line 32 (the <code>url</code> variable initialization) inside the <code>begin/rescue</code> block because it is considered an integral part of the REST API call.

[[Media:E2012_2.png| view code changes]]

===== match_new_teams_to_topics =====

1. Conclude lines 147-157 with a new method <code>construct_teams_bidding_info</code> that is responsible for generating team bidding information needed for the <code>assign_available_slots</code> method.

2. Conclude lines 161-169 with a new method <code>assign_available_slots</code> that uses the result returned from <code>construct_teams_bidding_info</code> to match unassigned teams to available topics.

These two steps greatly shorten the <code>match_new_teams_to_topics</code> method and make the workflow more intuitive to code maintainers.

[[Media:E2012_3.png| view code changes]]

=== Problem 2: Verbose Code ===

Verbosity contributes to more lines of codes and increases the cognitive complexity, which is the measure of how difficult a unit of code is to intuitively understand. Method extraction can't be applied to every long method as some methods only do one thing. For these methods, one can keep them simple with alternative language syntax and other ways of thinking of the behavior.

==== Solutions ====

===== Example 1 =====

# Before
bidding_matrix = {}
assignment.sign_up_topics.each_with_index do |topic, index|
bidding_matrix[topic.id] = [] unless bidding_matrix[topic.id]
bidding_matrix[topic.id] << bids[index]
end

The above code segment ensures that <code>bidding_matrix[topic.id]</code> returns an array object before pushing a new value to it. It is inelegant both in its readability and the time wasted to visit each <code>bidding_matrix[topic.id]</code> twice. The same behavior can be accomplished by constructing a Hash object that knows what default value to return when the key has not been seen before.

# After
bidding_matrix = Hash.new {|hash, key| hash[key] = [] }
sign_up_topics.each_with_index do |topic, index|
bidding_matrix[topic.id] << bids[index]
end

=====Example 2=====

# Before
teams.each do |team|
next if SignedUpTeam.where(team_id: team.id, is_waitlisted: 0).any?
... (rest of code omitted)
end

This block is extracted from the original implementation that, like many places in the <code>LotteryController</code>, uses <code>next</code> to skip any elements that don't fulfill the imposed condition. It's more reasonable to eliminate out unwanted elements outside of the loop using either <code>select</code> or <code>reject</code> call. Below is the modification made to this block.

# After
teams_not_signed_up = teams.reject {|team| SignedUpTeam.where(team_id: team.id, is_waitlisted: 0).any? }
teams_not_signed_up.each do |team|
... (rest of code omitted)
end

===== Example 3 =====

Same as example 1, the following block uses <code>next</code> to skip unwanted <code>team_user</code>s until the one that meets the condition, that is, who belongs to the specified assignment. This code does the filtering twice, which can be combined with one query.

# Before
TeamsUser.where(user_id: user_id).each do |team_user|
next unless team_user.team.parent_id == assignment.id
team_user.team_user_node.destroy rescue nil
team_user.destroy rescue nil
break
end

We first tried to use <code>find_by</code> with two parameters. <code>find_by</code> works by pulling column values from the model object. Since the <code>TeamsUser</code> model does not have a <code>team</code> attribute (it only stores <code>team_id</code>), this attempt is syntactically illegal.

team_user = TeamsUser.find_by(user_id: user_id, team.parent_id: assignment_id)
team_user.team_user_node.destroy rescue nil
team_user.destroy rescue nil

The way we refactored the code is to use the <code>find</code> method from the <code>Enumerable</code> mixin to get the first element for which the block is not false. In this way, the loop will no longer be necessary since the only thing it does is finding the first matching object.

# After
team_user = TeamsUser.where(user_id: user_id).find{|team_user| team_user.team.parent_id == assignment_id }
team_user.team_user_node.destroy rescue nil
team_user.destroy rescue nil

===== Example 4 =====

Besides pulling unwanted elements out before entering the loop, one can further reduce the verbosity of the code by replacing <code>next if</code> with <code>unless</code> and adding this checking step to the only statement that's inside the loop.

# Before
bidding_matrix.each do |topic_id, value|
next if value.inject(:+).zero?
bidding_matrix_summary << [value.count {|i| i != 0 }, value.inject(:+), topic_id]
end

# After
bidding_matrix.each do |topic_id, value|
bidding_matrix_summary << [value.count {|i| i != 0 }, value.inject(:+), topic_id] unless value.inject(:+).zero?
end

=== Problem 3: Ambiguous Identifiers ===

There were many puzzling variable names in the original <code>lottery_controller.rb</code> file. For example, copied from line 30, the slot with the label <code>users</code> accepts an input of <code>priority_info</code>.
bidding_data = {users: priority_info, max_team_size: assignment.max_team_size}

The <code>priority_info</code> is an array of hashes, each hash stores a pair of user id and bids that the user has made so far. This variable name is not descriptive enough to visualize its content. It is refactored into a new name, <code>users_bidding_info</code>, and the corresponding construction method is therefore named <code>construct_users_bidding_info</code>.

There is another array that has a very similar structure to the <code>users_bidding_info</code>, except that it holds bidding information for teams rather than users. To follow a good naming consistency, this array is granted a new name called <code>teams_bidding_info</code> (previously named <code>team_bids</code>) and the name of the corresponding construction method is changed to <code>construct_teams_bidding_info</code>.

'''Notes'''

We originally considered to change the label <code>ranks</code> to <code>bids</code> because the same identity has been presented in three different forms throughout the file: <code>rank</code>, <code>bid</code>, and <code>priority</code>.
|user| priority_info << {pid: user.id, ranks: bids}
However, such a change could have an impact on the later REST API call to the web service that uses students' bidding data to build teams automatically. We are uncertain about how the bidding data is actually used at the other end so it is not safe to apply such a change.

=== Problem 4: Multi-Purposed Methods ===

The <code>create_new_teams_for_bidding_response</code> method does more than 1 thing, both creating new teams and deleting residual teams that none of their team members remained in their teams. Since it is doing more beyond just what the name implies, it is reasonable to extract the removing part of the code to a new method and place the method call after the call to the <code>create_new_teams_for_bidding_response</code> method.

# Before
create_new_teams_for_bidding_response(teams, assignment, priority_info)
# ^^ <code>create_new_teams_for_bidding_response</code> is responsible for both creation and deletion of AssignmentTeam objects
match_new_teams_to_topics(assignment)

# After
create_new_teams_for_bidding_response(teams, assignment, users_bidding_info)
remove_empty_teams(assignment)
# ^^ <code>remove_empty_teams</code> becomes a separate method
match_new_teams_to_topics(assignment)

=== Problem 5: Unnecessary Looping ===

Many unnecessary looping can be avoided by moving the conditional check out. For example, consider this piece of code.

#Before
bids = []
topics.each do |topic|
bid_record = Bid.find_by(team_id: team.id, topic_id: topic.id)
bids << (bid_record.nil? ? 0 : bid_record.priority ||= 0)
end
team.users.each {|user| priority_info << {pid: user.id, ranks: bids} if bids.uniq != [0] }

The last line shows a looping on the team users that for each user in the team, it pushes its user id along with the generated bids to the <code>priority_info</code> array. If there are 3 users in a team, then the comparison <code>if bids.uniq != [0]</code> always gets executed 3 times while it should only be executed once because at this point the <code>bids</code> variable will not be changed anymore. The solution to this performance drawback would be to change the last line to the code below:

# After
team.users.each {|user| users_bidding_info << {pid: user.id, ranks: bids} } unless bids.uniq == [0]
# Note that priority_info has been renamed to users_bidding_info

Other fixes on the unnecessary looping are demonstrated in examples 2 and 3 of Problem 2. Instead of moving the checking condition out, they each move the filter out so the loop runs in a minimum number of times.

= Testing =

== Test Plan ==

== RSpec Testing ==

=== Existing Tests ===
There were existing RSpec tests for the Lottery Controller, but the tests only provided about 10% coverage of the code in the controller. The test coverage was determined by the Coveralls gem. Most of the existing tests actually didn't even test the functionality of the lottery controller. The existing tests included the following:

# Testing an http get request to google.com and expecting a 200 response code with the content-type of application/json This was an attempt at testing the web service used by the lottery controller
# Checking if an assignment had a correctly set is_intelligent attribute Didn't call any methods of the lottery controller
# Creating new team for a bid
# Sorting unassigned teams Didn't call any methods of the lottery controller

=== After Refactoring ===

After refactoring the Lottery Controller, the long methods were broken down into smaller, more simpler methods. This required a new RSpec test for each of the new methods, but it allowed for increased code coverage. In order to effectively test the rest of the controller, testing objects that mimic the expected application objects were created. The testing objects were created from the factories.rb file using the factory_bot_rails gem and the double feature of the RSpec gem. The objects are also recreated before each test so that the tests are independent from one another to truly validate the different testing sections. The added tests were able to expand the code coverage from about 10% to 97%. The new tests that were added for each of the newly condensed methods include testing for the following:

# Constructing the hash table for the users' bidding information
# A more exhaustive running Intelligent Assignment method
# Generate the hash for a whole team's bidding information
# Match new teams to a topic based on their bids
# Remove a user from a team
# Destroying teams that have no users
# Merging the bids from teams that have merged together so that they have a weighted uniform bid
# Assigning open topics to teams that do not have a topic yet
# Matching teams to topics based on their weighted bids

=== Running the Tests ===
These tests can be executed by utilizing the RSpec command in the Command Prompt window by entering:

<code> user:expertiza $rspec spec/controllers/lottery_controller_spec.rb </code>

== UI Testing (For Reviewers) ==

By following the below steps, you can manually test our refactored <code>LotteryController</code> class.

'''Note:''' The previous implementation does the intelligent assignment on a team basis. That is, students who do not belong to any team will not get assigned to topics (see method <code>construct_users_bidding_info</code>). We intended not to disrupt this behavior since it's beyond the scope of this refactoring.

# Visit [xxx]. Enter the credentials: '''Username:''' instructor6 '''Password:''' password
# From the menubar, hover on the '''Manage''' tab, and select '''Assignments'''.
# Scroll down the page and find the assignment with the name '''New test assignment'''. The orange book icon on the right links to the <code>run_intelligent_assignment</code> method of the <code>LotteryController</code> class. When you click on that, you should receive an error message claiming "500 Internal Server Error". This is because this assignment has not properly set up its participants. To get the intelligent assignment to run, you should click on the pencil icon, navigate to "Other stuff -> Add participant", and add the student by entering its user_id to the textbox next to the label "Enter a user login:".

'''Additional configuring:'''

# Under the '''General''' tab, mark the checkbox '''Has teams''' as true and set the maximum number of members per team to number that makes sense (e.g. our demonstration uses 3).
# You may also want to login to the account of each student you add (they all have "**password**" as their passwords), and help that student bid the topics just like how you did with our OSS project! The algorithm works more effectively when there are bids presented in the system. And by signing up topics, you indirectly create teams for them. We know this process can be very tedious, so thank you ahead for your patience!

Here are some data we used in our recording, you are free to come up with your own!

| User id | First Choice | Second Choice | Third Choice |
| ---------------------------------- | ------------ | ------------- | ------------ |
| student558 | 1 | | |
| student559, student562, student563 | 2 | 3 | 1 |
| student560, student561 | 2 | 1 | |
| student564 | 1 | 2 | |
| student565 | 3 | | |
| student566 | 2 | | |

Return to the '''Assignments''' page after you set up the New test assignment, and retry the orange book icon. You should be indicated that the intelligent assignment has run successfully. You can check the teaming result under the '''Topics''' tab of the edit page. The above data generate the teaming result of the following.

= Future Improvements =

# Improving the RSpec test for assigning open topics for teams without topics. The new test only checks for 1 open assignment and 1 unassigned team. The method is capable of assigning multiple open topics for multiple unassigned teams, but testing for this would require more Factory objects for assignments and teams. While the new test provides code coverage for the entire method, the nested loops currently only run through a single time before completion, which is not always the case in a realistic setting.
# Improving the time-space complexity for the method that merges bids from teams that are merging together. The existing method requires iterating through each member's bidding information to create a hash map. Then there is a nest iteration for the included topics. Afterwards there is an additional iterations through to exclude topics without bids.

CSC/ECE 517 Spring 2020/E2012. refactor lottery controller.rb

2020-03-30T15:37:46Z

Thwinter: /* Running the Tests */

= Introduction =

== Expertiza Background ==

Expertiza is an open-source project written using the Ruby on Rails framework. The Expertiza platform aims to improve student learning by using active and cooperative learning, allowing distance education students to participate in active learning, and by discouraging plagiarism through the use of multiple deadlines. Expertiza helps to improve teaching and resource utilization by having students contribute to the creation of course materials and through peer-graded assignments. Expertiza manages objects related to a course such as instructors, TAs, students, and other users. It also manages assignments, teams, signup topics, rubrics, peer grading, and peer evaluations.

== Lottery Controller Background ==

An assignment can have a list of signup topics. The teams associated with the assignment can bid on the signup topics to try and get assigned one they find more favorable. At a high level, the lottery controller assigns teams to topics based on the priorities the team gave to each signup topic during the bidding process.

In more detail, each student starts off on a team of size 1. They can bid on signup topics by arranging them in priority order. A team can invite other users to join their team. If student2 from Team B joins Team A which includes student1, student2 will lose their bids and take on the bids of TeamA and student1.
When the lottery controller is called to run its method run_intelligent_assignment, a web service takes the bidding data from each team and returns a new list of teams, each of which is closer to the maximum team size specified for the assignment. The web service combines teams together that have similar
bid data and then assigns those new teams to topics, giving each team their top bid on a topic that hasn't been assigned yet. Teams with larger team sizes and more bids are assigned their topics first.

== Problem Statement ==

Code Climate currently grades <code>lottery_controller.rb</code> at a C level, finding issues with the controller's long methods and complex statements. Most methods are around 40 lines and hard to decipher with a quick look. Also, the file currently has only 10% test coverage. Our goal is to resolve the Code Climate issues, make methods simpler to understand, and write new tests to improve test coverage.

== Tasks Identified ==

# Cut down on code in run_intelligent_assignment method by moving some code to helper methods
# Cut down on code in create_new_teams_for_bidding_response by moving some code to helper methods
# Cut down on code in match_new_teams_to_topics by moving some code to helper methods
# Reduce cognitive complexity of merge_bids_from_different_previous_teams method
# Add additional comments to clarify newly written methods
# Add additional RSpec tests to increase the coverage percentage

= Implementation =

We did the following refactoring to the <code>LotteryController</code> class:

# Added 6 helper methods (<code>construct_users_bidding_info</code>, <code>construct_teams_bidding_info</code>, <code>remove_user_from_previous_team</code>, <code>remove_empty_teams</code>, <code>assign_available_slots</code>) that each represent a specific and meaningful step
# Replaced large chunks of code with a small number of method calls
# Reduced the cognitive complexity by reducing the level of nested loops
# Optimized the performance by moving conditional checking and input filtering outside of loops, where appropriate
# Renamed some variable names to make their purpose clearer
# Refactored the parameter list to give each method the least amount of information needed to complete its task
# Added addition comments and fixed some typos

Here are some examples of the problems we identified and the solutions we found to these problems.

=== Problem 1: Long Methods ===

The code climate analysis tool identified two long methods exceeding 25 lines of code per method, <code>run_intelligent_assignment</code> and <code>match_new_teams_to_topics</code>. While some chunks of code are short and intuitive, others can be pulled out and put into helper methods with more meaningful method names.

====Solutions ====

=====run_intelligent_assignment=====

1. Put lines 19-29 into a separate method called <code>construct_users_bidding_info</code> that is responsible for generating user bidding information hash for students on a team who haven't signed up for a topic yet

[[Media:E2012_1.png| view code changes]]

2. Place the code closest to where it is needed. e.g. place line 32 (the <code>url</code> variable initialization) inside the <code>begin/rescue</code> block because it is considered an integral part of the REST API call.

[[Media:E2012_2.png| view code changes]]

===== match_new_teams_to_topics =====

1. Conclude lines 147-157 with a new method <code>construct_teams_bidding_info</code> that is responsible for generating team bidding information needed for the <code>assign_available_slots</code> method.

2. Conclude lines 161-169 with a new method <code>assign_available_slots</code> that uses the result returned from <code>construct_teams_bidding_info</code> to match unassigned teams to available topics.

These two steps greatly shorten the <code>match_new_teams_to_topics</code> method and make the workflow more intuitive to code maintainers.

[[Media:E2012_3.png| view code changes]]

=== Problem 2: Verbose Code ===

Verbosity contributes to more lines of codes and increases the cognitive complexity, which is the measure of how difficult a unit of code is to intuitively understand. Method extraction can't be applied to every long method as some methods only do one thing. For these methods, one can keep them simple with alternative language syntax and other ways of thinking of the behavior.

==== Solutions ====

===== Example 1 =====

# Before
bidding_matrix = {}
assignment.sign_up_topics.each_with_index do |topic, index|
bidding_matrix[topic.id] = [] unless bidding_matrix[topic.id]
bidding_matrix[topic.id] << bids[index]
end

The above code segment ensures that <code>bidding_matrix[topic.id]</code> returns an array object before pushing a new value to it. It is inelegant both in its readability and the time wasted to visit each <code>bidding_matrix[topic.id]</code> twice. The same behavior can be accomplished by constructing a Hash object that knows what default value to return when the key has not been seen before.

# After
bidding_matrix = Hash.new {|hash, key| hash[key] = [] }
sign_up_topics.each_with_index do |topic, index|
bidding_matrix[topic.id] << bids[index]
end

=====Example 2=====

# Before
teams.each do |team|
next if SignedUpTeam.where(team_id: team.id, is_waitlisted: 0).any?
... (rest of code omitted)
end

This block is extracted from the original implementation that, like many places in the <code>LotteryController</code>, uses <code>next</code> to skip any elements that don't fulfill the imposed condition. It's more reasonable to eliminate out unwanted elements outside of the loop using either <code>select</code> or <code>reject</code> call. Below is the modification made to this block.

# After
teams_not_signed_up = teams.reject {|team| SignedUpTeam.where(team_id: team.id, is_waitlisted: 0).any? }
teams_not_signed_up.each do |team|
... (rest of code omitted)
end

===== Example 3 =====

Same as example 1, the following block uses <code>next</code> to skip unwanted <code>team_user</code>s until the one that meets the condition, that is, who belongs to the specified assignment. This code does the filtering twice, which can be combined with one query.

# Before
TeamsUser.where(user_id: user_id).each do |team_user|
next unless team_user.team.parent_id == assignment.id
team_user.team_user_node.destroy rescue nil
team_user.destroy rescue nil
break
end

We first tried to use <code>find_by</code> with two parameters. <code>find_by</code> works by pulling column values from the model object. Since the <code>TeamsUser</code> model does not have a <code>team</code> attribute (it only stores <code>team_id</code>), this attempt is syntactically illegal.

team_user = TeamsUser.find_by(user_id: user_id, team.parent_id: assignment_id)
team_user.team_user_node.destroy rescue nil
team_user.destroy rescue nil

The way we refactored the code is to use the <code>find</code> method from the <code>Enumerable</code> mixin to get the first element for which the block is not false. In this way, the loop will no longer be necessary since the only thing it does is finding the first matching object.

# After
team_user = TeamsUser.where(user_id: user_id).find{|team_user| team_user.team.parent_id == assignment_id }
team_user.team_user_node.destroy rescue nil
team_user.destroy rescue nil

===== Example 4 =====

Besides pulling unwanted elements out before entering the loop, one can further reduce the verbosity of the code by replacing <code>next if</code> with <code>unless</code> and adding this checking step to the only statement that's inside the loop.

# Before
bidding_matrix.each do |topic_id, value|
next if value.inject(:+).zero?
bidding_matrix_summary << [value.count {|i| i != 0 }, value.inject(:+), topic_id]
end

# After
bidding_matrix.each do |topic_id, value|
bidding_matrix_summary << [value.count {|i| i != 0 }, value.inject(:+), topic_id] unless value.inject(:+).zero?
end

=== Problem 3: Ambiguous Identifiers ===

There were many puzzling variable names in the original <code>lottery_controller.rb</code> file. For example, copied from line 30, the slot with the label <code>users</code> accepts an input of <code>priority_info</code>.
bidding_data = {users: priority_info, max_team_size: assignment.max_team_size}

The <code>priority_info</code> is an array of hashes, each hash stores a pair of user id and bids that the user has made so far. This variable name is not descriptive enough to visualize its content. It is refactored into a new name, <code>users_bidding_info</code>, and the corresponding construction method is therefore named <code>construct_users_bidding_info</code>.

There is another array that has a very similar structure to the <code>users_bidding_info</code>, except that it holds bidding information for teams rather than users. To follow a good naming consistency, this array is granted a new name called <code>teams_bidding_info</code> (previously named <code>team_bids</code>) and the name of the corresponding construction method is changed to <code>construct_teams_bidding_info</code>.

'''Notes'''

We originally considered to change the label <code>ranks</code> to <code>bids</code> because the same identity has been presented in three different forms throughout the file: <code>rank</code>, <code>bid</code>, and <code>priority</code>.
|user| priority_info << {pid: user.id, ranks: bids}
However, such a change could have an impact on the later REST API call to the web service that uses students' bidding data to build teams automatically. We are uncertain about how the bidding data is actually used at the other end so it is not safe to apply such a change.

=== Problem 4: Multi-Purposed Methods ===

The <code>create_new_teams_for_bidding_response</code> method does more than 1 thing, both creating new teams and deleting residual teams that none of their team members remained in their teams. Since it is doing more beyond just what the name implies, it is reasonable to extract the removing part of the code to a new method and place the method call after the call to the <code>create_new_teams_for_bidding_response</code> method.

# Before
create_new_teams_for_bidding_response(teams, assignment, priority_info)
# ^^ <code>create_new_teams_for_bidding_response</code> is responsible for both creation and deletion of AssignmentTeam objects
match_new_teams_to_topics(assignment)

# After
create_new_teams_for_bidding_response(teams, assignment, users_bidding_info)
remove_empty_teams(assignment)
# ^^ <code>remove_empty_teams</code> becomes a separate method
match_new_teams_to_topics(assignment)

=== Problem 5: Unnecessary Looping ===

Many unnecessary looping can be avoided by moving the conditional check out. For example, consider this piece of code.

#Before
bids = []
topics.each do |topic|
bid_record = Bid.find_by(team_id: team.id, topic_id: topic.id)
bids << (bid_record.nil? ? 0 : bid_record.priority ||= 0)
end
team.users.each {|user| priority_info << {pid: user.id, ranks: bids} if bids.uniq != [0] }

The last line shows a looping on the team users that for each user in the team, it pushes its user id along with the generated bids to the <code>priority_info</code> array. If there are 3 users in a team, then the comparison <code>if bids.uniq != [0]</code> always gets executed 3 times while it should only be executed once because at this point the <code>bids</code> variable will not be changed anymore. The solution to this performance drawback would be to change the last line to the code below:

# After
team.users.each {|user| users_bidding_info << {pid: user.id, ranks: bids} } unless bids.uniq == [0]
# Note that priority_info has been renamed to users_bidding_info

Other fixes on the unnecessary looping are demonstrated in examples 2 and 3 of Problem 2. Instead of moving the checking condition out, they each move the filter out so the loop runs in a minimum number of times.

= Testing =

== RSpec Testing ==

=== Existing Tests ===
There were existing RSpec tests for the Lottery Controller, but the tests only provided about 10% coverage of the code in the controller. The test coverage was determined by the Coveralls gem. Most of the existing tests actually didn't even test the functionality of the lottery controller. The existing tests included the following:

# Testing an http get request to google.com and expecting a 200 response code with the content-type of application/json This was an attempt at testing the web service used by the lottery controller
# Checking if an assignment had a correctly set is_intelligent attribute Didn't call any methods of the lottery controller
# Creating new team for a bid
# Sorting unassigned teams Didn't call any methods of the lottery controller

=== After Refactoring ===

After refactoring the Lottery Controller, the long methods were broken down into smaller, more simpler methods. This required a new RSpec test for each of the new methods, but it allowed for increased code coverage. In order to effectively test the rest of the controller, testing objects that mimic the expected application objects were created. The testing objects were created from the factories.rb file using the factory_bot_rails gem and the double feature of the RSpec gem. The objects are also recreated before each test so that the tests are independent from one another to truly validate the different testing sections. The added tests were able to expand the code coverage from about 10% to 97%. The new tests that were added for each of the newly condensed methods include testing for the following:

# Constructing the hash table for the users' bidding information
# A more exhaustive running Intelligent Assignment method
# Generate the hash for a whole team's bidding information
# Match new teams to a topic based on their bids
# Remove a user from a team
# Destroying teams that have no users
# Merging the bids from teams that have merged together so that they have a weighted uniform bid
# Assigning open topics to teams that do not have a topic yet
# Matching teams to topics based on their weighted bids

=== Running the Tests ===
These tests can be executed by utilizing the RSpec command in the Command Prompt window by entering:

<code> user:expertiza $rspec spec/controllers/lottery_controller_spec.rb </code>

== UI Testing (For Reviewers) ==

By following the below steps, you can manually test our refactored <code>LotteryController</code> class.

'''Note:''' The previous implementation does the intelligent assignment on a team basis. That is, students who do not belong to any team will not get assigned to topics (see method <code>construct_users_bidding_info</code>). We intended not to disrupt this behavior since it's beyond the scope of this refactoring.

# Visit [xxx]. Enter the credentials: '''Username:''' instructor6 '''Password:''' password
# From the menubar, hover on the '''Manage''' tab, and select '''Assignments'''.
# Scroll down the page and find the assignment with the name '''New test assignment'''. The orange book icon on the right links to the <code>run_intelligent_assignment</code> method of the <code>LotteryController</code> class. When you click on that, you should receive an error message claiming "500 Internal Server Error". This is because this assignment has not properly set up its participants. To get the intelligent assignment to run, you should click on the pencil icon, navigate to "Other stuff -> Add participant", and add the student by entering its user_id to the textbox next to the label "Enter a user login:".

'''Additional configuring:'''

# Under the '''General''' tab, mark the checkbox '''Has teams''' as true and set the maximum number of members per team to number that makes sense (e.g. our demonstration uses 3).
# You may also want to login to the account of each student you add (they all have "**password**" as their passwords), and help that student bid the topics just like how you did with our OSS project! The algorithm works more effectively when there are bids presented in the system. And by signing up topics, you indirectly create teams for them. We know this process can be very tedious, so thank you ahead for your patience!

Here are some data we used in our recording, you are free to come up with your own!

| User id | First Choice | Second Choice | Third Choice |
| ---------------------------------- | ------------ | ------------- | ------------ |
| student558 | 1 | | |
| student559, student562, student563 | 2 | 3 | 1 |
| student560, student561 | 2 | 1 | |
| student564 | 1 | 2 | |
| student565 | 3 | | |
| student566 | 2 | | |

Return to the '''Assignments''' page after you set up the New test assignment, and retry the orange book icon. You should be indicated that the intelligent assignment has run successfully. You can check the teaming result under the '''Topics''' tab of the edit page. The above data generate the teaming result of the following.

= Future Improvements =

# Improving the RSpec test for assigning open topics for teams without topics. The new test only checks for 1 open assignment and 1 unassigned team. The method is capable of assigning multiple open topics for multiple unassigned teams, but testing for this would require more Factory objects for assignments and teams. While the new test provides code coverage for the entire method, the nested loops currently only run through a single time before completion, which is not always the case in a realistic setting.
# Improving the time-space complexity for the method that merges bids from teams that are merging together. The existing method requires iterating through each member's bidding information to create a hash map. Then there is a nest iteration for the included topics. Afterwards there is an additional iterations through to exclude topics without bids.

CSC/ECE 517 Spring 2020/E2012. refactor lottery controller.rb

2020-03-24T01:08:01Z

Thwinter: /* Future Improvements */

CSC/ECE 517 Spring 2020/E2012. refactor lottery controller.rb

2020-03-24T00:18:30Z

Thwinter: /* After Refactoring */

CSC/ECE 517 Spring 2020/E2012. refactor lottery controller.rb

2020-03-24T00:16:23Z

Thwinter: /* After Refactoring */

CSC/ECE 517 Spring 2020/E2012. refactor lottery controller.rb

2020-03-24T00:16:02Z

Thwinter: /* After Refactoring */

CSC/ECE 517 Spring 2020/E2012. refactor lottery controller.rb

2020-03-24T00:15:48Z

Thwinter: /* After Refactoring */

CSC/ECE 517 Spring 2020/E2012. refactor lottery controller.rb

2020-03-24T00:13:25Z

Thwinter: Future Improvements

CSC/ECE 517 Spring 2020/E2012. refactor lottery controller.rb

2020-03-24T00:13:00Z

Thwinter: RSpec Testing