CSC/ECE 517 Spring 2018- Project E1808: Refactor review mapping controller.rb

Introduction

Expertiza

Expertiza is a platform through which students are able to view and manage their assignments, form teams for a group project and review other team's work for improvement. It uses Ruby on Rails Framework

Objectives of the Project

• Refactor automatic_review_mapping method
  • Write failing tests first
  • Split into several simpler methods and assign reasonable names
  • Extract duplicated code into separate methods
• Refactor response_report method
  • Write failing tests first
  • Use factory pattern to replace the long switch statements
  • Move switch conditions to corresponding subclasses with same method name response_report
  • Create corresponding helper classes for different report types if they do not exist
  • Replace the conditional with the relevant method calls

About Review Mapping Controller

Review mapping controller handles the review responses that are done on every assignment. It assigns reviews to different teams or individual student depending on the type of assignment(team project or individual project). It keeps track that all the teams are reviewed and assigned teams to review too.

Refactoring

Refactoring is used to improve a design code by changing the structure of the code such that the functionality remains the same. The changes made are quite small, but the overall effect becomes significant.

Refactoring automatic_review_mapping method

As this method was long and complex it needed to be broken down into smaller methods with specific functionality. Original:

def automatic_review_mapping
    assignment_id = params[:id].to_i
    participants = AssignmentParticipant.where(parent_id: params[:id].to_i).to_a.reject {|p| p.can_review == false }.shuffle!
    teams = AssignmentTeam.where(parent_id: params[:id].to_i).to_a.shuffle!
    max_team_size = Integer(params[:max_team_size]) # Assignment.find(assignment_id).max_team_size
    # Create teams if its an individual assignment.
    if teams.empty? and max_team_size == 1
      participants.each do |participant|
        user = participant.user
        next if TeamsUser.team_id(assignment_id, user.id)
        team = AssignmentTeam.create_team_and_node(assignment_id)
        ApplicationController.helpers.create_team_users(participant.user, team.id)
        teams << team
      end
    end
    student_review_num = params[:num_reviews_per_student].to_i
    submission_review_num = params[:num_reviews_per_submission].to_i
    calibrated_artifacts_num = params[:num_calibrated_artifacts].to_i
    uncalibrated_artifacts_num = params[:num_uncalibrated_artifacts].to_i

    if calibrated_artifacts_num == 0 and uncalibrated_artifacts_num == 0
      if student_review_num == 0 and submission_review_num == 0
        flash[:error] = "Please choose either the number of reviews per student or the number of reviewers per team (student)."
      elsif (student_review_num != 0 and submission_review_num == 0) or (student_review_num == 0 and submission_review_num != 0)
        # REVIEW: mapping strategy
        automatic_review_mapping_strategy(assignment_id, participants, teams, student_review_num, submission_review_num)
      else
        flash[:error] = "Please choose either the number of reviews per student or the number of reviewers per team (student), not both."
      end
    else
      teams_with_calibrated_artifacts = []
      teams_with_uncalibrated_artifacts = []
      ReviewResponseMap.where(reviewed_object_id: assignment_id, calibrate_to: 1).each do |response_map|
        teams_with_calibrated_artifacts << AssignmentTeam.find(response_map.reviewee_id)
      end
      teams_with_uncalibrated_artifacts = teams - teams_with_calibrated_artifacts
      # REVIEW: mapping strategy
      automatic_review_mapping_strategy(assignment_id, participants, teams_with_calibrated_artifacts.shuffle!, calibrated_artifacts_num, 0)
      # REVIEW: mapping strategy
      # since after first mapping, participants (delete_at) will be nil
      participants = AssignmentParticipant.where(parent_id: params[:id].to_i).to_a.reject {|p| p.can_review == false }.shuffle!
      automatic_review_mapping_strategy(assignment_id, participants, teams_with_uncalibrated_artifacts.shuffle!, uncalibrated_artifacts_num, 0)
    end
    redirect_to action: 'list_mappings', id: assignment_id
  end

After Refactoring:

