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.