CSC/ECE 517 Fall 2013/oss aoa

OSS Project E 819 - Refactoring and Testing - Assignment

This Wiki page provides a detailed description of the Open Source Software project conducted on Expertiza, as part of the Object Oriented Languages and Systems coursework.

Introduction to Expertiza

Expertizais an open source project built using the Ruby on Rails platform. It 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 to manage peer reviews in coursework. The source code can be forked from github and cloned for performing modifications. The Expertiza environment is already set up in NC State's VCL image "Ruby on Rails". If you have access, this is quickest way to get a development environment running for Expertiza. See the Expertiza wiki(provide hyperlink) on developing Expertiza on the VCL.

Overview of project

The main objective of the project was to refactor three Ruby files:

• assignment.rb (925 lines)

• assignment_participant.rb (385 lines)

• assignment_team.rb (254 lines)

These three files are responsible for creation and management of assignments in Expertiza. They perform operations relating to a user participating in an assignment. AssignmentParticipant is a subclass of class Participant. CourseParticipant is Participant’s only other subclass. If a course has CourseParticipants, then an assignment can take its participants from the CourseParticipants.

Project Requirements

1. Do methods 'get_percentage_reviews_completed' and 'get_total_reviews_completed_by_type_and_date' belong to this class? The assignment.rb file should only contain those methods that pertain to the creation and maintenance of assignments in Expertiza.

2. The self.export method contains a lot of statements that have been repeated. Refactor this method to avoid duplication.

3. Look for other methods in the assignment.rb file that shouldn't belong to this file but to some other model/controller.

4. The 'get_scores' method in the 'assignment', 'assignment_participant' and 'assignment_team' files contain lines of code that appear to be repeated. Can this be refactored so that only one method implements the common functionality?

5. Refactor 'get_hyperlinks' to replace the conditional statement with polymorphism.

6. Refactor the 'get_reviews' method by replacing the conditional statement with polymorphism.

7. Look for any unused methods or variables in these files.

8. Also apply other refactorings such as Rename variable, Rename method to give the variables and methods more meaningful names.

Design Changes (Refactoring) Performed

Describe all refactorings done in detail along with code snippets.

Following are the code snippets of all the files that have been refactored as part of the project:

Refactoring done in assignment.rb

1. function candidate_topics_to_review

Code before Refactoring:

contributor_set.reject! { |contributor| signed_up_topic(contributor).nil? or !contributor.has_submissions? }

# Reject contributions of topics whose deadline has passed
contributor_set.reject! { |contributor| contributor.assignment.get_current_stage(signed_up_topic(contributor).id) == 'Complete' or
                                            contributor.assignment.get_current_stage(signed_up_topic(contributor).id) == 'submission' }

Code after refactoring (merging reject statements)

contributor_set.reject! do |contributor|
      signed_up_topic(contributor).nil? || !contributor.has_submissions? ||
          contributor.assignment.get_current_stage(signed_up_topic(contributor).id) == 'Complete' ||
          contributor.assignment.get_current_stage(signed_up_topic(contributor).id) == 'submission'

2. Refactoring of self.export method

Code before refactoring:

for index in 0 .. @scores[:teams].length - 1
      team = @scores[:teams][index.to_s.to_sym]
      for participant in team[:team].get_participants
        pscore = @scores[:participants][participant.id.to_s.to_sym]
        tcsv = Array.new
        tcsv << 'team'+index.to_s

        if options['team_score'] == 'true'
          if team[:scores]
            tcsv.push(team[:scores][:max], team[:scores][:avg], team[:scores][:min], participant.fullname)
          else
            tcsv.push('---', '---', '---')
          end
        end

        if options['submitted_score']
          if pscore[:review]
            tcsv.push(pscore[:review][:scores][:max], pscore[:review][:scores][:min], pscore[:review][:scores][:avg])
          else
            tcsv.push('---', '---', '---')
          end
        end

        if options['metareview_score']
          if pscore[:metareview]
            tcsv.push(pscore[:metareview][:scores][:max], pscore[:metareview][:scores][:min], pscore[:metareview][:scores][:avg])
          else
            tcsv.push('---', '---', '---')
          end
        end

        if options['author_feedback_score']
          if pscore[:feedback]
            tcsv.push(pscore[:feedback][:scores][:max], pscore[:feedback][:scores][:min], pscore[:feedback][:scores][:avg])
          else
            tcsv.push('---', '---', '---')
          end
        end

        if options['teammate_review_score']
          if pscore[:teammate]
            tcsv.push(pscore[:teammate][:scores][:max], pscore[:teammate][:scores][:min], pscore[:teammate][:scores][:avg])
          else
            tcsv.push('---', '---', '---')
          end
        end

        tcsv.push(pscore[:total_score])
        csv << tcsv
      end
    end

