CSC/ECE 517 Spring 2020/E2012. refactor lottery controller.rb: Difference between revisions
m (→Existing Tests) |
|||
(21 intermediate revisions by 3 users not shown) | |||
Line 10: | Line 10: | ||
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. | 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 | When the lottery controller is called to run its method <code>run_intelligent_assignment</code>, 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. | 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 == | == Problem Statement == | ||
Line 19: | Line 19: | ||
== Tasks Identified == | == Tasks Identified == | ||
# Cut down on code in run_intelligent_assignment method by moving some code to helper methods | # Cut down on code in <code>run_intelligent_assignment</code> 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 <code>create_new_teams_for_bidding_response</code> method by moving some code to helper methods | ||
# Cut down on code in match_new_teams_to_topics by moving some code to helper methods | # Cut down on code in <code>match_new_teams_to_topics</code> method by moving some code to helper methods | ||
# Reduce cognitive complexity of merge_bids_from_different_previous_teams method | # Reduce cognitive complexity of <code>merge_bids_from_different_previous_teams</code> method | ||
# Add additional comments to clarify newly written methods | # Add additional comments to clarify newly written methods | ||
# Add additional RSpec tests to increase the coverage percentage | # Add additional RSpec tests to increase the coverage percentage | ||
Line 36: | Line 36: | ||
# Renamed some variable names to make their purpose clearer | # 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 | # Refactored the parameter list to give each method the least amount of information needed to complete its task | ||
# Added | # Added additional comments and fixed some typos | ||
Here are some examples of the problems we identified and the solutions we found to these problems. | Here are some examples of the problems we identified and the solutions we found to these problems. | ||
Line 48: | Line 48: | ||
=====run_intelligent_assignment===== | =====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 | 1. Put lines 19-29 into a separate method called <code>construct_users_bidding_info</code> that is responsible for generating a hash containing user bidding information for students on a team who haven't signed up for a topic yet | ||
[[Media:E2012_1.png| view code changes]] | [[Media:E2012_1.png| view code changes]] | ||
Line 68: | Line 68: | ||
=== Problem 2: Verbose Code === | === 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 | 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 simplify them by using alternative language syntax or implementing the same behavior in a different, more concise way. | ||
==== Solutions ==== | ==== Solutions ==== | ||
Line 107: | Line 107: | ||
===== Example 3 ===== | ===== Example 3 ===== | ||
As in example 1, the following block uses <code>next</code> to skip <code>team_user</code>s unless they meet the condition that they belong to the specified assignment. This code does the filtering twice, which can be combined with one query. | |||
| | ||
# Before | # Before | ||
Line 150: | Line 150: | ||
bidding_data = {users: priority_info, max_team_size: assignment.max_team_size} | 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>. | 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. | ||
====Solutions ==== | |||
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>. | 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>. | ||
Line 162: | Line 165: | ||
=== Problem 4: Multi-Purposed Methods === | === 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. | 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. | ||
====Solutions ==== | |||
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 | # Before | ||
Line 187: | Line 192: | ||
team.users.each {|user| priority_info << {pid: user.id, ranks: bids} if bids.uniq != [0] } | 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: | 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. | ||
====Solutions ==== | |||
The solution to this performance drawback would be to change the last line to the code below: | |||
# After | # After | ||
Line 199: | Line 207: | ||
== Test Plan == | == 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 by 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 it to print error message. | |||
#* We did not test <code>run_intelligent_assignment</code> on invalid assignment id since it is beyond the scope of this refactoring. | |||
# 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. This test includes cases like: | |||
#** Team with more than 1 member (assignment_team1) | |||
#** Team that has more than 1 bid (assignment_team2) | |||
#** Team with no bids (assignment_team4) | |||
#** Team with 1 bid of priority 0 (assignment_team3) | |||
# 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 TeamsUser count to remain the same and AssignmentTeam count to increase by 2. (Notice that both TeamNode count and TeamUserNode count increased because we haven't set up a proper number of node objects prior to the test. These numbers increased because this method deletes the old TeamsUser and TeamUserNode objects and replaces them with the new ones. Since there are no node objects to begin with, these numbers would only increase.) | |||
# match_new_teams_to_topics: | |||
#* Set up an intelligent assignment. Set up a team. Set up that team’s bidding information. Execute the method. Expect the assignment to become unintelligent. | |||
#* 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 counts 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 for the new team. | |||
== RSpec Testing == | == RSpec Testing == | ||
=== Existing Tests === | === Existing Tests === | ||
There were existing RSpec tests for the Lottery Controller, but | There were existing RSpec tests for the Lottery Controller, but these 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 | # Testing an HTTP Get request to google.com and expecting a 200 response code with the content-type of application/json<br>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 | # 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 | # Creating a new team for a bid | ||
# Sorting unassigned teams | # Sorting unassigned teams, but it didn't call any methods of the lottery controller | ||
=== After Refactoring === | === After Refactoring === | ||
After refactoring the Lottery Controller, the long methods were broken down into smaller, | After refactoring the Lottery Controller, the long methods were broken down into smaller, 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 of 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 | # Constructing the hash table for the users' bidding information | ||
Line 229: | Line 289: | ||
<code> user:expertiza $rspec spec/controllers/lottery_controller_spec.rb </code> | <code> user:expertiza $rspec spec/controllers/lottery_controller_spec.rb </code> | ||
[https://drive.google.com/open?id=1Kljltykih20bcp87rpG1yNKJgRelPqPk Click here] to see our test result and coverage of the <code>LotteryController</code> class. | |||
== UI Testing (For Reviewers) == | == UI Testing (For Reviewers) == | ||
Line 236: | Line 298: | ||
'''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. | '''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 [ | # Visit [http://152.46.17.241:8080 http://152.46.17.241:8080]. Enter the credentials:<br>'''Username:''' instructor6<br>'''Password:''' password | ||
# From the menubar, hover on the '''Manage''' tab, and select '''Assignments'''. | # 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:". | # 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 (if you can't see the orange book icon, please consult the "Make an assignment intelligent" subsection placed below).<br>[[File:E2012_4.png]]<br>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:". | ||
'''Make an assignment intelligent:''' | |||
# Click on the pencil icon, which leads you to the edit page. | |||
# Under the '''General''' tab, make sure these three checkboxes are selected: | |||
#* Has teams? | |||
#* Has topics? | |||
#* Available to students? | |||
# Under the '''Topics''' tab, make "Enable bidding for topics?" to true. | |||
# Then return to the assignment page, you should see the orange book icon next to the assignment you just edited. | |||
'''Additional configuring:''' | '''Additional configuring:''' | ||
Line 257: | Line 329: | ||
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. | 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]] | |||
[https://drive.google.com/open?id=1KYApz_ZqfWm32Tpj2wiqeB6vXLzaUVhe Click here] to see our demonstration on how to test the <code>run_intelligent_assignment</code> method. | |||
= Future Improvements = | = 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 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. | # 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. Afterward, there is an additional iteration to exclude topics without bids. |
Latest revision as of 19:48, 31 March 2020
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 lottery_controller.rb
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
method by moving some code to helper methods - Cut down on code in
match_new_teams_to_topics
method 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 LotteryController
class:
- Added 6 helper methods (
construct_users_bidding_info
,construct_teams_bidding_info
,remove_user_from_previous_team
,remove_empty_teams
,assign_available_slots
) 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 additional 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, run_intelligent_assignment
and match_new_teams_to_topics
. 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 construct_users_bidding_info
that is responsible for generating a hash containing user bidding information for students on a team who haven't signed up for a topic yet
2. Place the code closest to where it is needed. e.g. place line 32 (the url
variable initialization) inside the begin/rescue
block because it is considered an integral part of the REST API call.
match_new_teams_to_topics
1. Conclude lines 147-157 with a new method construct_teams_bidding_info
that is responsible for generating team bidding information needed for the assign_available_slots
method.
2. Conclude lines 161-169 with a new method assign_available_slots
that uses the result returned from construct_teams_bidding_info
to match unassigned teams to available topics.
These two steps greatly shorten the match_new_teams_to_topics
method and make the workflow more intuitive to code maintainers.
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 simplify them by using alternative language syntax or implementing the same behavior in a different, more concise way.
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 bidding_matrix[topic.id]
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 bidding_matrix[topic.id]
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 LotteryController
, uses next
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 select
or reject
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
As in example 1, the following block uses next
to skip team_user
s unless they meet the condition that they belong 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 find_by
with two parameters. find_by
works by pulling column values from the model object. Since the TeamsUser
model does not have a team
attribute (it only stores team_id
), 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 find
method from the Enumerable
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 next if
with unless
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 lottery_controller.rb
file. For example, copied from line 30, the slot with the label users
accepts an input of priority_info
.
bidding_data = {users: priority_info, max_team_size: assignment.max_team_size}
The priority_info
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.
Solutions
It is refactored into a new name, users_bidding_info
, and the corresponding construction method is therefore named construct_users_bidding_info
.
There is another array that has a very similar structure to the users_bidding_info
, 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 teams_bidding_info
(previously named team_bids
) and the name of the corresponding construction method is changed to construct_teams_bidding_info
.
Notes
We originally considered to change the label ranks
to bids
because the same identity has been presented in three different forms throughout the file: rank
, bid
, and priority
.
|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 create_new_teams_for_bidding_response
method does more than 1 thing, both creating new teams and deleting residual teams that none of their team members remained in their teams.
Solutions
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 create_new_teams_for_bidding_response
method.
# Before
create_new_teams_for_bidding_response(teams, assignment, priority_info)
# ^^ create_new_teams_for_bidding_response
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)
# ^^ remove_empty_teams
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 priority_info
array. If there are 3 users in a team, then the comparison if bids.uniq != [0]
always gets executed 3 times while it should only be executed once because at this point the bids
variable will not be changed anymore.
Solutions
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 by 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 it to print error message.
- We did not test
run_intelligent_assignment
on invalid assignment id since it is beyond the scope of this refactoring.
- 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. This test includes cases like:
- Team with more than 1 member (assignment_team1)
- Team that has more than 1 bid (assignment_team2)
- Team with no bids (assignment_team4)
- Team with 1 bid of priority 0 (assignment_team3)
- 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. This test includes cases like:
- 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 TeamsUser count to remain the same and AssignmentTeam count to increase by 2. (Notice that both TeamNode count and TeamUserNode count increased because we haven't set up a proper number of node objects prior to the test. These numbers increased because this method deletes the old TeamsUser and TeamUserNode objects and replaces them with the new ones. Since there are no node objects to begin with, these numbers would only increase.)
- match_new_teams_to_topics:
- Set up an intelligent assignment. Set up a team. Set up that team’s bidding information. Execute the method. Expect the assignment to become unintelligent.
- 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 counts 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 for the new team.
RSpec Testing
Existing Tests
There were existing RSpec tests for the Lottery Controller, but these 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 a 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, 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 of 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:
user:expertiza $rspec spec/controllers/lottery_controller_spec.rb
Click here to see our test result and coverage of the LotteryController
class.
UI Testing (For Reviewers)
By following the below steps, you can manually test our refactored LotteryController
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 construct_users_bidding_info
). We intended not to disrupt this behavior since it's beyond the scope of this refactoring.
- Visit http://152.46.17.241:8080. 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
run_intelligent_assignment
method of theLotteryController
class (if you can't see the orange book icon, please consult the "Make an assignment intelligent" subsection placed below).
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:".
Make an assignment intelligent:
- Click on the pencil icon, which leads you to the edit page.
- Under the General tab, make sure these three checkboxes are selected:
- Has teams?
- Has topics?
- Available to students?
- Under the Topics tab, make "Enable bidding for topics?" to true.
- Then return to the assignment page, you should see the orange book icon next to the assignment you just edited.
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.
Click here to see our demonstration on how to test the run_intelligent_assignment
method.
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. Afterward, there is an additional iteration to exclude topics without bids.