CSC/ECE 517 Spring 2016/Refactor response controller

E1614. Refactoring Response Controller

This page provide the details of the changes done as part of the refactoring response controller project(E1614).

A brief overview of Expertiza

Expertiza project is a platform to create reusable learning objects through peer review. It is an open source project which uses Ruby on Rails framework.

Project Statement

The main aim of this project was to refactor the response_controller. The following tasks were completed as part of refactoring in this project:

• Moving variable declaration to right places.
• Removing unused variables.
• Fixing code duplication.
• Replacing if else block with switch statements.
• Remove the unreachable code.

About Response Controller

The Response controller is responsible for the CRUD(Create, Read, Update and Delete) operations on responses. The users can fill out a questionnaire such as review rubric or feedback on the partner's contribution. So the ResponseController handles the various operations on the responses, which are objects that Expertiza creates when you fill out a questionnaire.

Changes done as part of refactoring

Moving variable declaration to right places

There were two variables "msg" and "error_msg" in the create method which were declared and initialized to blank strings and then again assigned some string value before it was used in the method. So we moved the variable declaration to the place were it was used and removed the unnecessary variable declaration. Below if the code snippet for the changes done:

 def create
    @map = ResponseMap.find(params[:id]) #assignment/review/metareview id is in params id

    set_all_responses

    #to save the response for ReviewResponseMap, a questionnaire_id is wrapped in the params
    if params[:review][:questionnaire_id]
      @questionnaire = Questionnaire.find(params[:review][:questionnaire_id])
      @round = params[:review][:round]
    else
      @round=nil
    end

    # create the response
    if params[:isSubmit].eql?('Yes')
      is_submitted = true
    else
      is_submitted = false
    end
    @response = Response.create(:map_id => @map.id, :additional_comment => params[:review][:comments],:round => @round, :is_submitted => is_submitted)#,:version_num=>@version)

    #Change the order for displaying questions for editing response views.
    questions=sort_questions(@questionnaire.questions)

    if params[:responses]
       create_answers(params, questions)
    end

    #@map.save
    msg = "Your response was successfully saved."
    error_msg="Error"
    @response.email();
    redirect_to :controller => 'response', :action => 'saving', :id => @map.map_id, :return => params[:return], :msg => msg, :error_msg => error_msg, :save_options => params[:save_options]
  end

Removing unused variables

There were some unused variables in this controller. The edit method contained a variable "array_not_empty" variable which was not used anywhere. So we removed the unused variables.

Fixing code duplication

There was some duplicate code in the update method. So we removed the duplicate code and retained the code snippet inside the if block. Below is the modified code for the update method:

  #Update the response and answers when student "edit" existing response
  def update
    return unless action_allowed?

    # the response to be updated
    @response = Response.find(params[:id])

    msg = ""
    begin
      @map = @response.map
      @response.update_attribute('additional_comment', params[:review][:comments])
      if @map.type=="ReviewResponseMap" && @response.round
        @questionnaire = @map.questionnaire(@response.round)
      elsif @map.type=="ReviewResponseMap"
        @questionnaire = @map.questionnaire(nil)
      else
        @questionnaire = @map.questionnaire
      end
      questions = @questionnaire.questions.sort { |a,b| a.seq <=> b.seq }


       questions=sort_questions(@questionnaire.questions)
       create_answers(params,questions)
      questions = @questionnaire.questions.sort { |a,b| a.seq <=> b.seq }

      if !params[:responses].nil? # for some rubrics, there might be no questions but only file submission (Dr. Ayala's rubric)
        params[:responses].each_pair do |k, v|
          score = Answer.where(response_id: @response.id, question_id:  questions[k.to_i].id).first
          unless score
            score = Answer.create(:response_id => @response.id, :question_id => questions[k.to_i].id, :answer => v[:score], :comments => v[:comment])
          end
          score.update_attribute('answer', v[:score])
          score.update_attribute('comments', v[:comment])
        end
      end
      if (params['isSubmit'] && (params['isSubmit'].eql?'Yes'))
        # Update the submission flag.
        @response.update_attribute('is_submitted',true)
      else
        @response.update_attribute('is_submitted',false)
      end
    rescue
      msg = "Your response was not saved. Cause:189 #{$!}"
    end
    redirect_to :controller => 'response', :action => 'saving', :id => @map.map_id, :return => params[:return], :msg => msg, :save_options => params[:save_options]
  end

Replacing if else block with switch statements

The redirection method used the if-else statements. The if-else block was replaced with switch statements. Below if is the code snippet for the redirection method after the code changes:

 def redirection
    flash[:error] = params[:error_msg] unless params[:error_msg] and params[:error_msg].empty?
    flash[:note] = params[:msg] unless params[:msg] and params[:msg].empty?
    @map = Response.find_by_map_id(params[:id])

    case params[:return]
      when "feedback"
        redirect_to :controller => 'grades', :action => 'view_my_scores', :id => @map.reviewer.id
      when "teammate"
        redirect_to view_student_teams_path student_id: @map.reviewer.id
      when "instructor"
        redirect_to :controller => 'grades', :action => 'view', :id => @map.response_map.assignment.id
      when "assignment_edit"
        redirect_to controller: 'assignments', action: 'edit', id: @map.response_map.assignment.id
      else
        redirect_to :controller => 'student_review', :action => 'list', :id => @map.reviewer.id
    end
  end

Remove the unreachable code

In the action_allowed? method of the controller there was a case for the "edit" case which was getting executed all the time making the second case block unreachable. Modified the code to use the proper case statements for "edit", "delete" and "update". Below is the code snippet for the action_allowed? method after the code changes:

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

        # if it is a review response map, all the members of revieweee 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)
          current_user_id?(response.map.reviewer.user_id) || reviewee_team.has_user(current_user) || (['Administrator','Instructor','Teaching Assistant'].include? current_user.role.name)
        else
          current_user_id?(response.map.reviewer.user_id)
        end
      else
        current_user
    end
  end

References

1. Forked Repository for Expertiza Project
2. Expertiza project documentation wiki
3. Expertiza on GitHub