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

6. 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

1. 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

2. Discarded all unnecessary return statements at the end of functions and eliminated parantheses

Code before refactoring:

def get_submitted_files()
    files = Array.new
    if(self.directory_num)      
      files = get_files(self.get_path)
    end
    return files
  end

Code after refactoring:

def get_submitted_files
    files = Array.new
    files = get_files(self.get_path) if self.directory_num
    files
  end 

Refactoring done in assignment_team.rb

1. Refactoring done on method has_submissions?

Before refactoring:

def has_submissions?
    participants.each do |participant|
      return true if participant.has_submissions?
    end
    return false
  end

After refactoring(Replacing do block with single line {} block and discarding return statement):

def has_submissions?
  participants.each { |participant| return true if participant.has_submissions? }
  false
end

Future Work

The following tasks can be performed as future work in this project:

1. Functional tests for newly added methods

References

Rails Guide

Ruby Style Guide

External Links

RubyMonk