CSC/ECE 517 Fall 2015/oss E1566 ARB

Expertiza

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 the current project

The following is an Expertiza based OSS project which deals primarily with the GradesController and GradesHelper. 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.

Files modified in current project

A controller and a helper file were modified for this project namely: 1.GradesController 2.GradesHelper

GradesController

This is a controller that helps students and instructors view grades and reviews, update scores, check for grading conflicts and calculate penalties. A couple of long and complex methods were refactored from this controller along with removal of some non-functional code and a few language changes to make it Ruby style. Three methods in particular, namely conflict_notification ,calculate_all_penalties and edit were found to be too long and were in need of refactoring into smaller, easier to manage methods. Few more compact methods were created for this purpose.

calculate_all_penalties

This is used to calculate various penalty values for each assignment if penalty is applicable. This method was very complex, performing too many functions within a single method and had to be broken into 3 smaller methods each having a more well defined function. This was split into the following three methods: i. calculate_all_penalties ii. calculate_penatly_attributes iii. assign_all_penalties Changes were also made to make the code follow ruby style. Finally some redundant code was commented out as it was non-functional.

Refactoring into smaller more specific methods:

Before :

   def calculate_all_penalties(assignment_id)
   ………………………………………………………………………………………………………………………………………………………………
   ………………………………………………………………………………………………………………………………………………………………
   ………………………………………………………………………………………………………………………………………………………………                   
       if calculate_for_participants == true
         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)
       end
     end
     @all_penalties[participant.id] = {}
     @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
   end
   unless @assignment.is_penalty_calculated
     @assignment.update_attribute(:is_penalty_calculated, true)
   end
 end

After:

   def calculate_all_penalties(assignment_id)
   ………………………………………………………………………………………………………………………………………………………………
   ………………………………………………………………………………………………………………………………………………………………
   ………………………………………………………………………………………………………………………………………………………………                     
   if calculate_for_participants	
     calculate_penatly_attributes(@participant) #Refactored#Removed
       end
     end
     assign_all_penalties(@participant)
   end
   unless @assignment.is_penalty_calculated      #Refactored#Removed
     @assignment.update_attribute(:is_penalty_calculated, true)
   end
 end

     def calculate_penatly_attributes(participant)
         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)
     end   

    def assign_all_penalties(participant)
     @all_penalties[participant.id] = {}
     @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
   end

Removal of non-functional code :

Before:

     if(penalties[:submission] != 0 || penalties[:review] != 0 || penalties[:meta_review] != 0)
       unless penalties[:submission]
         penalties[:submission] = 0
       end
       unless penalties[:review]
         penalties[:review] = 0
       end
       unless penalties[:meta_review]
         penalties[:meta_review] = 0
       end
       @total_penalty = (penalties[:submission] + penalties[:review] + penalties[:meta_review])

After:

    if(penalties[:submission] != 0 || penalties[:review] != 0 || penalties[:meta_review] != 0)
        @total_penalty = (penalties[:submission] + penalties[:review] + penalties[:meta_review])

Change of language to make it more Ruby friendly:

Before:

  if(penalties[:submission] != 0 || penalties[:review] != 0 || penalties[:meta_review] != 0)

After:

   unless(penalties[:submission].zero? || penalties[:review].zero? || penalties[:meta_review].zero? )

conflict_notification

This method is used to help the instructors decide if one of the reviews are unfair or inaccurate. This was again split into 2 methods with some part of the code which is repeated in another method refactored into a new method.

