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

Caklier: /* Existing Tests */

2020-03-31T19:48:09Z

Existing Tests

← Older revision		Revision as of 19:48, 31 March 2020
Line 264:		Line 264:

	=== Existing Tests ===		=== 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:		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<br>This was an attempt at testing the web service used by the lottery controller		# 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

Caklier: /* Test Plan */

2020-03-31T19:37:10Z

Test Plan

← Older revision		Revision as of 19:37, 31 March 2020
Line 209:		Line 209:
	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 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.		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.		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.

Caklier: /* Problem 5: Unnecessary Looping */

2020-03-31T19:31:01Z

Problem 5: Unnecessary Looping

← Older revision		Revision as of 19:31, 31 March 2020
Line 192:		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

Caklier: /* Problem 4: Multi-Purposed Methods */

2020-03-31T19:30:17Z

Problem 4: Multi-Purposed Methods

← Older revision		Revision as of 19:30, 31 March 2020
Line 165:		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

Caklier: /* Problem 3: Ambiguous Identifiers */

2020-03-31T19:28:56Z

Problem 3: Ambiguous Identifiers

← Older revision		Revision as of 19:28, 31 March 2020
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>.

Caklier: /* Example 3 */

2020-03-31T19:25:22Z

Example 3

← Older revision		Revision as of 19:25, 31 March 2020
Line 107:		Line 107:
	===== Example 3 =====		===== 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.		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

Caklier: /* Problem 2: Verbose Code */

2020-03-31T19:20:29Z

Problem 2: Verbose Code

← Older revision		Revision as of 19:20, 31 March 2020
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 ~~keep~~ them ~~simple with~~ alternative language syntax ~~and other ways of thinking of~~ the behavior.		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 ====

Caklier: /* run_intelligent_assignment */

2020-03-31T19:17:25Z

run_intelligent_assignment

← Older revision		Revision as of 19:17, 31 March 2020
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 ~~hash~~ for students on a team who haven't signed up for a topic yet		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]]

Caklier: /* Implementation */

2020-03-31T19:12:19Z

Implementation

← Older revision		Revision as of 19:12, 31 March 2020
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 ~~addition~~ comments and fixed some typos		# 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.

Caklier: /* Tasks Identified */

2020-03-31T19:10:43Z

Tasks Identified

← Older revision		Revision as of 19:10, 31 March 2020
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