CSC/ECE 517 Fall 2016/E1658. Refractor lottery controller.rb and write integration tests
Expertiza Background
Expertiza is an educational web application created and maintained by the joint efforts of the students and the faculty at NCSU. It’s an open source project developed on Ruby on Rails platform and it’s code is available on Github. It allows students to review each other’s work and improve their work upon this feedback.
Description of Current Project
The following is an Expertiza based OSS project which deals primarily with the lottery_controller.rb file. It focusses on refactoring some of the more complex methods, modifying some of the language to make it more Ruby friendly, removing some redundant code. The goal of this project is to attempt to make this part of the application easier to read and maintain. The lottery_controller contains all of the logic for running the lottery. Code climate is used in this project to detect various issues such as high cyclomatic complexity, bad naming, etc.
Changes needed
Fix following issues mentioned below:
- Cyclomatic complexity for run_intelligent_bid is too high.
- Perceived complexity for run_intelligent_bid is too high.
- Use snake_case for variable names.
- Avoid more than 3 levels of block nesting.
Changes performed
The code was refactored and its Code Climate rating improved from F to A.
Before Refractoring:
After Refractoring:
- Cyclomatic complexity for run_intelligent_bid and other methods is greatly reduced.
- Perceived complexity for run_intelligent_bid is also reduced.
- snake_case are used for variable names.
- More than 3 levels of block nesting is avoided.
Before:
if bid_record.nil?
bids << 0
else
bids << bid_record.priority ||= 0
end
After:
bids << bid_record.nil? ? 0 : bid_record.priority ||= 0
Assignment using if condition was changed to conditional operator
Before:
assignment = Assignment.find_by_id(params[:id])
After:
assignment = Assignment.find_by(id: params[:id])
Changed to find_by from find_by_id
Before:
response = RestClient.post url, data.to_json, :content_type => :json, :accept => :json
After:
response = RestClient.post url, data.to_json, content_type: :json, accept: :json
Changed to the latest ruby convention for HashMaps
Before:
def create_new_teams_for_bidding_response(teams, assignment)
teams.each_with_index do |user_ids, index|
new_team = AssignmentTeam.create(name: assignment.name + '_Team' + rand(1000).to_s,
parent_id: assignment.id,
type: 'AssignmentTeam')
parent = TeamNode.create(parent_id: assignment.id, node_object_id: new_team.id)
user_ids.each do |user_id|
team_user = TeamsUser.where(user_id: user_id, team_id: new_team.id).first rescue nil
team_user = TeamsUser.create(user_id: user_id, team_id: new_team.id) if team_user.nil?
TeamUserNode.create(parent_id: parent.id, node_object_id: team_user.id)
end
end
end
After:
def create_new_teams_for_bidding_response(teams, assignment)
teams.each_with_index do |user_ids|
new_team = AssignmentTeam.create(name: assignment.name + '_Team' + rand(1000).to_s,
parent_id: assignment.id,
type: 'AssignmentTeam')
parent = TeamNode.create(parent_id: assignment.id, node_object_id: new_team.id)
user_ids.each do |user_id|
team_user = TeamsUser.where(user_id: user_id, team_id: new_team.id).first rescue nil
team_user = TeamsUser.create(user_id: user_id, team_id: new_team.id) if team_user.nil?
TeamUserNode.create(parent_id: parent.id, node_object_id: team_user.id)
end
end
Removed unused variables
Before:
unless Assignment.find_by_id(params[:id]).is_intelligent # if the assignment is intelligent then redirect to the tree display list
flash[:error] = "This action not allowed. The assignment " + Assignment.find_by_id(params[:id]).name + " does not enabled intelligent assignments."
After:
unless Assignment.find_by(id: params[:id]).is_intelligent # if the assignment is intelligent then redirect to the tree display list
flash[:error] = "This action not allowed. The assignment " + Assignment.find_by(id: params[:id]).name + " does not enabled intelligent assignments."
Changed find_by_id to find_by
Before:
unassignedTeams = AssignmentTeam.where(parent_id: params[:id]).reject {|t| !SignedUpTeam.where(team_id: t.id).empty?}
unassignedTeams.sort! {|t1, t2| TeamsUser.where(team_id: t2.id).size <=> TeamsUser.where(team_id: t1.id).size}
team_bids = []
unassignedTeams.each do |team|
topic_bids = []
sign_up_topics.each do |topic|
student_bids = []
TeamsUser.where(team_id: team.id).each do |s|
student_bid = Bid.where(user_id: s.user_id, topic_id: topic.id).first rescue nil
if !student_bid.nil? and !student_bid.priority.nil?
student_bids << student_bid.priority
end
end
#takes the most frequent priority as the team priority
freq = student_bids.inject(Hash.new(0)) { |h,v| h[v] += 1; h}
topic_bids << {topic_id: topic.id, priority: student_bids.max_by { |v| freq[v] }} unless freq.empty?
end
topic_bids.sort! {|b| b[:priority]}
team_bids << {team_id: team.id, bids: topic_bids}
end
After:
unassigned_teams = AssignmentTeam.where(parent_id: params[:id]).reject {|t| !SignedUpTeam.where(team_id: t.id).empty? }
unassigned_teams.sort! {|t1, t2| TeamsUser.where(team_id: t2.id).size <=> TeamsUser.where(team_id: t1.id).size }
team_bids = []
unassigned_teams.each do |team|
topic_bids = []
sign_up_topics.each do |topic|
student_bids = []
TeamsUser.where(team_id: team.id).each do |s|
student_bid = Bid.where(user_id: s.user_id, topic_id: topic.id).first rescue nil
if !student_bid.nil?
if !student_bid.priority.nil?
student_bids << student_bid.priority
end
end
# takes the most frequent priority as the team priority
freq = student_bids.each_with_object(Hash.new(0)) do |v, h|
h[v] += 1
end
topic_bids << {topic_id: topic.id, priority: student_bids.max_by {|v| freq[v] }} unless freq.empty?
end
topic_bids.sort! {|b| b[:priority] }
team_bids << {team_id: team.id, bids: topic_bids}
end
Used snake case naming convention. Removed and condition inside if and converted it to nested if condition check. Replaced inject with each_with_obj.
Before:
def auto_merge_teams(unassignedTeams, _finalTeamTopics)
assignment = Assignment.find(params[:id])
# Sort unassigned
unassignedTeams = Team.where(id: unassignedTeams).sort_by {|t| !t.users.size }
unassignedTeams.each do |team|
sortedBids = Bid.where(user_id: team.id).sort_by(&:priority) # Get priority for each unassignmed team
sortedBids.each do |b|
# SignedUpTeam.where(:topic=>b.topic_id).first.team_id
winningTeam = SignedUpTeam.where(topic: b.topic_id).first.team_id
next unless TeamsUser.where(team_id: winningTeam).size + team.users.size <= assignment.max_team_size # If the team can be merged to a bigger team
TeamsUser.where(team_id: team.id).update_all(team_id: winningTeam)
Bid.delete_all(user_id: team.id)
Team.delete(team.id)
break
end
end
end
After:
before_action :fetch_assignment, only: [:auto_merge_teams]
before_action :sort_unassigned_teams, only: [:auto_merge_teams]
def auto_merge_teams(unassigned_teams, _final_team_topics)
# Sort unassigned
unassigned_teams.each do |team|
sorted_bids = Bid.where(user_id: team.id).sort_by(&:priority) # Get priority for each unassigned team
sorted_bids.each do |b|
# SignedUpTeam.where(:topic=>b.topic_id).first.team_id
winning_team = SignedUpTeam.where(topic: b.topic_id).first.team_id
next unless TeamsUser.where(team_id: winning_team).size + team.users.size <= assignment.max_team_size # If the team can be merged to a bigger team
TeamsUser.where(team_id: team.id).update_all(team_id: winning_team)
Bid.delete_all(user_id: team.id)
Team.delete(team.id)
break
end
end
end
def fetch_assignment
assignment = Assignment.find(params[:id])
end
def sort_unassigned_teams
unassigned_teams = Team.where(id: unassigned_teams).sort_by {|t| !t.users.size }
end
end
Split the function into smaller pieces to keep up with coding conventions.
Test Cases
Initially, the lottery controller had only one test case: for the action run_intelligent_bid. We added one more test case for run_intelligent_bid and also added test cases for all the other actions in the controller.
1. For the action run_intelligent_assignmnent, we have two test cases:
- webservice call should be successful
This is to check if the web service is getting called. But the URL given in the code is currently is of a web service that is currently down. Hence for testing purposes we have used a dummy URL. This can be replaced with the correct URL to the web service when it becomes available in the future.
- should return json response
This test case checks if the response from the web service is in JSON format or not.
2. For the action run_intelligent_bid, we have the following test cases:
- should do intelligent assignment
This was an already existing test case. It tests whether the assignment received is intelligent or not.
- should exit gracefully when assignment not intelligent
This checks if the control redirects to main page in case the assignment received is not intelligent.
3. For the action create_new_teams_for_bidding_response, we have the following test cases:
- should create team and return teamid
This test checks if the method is creating the team for bidding by taking in the assignment as parameter.
4. For the action auto_merge_teams, we have written the following test cases:
- sorts the unassigned teams
This test case checks whether the method is able to sort teams which have not been assigned to any assignments yet.
Conclusions
As a result of our work, the code quality of the controller lottery_controller.rb has improved a lot. Code Climate has given it the highest possible rating. The original code had a code climate rating of F. We worked on all the issues pointed out by code climate and and improved the rating to A.
Initially, there was only one test case and only one of the methods in the controller was covered. We have also added RSpec test cases for all the methods in the the controller and hence the test coverage has increased.
This was a very good learning experience. We learnt how to write test cases in RSpec and got a chance to contribute to Open Source Software. Another major take away from the project was the chance that we got to read and understand code written by other people. This is very important skill to have when you work in the industry and can be sometimes more difficult than writing code from scratch.