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

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.

For the action run_intelligent_assignmnent, we have two test cases:
1. 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.

2. should return json response
This test case checks if the response from the web service is in JSON format or not.

For the action run_intelligent_bid, we have the following test cases:
1.should do intelligent assignment
This was an already existing test case. It tests whether the assignment received is intelligent or not.
2. should exit gracefully when assignment not intelligent
This checks if the control redirects to main page in case the assignment received is not intelligent.

For the action create_new_teams_for_bidding_response, we have the following test cases:
1. 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.

For the action auto_merge_teams, we have written the following test cases:
1. 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.