Code after refactoring (merging the if-else statements and replacing them with ternary operator):

for index in 0 .. @scores[:teams].length - 1
      team = @scores[:teams][index.to_s.to_sym]
      for participant in team[:team].get_participants
        pscore = @scores[:participants][participant.id.to_s.to_sym]
        tcsv = Array.new
        tcsv << 'team'+index.to_s

        team[:scores] ?
            tcsv.push(team[:scores][:max], team[:scores][:avg], team[:scores][:min], participant.fullname) :
            tcsv.push('---', '---', '---') if options['team_score'] == 'true'

        pscore[:review] ?
            tcsv.push(pscore[:review][:scores][:max], pscore[:review][:scores][:min], pscore[:review][:scores][:avg]) :
            tcsv.push('---', '---', '---') if options['submitted_score']

        pscore[:metareview] ?
            tcsv.push(pscore[:metareview][:scores][:max], pscore[:metareview][:scores][:min], pscore[:metareview][:scores][:avg]) :
            tcsv.push('---', '---', '---') if options['metareview_score']

        pscore[:feedback] ?
            tcsv.push(pscore[:feedback][:scores][:max], pscore[:feedback][:scores][:min], pscore[:feedback][:scores][:avg]) :
            tcsv.push('---', '---', '---') if options['author_feedback_score']

        pscore[:teammate] ?
            tcsv.push(pscore[:teammate][:scores][:max], pscore[:teammate][:scores][:min], pscore[:teammate][:scores][:avg]) :
            tcsv.push('---', '---', '---') if options['teammate_review_score']

        tcsv.push(pscore[:total_score])
        csv << tcsv
      end
    end

3. Replaced all occurrences of if statements with one-line if statements

Code before refactoring:

if min_reviews > 0 
  contributor_set.sort! { |a, b| a.review_mappings.last.id <=> b.review_mappings.last.id }	 	
end

Code after refactoring:

contributor_set.sort! { |a, b| a.review_mappings.last.id <=> b.review_mappings.last.id } if min_reviews > 0

4. Replaced all occurrences of if-else-end with ternary operator

Code before refactoring:

def is_using_dynamic_reviewer_assignment?
if self.review_assignment_strategy == RS_AUTO_SELECTED or self.review_assignment_strategy == RS_STUDENT_SELECTED
  true
else	 	
 false	 	
end

Code after refactoring:

def dynamic_reviewer_assignment?
(self.review_assignment_strategy == RS_AUTO_SELECTED || self.review_assignment_strategy == RS_STUDENT_SELECTED) ? true : false

5. Replaced all occurrences of for loops with an each iterator

Code before refactoring:

for rm in reviewer_mappings
if rm.reviewee.id != mapping.reviewee.id
  review_num += 1
else
  break
end

Code after refactoring:

reviewer_mappings.each do |rm|
  (rm.reviewee.id != mapping.reviewee.id) ? review_num += 1 : break
end

Replaced all occurrences of "AND" and "OR" with logical && and || Code before refactoring:

raise 'Please select a topic' if has_topics? and topic.nil?
raise 'This assignment does not have topics' if !has_topics? and topic

Code after refactoring:

raise 'Please select a topic' if has_topics? && topic.nil?
raise 'This assignment does not have topics' if !has_topics? && topic

Refactoring done in assignment_participant.rb

Refactoring of get_hyperlinks method

Code after refactoring:

def get_hyperlinks
    team.try :get_hyperlinks
  end

def get_hyperlinks_array
  self.submitted_hyperlinks.nil? ? [] : YAML::load(self.submitted_hyperlinks)
end