def automatic_review_mapping
    count = 0 # this variable is used to control whether the calibrated or the uncalibrated automatic_review_map_controller_strategy will be called on yield.
    helper = AutomaticReviewMappingHelper::AutomaticReviewMappingHelper.new(params)
    # Create teams if its an individual assignment.
    helper.create_teams_if_individual_assignment
    if helper.check_if_all_artifacts_num_are_zero(flash,params)
       helper.assign_reviews_for_artifacts_num_zero(flash,params) {automatic_review_mapping_strategy(helper.assignment_id, helper.participants, helper.teams, helper.student_review_num, helper.submission_review_num)}
    else
       helper.assign_reviews_for_artifacts_num_not_zero(params) do
        automatic_review_mapping_strategy(helper.assignment_id, helper.participants, helper.teams_with_calibrated_artifacts.shuffle!, helper.calibrated_artifacts_num, 0) if count == 0
        count = count+1
        if count == 2 
          automatic_review_mapping_strategy(helper.assignment_id, helper.participants, helper.teams_with_uncalibrated_artifacts.shuffle!, helper.uncalibrated_artifacts_num, 0)
          count == 0
        end
      end
    end

Approach

As seen, the method originally was very long and complex. So, the method was broken down into smaller methods and these methods were shifted to a helper class. So the methods were separated so that each method is associated with a particular task and all these methods were included in the helper class: automatic_review_mapping_helper class. The parameters used in the original method were now called using the object of the helper class.

Helper Class

module AutomaticReviewMappingHelper
	class AutomaticReviewMappingHelper
		attr_accessor :assignment_id, :participants, :teams, :max_team_size, :student_review_num, :submission_review_num, :calibrated_artifacts_num, 
                              :uncalibrated_artifacts_num, :teams_with_calibrated_artifacts, :teams_with_uncalibrated_artifacts
		def initialize(params)
		     @assignment_id = params[:id].to_i
    		     @participants = AssignmentParticipant.where(parent_id: params[:id].to_i).to_a.reject {|p| p.can_review == false }.shuffle!
    		     @teams = AssignmentTeam.where(parent_id: params[:id].to_i).to_a.shuffle!
    		     @max_team_size = Integer(params[:max_team_size]) # Assignment.find(assignment_id).max_team_size
    		     @student_review_num = params[:num_reviews_per_student].to_i
    		     @submission_review_num = params[:num_reviews_per_submission].to_i
    		     @calibrated_artifacts_num = params[:num_calibrated_artifacts].to_i
    		     @uncalibrated_artifacts_num = params[:num_uncalibrated_artifacts].to_i
    		     @teams_with_calibrated_artifacts = []
      		     @teams_with_uncalibrated_artifacts = []
    	        end

    	def create_teams_if_individual_assignment
    		if @teams.empty? and @max_team_size == 1
     			 @participants.each do |participant|
        		 next if TeamsUser.team_id(@assignment_id, participant.user.id)
        		 team = AssignmentTeam.create_team_and_node(@assignment_id)
        		 ApplicationController.helpers.create_team_users(participant.user, team.id)
        		 @teams << team
                 end
    		end
    	end

    	def check_if_all_artifacts_num_are_zero(flash,params)
    		
    		if @calibrated_artifacts_num == 0 and @uncalibrated_artifacts_num == 0
    			true
    			#check_review_num_before_assigning_review(flash,obj,params)
    		else
    			false
    			#artifacts_num_not_zero(obj,params)
    		end

    	end

    	def assign_reviews_for_artifacts_num_zero(flash,params)
    		
    		if @student_review_num == 0 and @submission_review_num == 0
        		flash[:error] = "Please choose either the number of reviews per student or the number of reviewers per team (student)."
      		elsif (@student_review_num != 0 and @submission_review_num == 0) or (@tudent_review_num == 0 and @submission_review_num != 0)
        		# REVIEW: mapping strategy
             	        yield
      		else
        		flash[:error] = "Please choose either the number of reviews per student or the number of reviewers per team (student), not both."
            end
    	end

    	def assign_reviews_for_artifacts_num_not_zero(params)
    		
           @teams_with_calibrated_artifacts = []
      	   @teams_with_uncalibrated_artifacts = []

      	   ReviewResponseMap.where(reviewed_object_id: @assignment_id, calibrate_to: 1).each do |response_map|
           @teams_with_calibrated_artifacts << AssignmentTeam.find(response_map.reviewee_id)
      	   end
      	   @teams_with_uncalibrated_artifacts = @teams - @teams_with_calibrated_artifacts
      	   # REVIEW: mapping strategy
      	   yield
      		
      	   # REVIEW: mapping strategy
           # since after first mapping, participants (delete_at) will be nil
      	   @participants = AssignmentParticipant.where(parent_id: params[:id].to_i).to_a.reject {|p| p.can_review == false }.shuffle!
      	   yield
         end
     end
 end