Before:

  def conflict_notification
   if session[:user].role_id !=6
     @instructor = session[:user]
   else
     @instructor = Ta.get_my_instructor(session[:user].id)
   end
   @participant = AssignmentParticipant.find(params[:id])
   @assignment = Assignment.find(@participant.parent_id)
   @questions = Hash.new
   questionnaires = @assignment.questionnaires
   questionnaires.each {
     |questionnaire|
     @questions[questionnaire.symbol] = questionnaire.questions
   }
   @reviewers_email_hash = Hash.new
   @caction = "view"
   @submission = params[:submission]
   if @submission == "review"
     @caction = "view_review"
     @symbol = "review"
     process_response("Review", "Reviewer", @participant.reviews, "ReviewQuestionnaire")
   elsif @submission == "review_of_review"
     @symbol = "metareview"
     process_response("Metareview", "Metareviewer", @participant.metareviews, "MetareviewQuestionnaire")
   elsif @submission == "review_feedback"
     @symbol = "feedback"
     process_response("Feedback", "Author", @participant.feedback, "AuthorFeedbackQuestionnaire")
   elsif @submission == "teammate_review"
     @symbol = "teammate"
     process_response("Teammate Review", "Reviewer", @participant.teammate_reviews, "TeammateReviewQuestionnaire")
   end
   @subject = " Your "+@collabel.downcase+" score for " + @assignment.name + " conflicts with another "+@rowlabel.downcase+"'s score."
   @body = get_body_text(params[:submission])
 end

After:

   def conflict_notification
   if session[:user].role_id !=6
     @instructor = session[:user]
   else
     @instructor = Ta.get_my_instructor(session[:user].id)
   end
   @participant = AssignmentParticipant.find(params[:id])
   @assignment = Assignment.find(@participant.parent_id)
   ##################################################################################
   #	@questions = Hash.new
   #    questionnaires = assignment.questionnaires
   #    questionnaires.each {
   #        |questionnaire|
   #      @questions[questionnaire.symbol] = questionnaire.questions
   #    }
   ##################################################################################
   #Refactored--> Replaced with a methood call
   list_questions (@assignment)
   #################################################################################
   @reviewers_email_hash = Hash.new
   @caction = "view"
   @submission = params[:submission]
   if @submission == "review"
     @caction = "view_review"
     @symbol = "review"
     process_response("Review", "Reviewer", @participant.reviews, "ReviewQuestionnaire")
   elsif @submission == "review_of_review"
     @symbol = "metareview"
     process_response("Metareview", "Metareviewer", @participant.metareviews, "MetareviewQuestionnaire")
   elsif @submission == "review_feedback"
     @symbol = "feedback"
     process_response("Feedback", "Author", @participant.feedback, "AuthorFeedbackQuestionnaire")
   elsif @submission == "teammate_review"
     @symbol = "teammate"
     process_response("Teammate Review", "Reviewer", @participant.teammate_reviews, "TeammateReviewQuestionnaire")
   end
   @subject = " Your "+@collabel.downcase+" score for " + @assignment.name + " conflicts with another "+@rowlabel.downcase+"'s score."
   @body = get_body_text(params[:submission])
 end

#Refactored #Created a method which was a duplicate in conflict_notification and edit methods
 def list_questions (assignment)
   @questions = Hash.new
   questionnaires = assignment.questionnaires
   questionnaires.each {
       |questionnaire|
     @questions[questionnaire.symbol] = questionnaire.questions
   }
 end

edit

This method is used to edit the questionnaires. This method again has code which is repeated in the conflict_notification method and thus the repeated section was split into a new method.

Before:

   def edit
   @participant = AssignmentParticipant.find(params[:id])
   @assignment = @participant.assignment
   @questions = Hash.new
   questionnaires = @assignment.questionnaires
   questionnaires.each {
     |questionnaire|
     @questions[questionnaire.symbol] = questionnaire.questions
   }
   @scores = @participant.scores(@questions)
 end

After:

   def edit
   @participant = AssignmentParticipant.find(params[:id])
   @assignment = @participant.assignment
   ##################################################################################
   #	@questions = Hash.new
   #    questionnaires = assignment.questionnaires
   #    questionnaires.each {
   #        |questionnaire|
   #      @questions[questionnaire.symbol] = questionnaire.questions
   #    }
   ##################################################################################
   #Refactored--> Replaced with a methood call
   list_questions (@assignment)
   #################################################################################
   @scores = @participant.scores(@questions)
 end

#Refactored #Created a method which was a duplicate in conflict_notification and edit methods
 def list_questions (assignment)
   @questions = Hash.new
   questionnaires = assignment.questionnaires
   questionnaires.each {
       |questionnaire|
     @questions[questionnaire.symbol] = questionnaire.questions
   }
 end