CSC/ECE 517 Fall 2017/E1745 Refactor response controller.rb

Introduction

Expertiza is an open source webapp built on Ruby on Rails stack. It provides a platform to students with various features like peer-reviewing projects, submitting work, form teams, viewing grades etc. The project is being built and maintained by students and faculty at NCSU.

Tasks operated by Team

The tasks completed by the team consisted of refactoring response_controller.rb which is mainly responsible for handling the response for peer reviews/questionnaires/survey with tasks such as creating, saving, editing, updating, deletion. The functionality is further ensured stable by the team with 23 integration tests being written for the same.

Team Members

1) Pavneet Singh Anand

2) Dipansha Gupta

3) Kshittiz Kumar

Refactoring Explained

Without wasting much time, lets jump into the refactoring by describing code which existed previously followed by the changed code.

1) action_allowed? method

Previous Code :-

  def action_allowed?
    case params[:action]
    when 'edit' # If response has been submitted, no further editing allowed
      response = Response.find(params[:id])
      return false if response.is_submitted
      return current_user_id?(response.map.reviewer.user_id)
      # Deny access to anyone except reviewer & author's team
    when 'delete', 'update'
      response = Response.find(params[:id])
      return current_user_id?(response.map.reviewer.user_id)
    when 'view'
      response = Response.find(params[:id])
      map = response.map
      assignment = response.map.reviewer.assignment
      # if it is a review response map, all the members of reviewee team should be able to view the reponse (can be done from heat map)
      if map.is_a? ReviewResponseMap
        reviewee_team = AssignmentTeam.find(map.reviewee_id)
        return current_user_id?(response.map.reviewer.user_id) || reviewee_team.has_user(current_user) || current_user.role.name == 'Administrator' || (current_user.role.name == 'Instructor' and assignment.instructor_id == current_user.id) || (current_user.role.name == 'Teaching Assistant' and TaMapping.exists?(ta_id: current_user.id, course_id: assignment.course.id))
      else
        return current_user_id?(response.map.reviewer.user_id)
      end
    else
      current_user
    end
  end

The view case is extracted into a separate method and some common variables have been extracted out of the cases

def action_allowed?
    response = user_id = nil
    action = params[:action]
    if %w(edit delete update view).include?(action)
      response = Response.find(params[:id])
      user_id = response.map.reviewer.user_id if (response.map.reviewer)
    end
    case action
    when 'edit' # If response has been submitted, no further editing allowed
      return false if response.is_submitted
      return current_user_id?(user_id)
      # Deny access to anyone except reviewer & author's team
    when 'delete', 'update'
      return current_user_id?(user_id)
    when 'view'
      return edit_allowed?(response.map, user_id)
    else
      current_user
    end
  end

  def edit_allowed?(map, user_id)
    assignment = map.reviewer.assignment
    # if it is a review response map, all the members of reviewee team should be able to view the reponse (can be done from heat map)
    if map.is_a? ReviewResponseMap
      reviewee_team = AssignmentTeam.find(map.reviewee_id)
      return current_user_id?(user_id) || reviewee_team.has_user(current_user) || current_user.role.name == 'Administrator' || (current_user.role.name == 'Instructor' and assignment.instructor_id == current_user.id) || (current_user.role.name == 'Teaching Assistant' and TaMapping.exists?(ta_id: current_user.id, course_id: assignment.course.id))
    else
      return current_user_id?(user_id)
    end
  end

This has extracted an independent functionality and enhanced readability apart from sticking to guidelines of short methods.

2) Replaced find_by_map_id with find_by

@map = Response.find_by_map_id(params[:id])

Moving on with latest Rails conventions, function is being modified as

@map = Response.find_by(map_id: params[:id])

This will avoid any unexpected behaviour when further upgrading the Rails framework.

3) Replacing sorting technique

questions.sort {|a, b| a.seq <=> b.seq }

has been replaced with

questions.sort_by(&:seq)

This is the way to sort based on an object attribute.

4) Refactoring pending_surveys method

  def pending_surveys
    unless session[:user] # Check for a valid user
      redirect_to '/'
      return
    end

    # Get all the participant(course or assignment) entries for this user
    course_participants = CourseParticipant.where(user_id: session[:user].id)
    assignment_participants = AssignmentParticipant.where(user_id: session[:user].id)

    # Get all the course survey deployments for this user
    @surveys = []
    if course_participants
      course_participants.each do |cp|
        survey_deployments = CourseSurveyDeployment.where(parent_id: cp.parent_id)
        next unless survey_deployments
        survey_deployments.each do |survey_deployment|
          next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
          @surveys <<
          [
            'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
            'survey_deployment_id' => survey_deployment.id,
            'start_date' => survey_deployment.start_date,
            'end_date' => survey_deployment.end_date,
            'parent_id' => cp.parent_id,
            'participant_id' => cp.id,
            'global_survey_id' => survey_deployment.global_survey_id
          ]
        end
      end
    end

    # Get all the assignment survey deployments for this user
    if assignment_participants
      assignment_participants.each do |ap|
        survey_deployments = AssignmentSurveyDeployment.where(parent_id: ap.parent_id)
        next unless survey_deployments
        survey_deployments.each do |survey_deployment|
          next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
          @surveys <<
          [
            'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
            'survey_deployment_id' => survey_deployment.id,
            'start_date' => survey_deployment.start_date,
            'end_date' => survey_deployment.end_date,
            'parent_id' => ap.parent_id,
            'participant_id' => ap.id,
            'global_survey_id' => survey_deployment.global_survey_id
          ]
        end
      end
    end
  end

This method involved a lot of code which violates guideline of short methods. Moreover, the lines of code can be reduced along with readability enhanced as similar functionalities in multiple lines can be combined.

53 lines of code have been reduced to 32. The refactored method is given below:-

  def pending_surveys
    unless session[:user] # Check for a valid user
      redirect_to '/'
      return
    end

    # Get all the course survey deployments for this user
    @surveys = []
    [CourseParticipant, AssignmentParticipant].each do |item|
      # Get all the participant(course or assignment) entries for this user
      participant_type = item.where(user_id: session[:user].id)
      next unless participant_type
      participant_type.each do |p|
        survey_deployment_type = participant_type == CourseParticipant ? AssignmentSurveyDeployment : CourseSurveyDeployment
        survey_deployments = survey_deployment_type.where(parent_id: p.parent_id)
        next unless survey_deployments
        survey_deployments.each do |survey_deployment|
          next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
          @surveys <<
              [
                'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
                'survey_deployment_id' => survey_deployment.id,
                'start_date' => survey_deployment.start_date,
                'end_date' => survey_deployment.end_date,
                'parent_id' => p.parent_id,
                'participant_id' => p.id,
                'global_survey_id' => survey_deployment.global_survey_id
              ]
        end
      end
    end
  end