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 unassignmed 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
Splited the function into smaller pieces to keep up with coding conventions.