CSC/ECE 517 Fall 2014/OSS E1466 gjf

E1466: Refactoring GradesController

This wiki deals with our implementation of a controller in expertiza: grade_controller for the Expertiza Project using Ruby on Rails.

Introduction

Expertiza is a web application where students can submit and peer-review learning objects (articles, code, web sites, etc). It is used in select courses at NC State and by professors at several other colleges and universities.

Project Description

Classes involved: grades_controller.rb

What it does:

This class lists the grades of all the participants for an assignments and also their reviews. Instructor can edit scores, calculate penalties and send emails for conflicts.

What needs to be done:

• Modify calculate_all_penalties method which is too complex and long.
• Put the get_body_text method in a mailer rather than in the grades_controller.
• Refactor conflict_nofication method to conflict_email and make it delegated to the mailer.
• Refactor view_my_scores method to grades_show and delete the unnecessary variables.
• Try not to query for reviews and meta reviews in grades_show method.

Code Modification

calculate_all_penalties Method

Summary

We have modified the calculate_penalties method and implemented all the functions it originally has. After our modification the method shrinks from 45 lines to 32 lines of Ruby code. Originally, the method includes a lot of unnecessary if and unless statements which makes the method uneasy to read and seems to have a difficult logic to calculate the penalties.

According to the codeclimate which calculate the complexity of the method, originally, it has a complexity of 85, however, after we modified it, the complexity goes down to 65. Please check at original version,modified version

We have done the following to the original methods:

• Used for loop with selective choice to create penalty attributes and put it into the CalculatedPenalty Table, instead of duplicating the almost same code.

• Used Object-Oriented Design patterns of Ruby: let the array respond to the method min which returns the minimum number in the array and get the logical penalty other than using if statement.(max_penalty if total_penalty is larger than max_penalty, total_penalty if total_penalty is smaller than max_penalty)

• Eliminated the unnecessary if statements to make the code clear to read.

Implementation

• Modify if statements:

Before Refactoring:

 if(penalties[:submission].nil?)
      penalties[:submission]=0
 end
 if(penalties[:review].nil?)
      penalties[:review]=0
 end
 if(penalties[:meta_review].nil?)
      penalties[:meta_review]=0
 end

After Refactoring:

penalties[:submission] = 0 if penalties[:submission].nil?
penalties[:review] = 0 if penalties[:review].nil?
penalties[:meta_review] = 0 if penalties[:meta_review].nil?

• Using Objected-oriented Pattern

Before Refactoring:

 if(@total_penalty > l_policy.max_penalty)
      @total_penalty = l_policy.max_penalty
 end

After Refactoring:

 @total_penality=[l_policy.max_penalty,@total_penality].min

• Using loops rather than duplicate same code

Before Refactoring:

 penalty_attr1 = {:deadline_type_id => 1,:participant_id => @participant.id, :penalty_points => penalties[:submission]}
       CalculatedPenalty.create(penalty_attr1)
 penalty_attr2 = {:deadline_type_id => 2,:participant_id => @participant.id, :penalty_points => penalties[:review]}
       CalculatedPenalty.create(penalty_attr2)
 penalty_attr3 = {:deadline_type_id => 5,:participant_id => @participant.id, :penalty_points => penalties[:meta_review]}
       CalculatedPenalty.create(penalty_attr3)"
 @all_penalties[participant.id] = Hash.new
       @all_penalties[participant.id][:submission] = penalties[:submission]
       @all_penalties[participant.id][:review] = penalties[:review]
       @all_penalties[participant.id][:meta_review] = penalties[:meta_review]
       @all_penalties[participant.id][:total_penalty] = @total_penalty"

After Refactoring:

 deadline_type=[1,2,5]
 penalty_type=[:submission,:review,:meta_review]
 if calculate_for_participants
      for i in 0..2
           penalty_attr={:deadline_type_id=>deadline_type[i],:participant_id => @participant.id, :penalty_points => penalties[penalty_type[i]]}
           CalculatedPenalty.create(penalty_attr)
      end
 end
 @all_penalties[participant.id] = {}
      for i in 0..2
          @all_penalties[participant.id][:penalty_type[i]] = penalties[:penalty_type[i]]
      end
 @all_penalties[participant.id][:total_penalty] = @total_penalty"

conflict_email Method

Implementation

• specific reasons/ Design Pattern

Before Refactoring:

 code here

After Refactoring:

 code here

view_my_scores method

Summary

We refactored view_my_scores method to grades_show, deleted many unnecessary instance variables along with several review and meta reviews methods inside it. Therefore, the system will only search for scores in the database rather than search for scores and reviews, which wastes a lot of time during the grade show process. After refactoring, the complexity on this decreased from 53 to 35. We also optimize get_scores method to improve the efficiency of showing scores by students and instructors, which cost us most of the time during the project. After refactoring this method, the time cost of views in grade controller decreased by 80%.

We have done the following to the original methods:

• Refactored view_my_scores method to grades_show method

• Searched for the key reasons which lead to huge waiting time for getting score

• Refactored get_assessments_for method in response_map.rb and lead to 80% off the current waiting time.

• Eliminated the search for reviews during the grades_show method

• Used Ajax to show and search for reviews for a specific team

• Deleted unnecessary instance Variables

Implementation

• Deleted the unnecessary instance variables

Deleted Code:

 @average_score_results = Array.new
 @average_score_results = ScoreCache.get_class_scores(@participant.id)
 @statistics = Array.new
 @average_score_results.each { |x|
 @statistics << x
 }

• Deleted query for reviews and meta_reviews

Deleted Code:

 @average_reviews = ScoreCache.get_reviews_average(@participant.id)
 @average_metareviews = ScoreCache.get_metareviews_average(@participant.id)
 @my_reviews = ScoreCache.my_reviews(@participant.id)
 @my_metareviews = ScoreCache.my_metareviews(@participant.id)

• Modify get_assessments_for method in response_map.rb

After doing this, the time cost of view function decreased by 80% Before Refactoring:

 # the original method to find all response 
 @all_resp=Response.all
 for element in @all_resp
     if (element.map_id == map.map_id)
         @array_sort << element
         @test << map
     end
 end

After Refactoring:

 @all_resp=Response.find_by_map_id(map.map_id)
 @array_sort << @all_resp
 @test << map

• Adopt Ajax into grades_controller.rb

Add Method:

 #show_reviews method gets reviews, meta reviews and author feedback information from server.
  def show_reviews
    @partial='grades/'+params[:path]
    @prefix=params[:prefix]
    @score=Hash.new
    @score[:assessments]=Array.new
    params[:score][:assessments].each do |assessment|
        @score[:assessments]<<Response.find(assessment)
    end
    @score[:scores]=params[:score][:scores]
    @participant=AssignmentParticipant.find(params[:participant])
    @assignment = @participant.assignment
    @questions = {}
    questionnaires = @assignment.questionnaires
    questionnaires.each do |questionnaire|
      @questions[questionnaire.symbol] = questionnaire.questions
    end
  end

• Add Ajax in views(show_reviews.html.haml)

 %h1 Grades#show_reviews 
 %p Find me in app/views/grades/show_reviews.html.haml
 <%= render :partial=>'grades/reviews', :locals => {:prefix => 'user', :participant => @participant, :rscore => @score} )%>

Result

Name	Before Refactoring	After Refactoring	Reduced By
View all team's score(instructor)	484988ms	8642ms	98.21%
View own score(student)	8941ms	651ms	92.71%