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

Expertiza is 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 page 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

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'
end

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	 	
end

Code after refactoring:

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

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

List of classes and corresponding methods refactored

1. assignment.rb
   1. candidate_topics_to_review
   2. contributor_to_review(reviewer, topic)
   3. response_map_to_metareview(metareviewer)
   4. is_using_dynamic_reviewer_assignment?
   5. get_contributor(contrib_id)
   6. get_path
   7. check_condition(column, topic_id = nil)
   8. submission_allowed(topic_id = nil)
   9. review_allowed(topic_id = nil)
   10. metareview_allowed(topic_id=nil)
   11. email(author_id)
   12. get_review_number(mapping)
   13. is_wiki_assignment
   14. is_microtask?
   15. create_node
   16. get_current_stage(topic_id = nil)
   17. get_stage_deadline(topic_id = nil)
   18. get_review_rounds
   19. get_review_questionnaire_id
   20. get_next_due_date
   21. find_next_stage()
   22. get_total_reviews_assigned_by_type(type)
   23. get_total_reviews_completed_by_type_and_date(type, date)
   24. compute_total_score(scores)

1. assignment_participant.rb
   1. includes?(participant)
   2. reviewed_by?(reviewer)
   3. has_submissions?
   4. get_reviewees
   5. get_reviewers
   6. get_cycle_similarity_score(cycle)
   7. get_cycle_deviation_score(cycle)
   8. copy(course_id)
   9. get_submitted_files
   10. get_wiki_submissions
   11. self.import(row,session,id)
   12. self.get_export_fields(options)
   13. set_handle()
   14. set_student_directory_num
   15. get_topic_string

1. assignment_team.rb
   1. includes?(participant)
   2. reviewed_by?(reviewer)
   3. has_submissions?
   4. reviewed_contributor?(contributor)
   5. get_hyperlinks
   6. get_review_map_type
   7. self.handle_duplicate(team, name, assignment_id, handle_duplicates)
   8. self.import(row,session,assignment_id,options)
   9. get_participants
   10. add_participant
   11. get_scores(questions)
   12. self.get_team(participant)
   13. self.get_export_fields(options)
   14. self.create_team_and_node(assignment_id)

Common refactoring done in all files

1. Merged all if-else-end statements and replaced them with ternary operator.

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

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

4. Replaced all occurrences of "AND" and "OR" with logical && and ||.

5. Discarded all unnecessary return statements at the end of functions and eliminated parentheses.

Future Work

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

1. Functional tests for newly added methods

References

1. Rails Guide

2. Ruby Style Guide

3. refactoring.com

4. Refactoring Notes

External Links

[1].RubyMonk

[2].Refactoring in Ruby

[3]. Expertiza Repo

[4]. VCL Link

[5]. Steps to setup Expertiza