CSC/ECE 517 Spring 2015/oss E1505 xzl

Expertiza - Refactoring AssignmentParticipant model

Expertiza is a web application developed using Ruby on Rails that serves as a peer-review system. The application allows students to submit and peer-review learning objects (articles, code, web sites, etc)<ref>Expertiza on GitHub</ref><ref>Expertiza Wiki Page</ref>. It is an open source project and it's codebase is maintained in GitHub. We are contributing to Expertiza as a part of our Object-Oriented Design and Development's Open-Source Software (OSS) Project. Our goal in this project is to refactor the assignment_participant model. In this Wiki Page, we will explain the changes that we have made for the same.

Project Description

The assignment_participant model is subclass of Participant model and is used to maintain the list of students/users participating in a given assignment.
The changes that are needed to be done are described as follows:<ref>GoogleDoc for our project requirements</ref>

1. Rename methods prefixed with “get”.
2. Move methods to appropriate file helper classes.
3. Move methods to appropriate models.
4. Remove unused methods
5. Use good Ruby style guidelines in the code.

Modifications made to the Existing Code

Rename methods prefixed with “get”

---

According to Ruby naming conventions, the name of getter and setter methods need not to be verbs and they should not be prefixed with "get".
The following methods were renamed.

get_reviewers => reviewers
get_scores => scores
get_members => members
get_team_hyperlinks => team_hyper_links
get_hyperlinks_array => hyperlinks_array
get_course_string => course_string
get_feedback => feedback
get_reviews => reviews
self.get_export_fields => self.export_fields
get_path => path
get_current_stage => current_stage
def get_stage_deadline => def stage_deadline

Move methods to appropriate file helper classes

---

Methods get_submitted_files, get_files should not be in this class. They deal with files and should be moved to appropriate file helper classes.

Move methods to appropriate models

---

reviewed_by? , quiz_taken_by? do not belong to AssignmentParticipant model. Move them to appropriate models. Because in the reviewed_by? method the keyword ParticipantReviewResponseMap is referenced, so we move the method reviewed_by? to model app/models/participant_review_response_map.rb, also the same to quiz_taken_by?, since the keyword QuizQuestionnaire is referenced in method quiz_taken_by?, so we move it to model app/models/quiz_questionnaire.rb. Also We did the same operation to method get_quizzes_taken to move it to app/models/quiz_response_map.rb.
Below we use a pair picture of method reviewed_by? as an example to demonstrate operation of this category.

Remove unused methods

---

Methods is_reviewed_by? , quiz_taken_by? are not getting invoked from anywhere. Find all such methods to see if we need them and delete the methods if we don’t need them. Additionally, get_two_node_cycles, get_three_node_cycles, get_four_node_cycles, get_cycle_similarity_score, get_cycle_deviation_score are also available in CollusionCycle model. Determine if we need them in AssignmentParticipant model and delete the methods if we don’t need them.
The following method are removed:

average_score_per_assignment(assignment_id)
quiz_taken_by?(contributor, reviewer)
has_quiz?
reviewees
get_two_node_cycles
get_three_node_cycles
get_four_node_cycles
get_cycle_similarity_score(cycle)
get_cycle_deviation_score(cycle)
get_reviews_by_reviewer(reviewer)
get_submitted_files()
get_files(directory)
get_wiki_submissions
get_hash
review_response_maps
get_topic_string

Use good Ruby style guidelines in the code

---

At many places in the code we found that the Ruby Style Guidelines were not followed. We have refactored the code so that it uses the good code style guidelines. The following are some of the refactorings the we have done:<ref>https://docs.google.com/a/ncsu.edu/document/d/1qQD7fcypFk77nq7Jx7ZNyCNpLyt1oXKaq5G-W7zkV3k/edit</ref>

Use `&&` and `||` rather than `and` and `or` to keep boolean precedence

---

Before Refactoring

 
line 596: if self.user.handle == nil or self.user.handle == ""
line 624: if member.directory_num == nil or member.directory_num < 0

After Refactoring

 
line 404: if self.user.handle == nil || self.user.handle == ""
line 432: if member.directory_num == nil || member.directory_num < 0

@users_team.size == 0 should be @users_team.zero?

---

Before Refactoring

 
line 45: return 0 if number_of_scores == 0
line 55: return 0 if self.response_maps.size == 0
line 294: if length_of_assessments=scores[round_sym][:assessments].nil? || scores[round_sym][:assessments].length==0
line 420: if course.name.strip.length == 0
line 538: if all({conditions: ['user_id=? && parent_id=?', user.id, id]}).size == 0

After Refactoring

 
line 45: return 0 if number_of_scores.zero?
line 55: return 0 if self.response_maps.size.zero?
line 178: if length_of_assessments=scores[round_sym][:assessments].nil? || scores[round_sym][:assessments].length.zero?
line 304: if course.name.strip.length.zero?
line 355: if all({conditions: ['user_id=? && parent_id=?', user.id, id]}).size.zero?

Use 'key:value' rather than ':key=>value'

---

Before Refactoring

 
line 14:  belongs_to  :assignment, :class_name => 'Assignment', :foreign_key => 'parent_id'
line 15:  has_many    :review_mappings, :class_name => 'ParticipantReviewResponseMap', :foreign_key => 'reviewee_id'
line 16:  has_many    :quiz_mappings, :class_name => 'QuizResponseMap', :foreign_key => 'reviewee_id'
line 87： ParticipantReviewResponseMap.create(:reviewee_id => self.id, :reviewer_id => reviewer.id,
line 88:   :reviewed_object_id => assignment.id)
line 95:   QuizResponseMap.create(:reviewed_object_id => quiz.id,:reviewee_id => contributor.id, :reviewer_id => reviewer.id,
line 96:   :type=>"QuizResponseMap", :notification_accepted => 0)
line 100:  return AssignmentParticipant.where(:user_id=>user_id,:parent_id=>assignment_id).first
line 413：CourseParticipant.create(:user_id => self.user_id, :parent_id => course_id) if part.nil?
line 539:  new_part = AssignmentParticipant.create(:user_id => user.id, :parent_id => id)
line 612: new_submit = ResubmissionTime.new(:resubmitted_at => Time.now.to_s)
line 618：max_num = AssignmentParticipant.where(['parent_id = ?', self.parent_id], :order => 'directory_num desc').first.directory_num

After Refactoring

line 14: belongs_to  :assignment, class_name: 'Assignment', foreign_key: 'parent_id'
line 15: has_many    :review_mappings, class_name: 'ParticipantReviewResponseMap', foreign_key: 'reviewee_id'
line 16: has_many    :quiz_mappings, class_name: 'QuizResponseMap', foreign_key: 'reviewee_id' 
line 74: ParticipantReviewResponseMap.create(reviewee_id: self.id, reviewer_id: reviewer.id,
line 75: reviewed_object_id: assignment.id)
line 82: QuizResponseMap.create(reviewed_object_id: quiz.id,reviewee_id: contributor.id, reviewer_id: reviewer.id,
line 83: type:"QuizResponseMap", notification_accepted: 0)
line 87: return AssignmentParticipant.where(user_id:user_id,parent_id:assignment_id).first
line 297: CourseParticipant.create(user_id: self.user_id, parent_id: course_id) if part.nil?
line 356: new_part = AssignmentParticipant.create(user_id: user.id, parent_id: id)
line 420: new_submit = ResubmissionTime.new(resubmitted_at: Time.now.to_s)
line 426: max_num = AssignmentParticipant.where(['parent_id = ?', self.parent_id], order: 'directory_num desc').first.directory_num

Use good array checking

---

Before Refactoring

 
line 264: if(round!=nil) 
line 301: if(scores[round_sym][:scores][:max]!=nil && scores[review_sym][:scores][:max]<scores[round_sym][:scores][:max]) 
line 304: if(scores[round_sym][:scores][:min]!= nil && scores[review_sym][:scores][:min]>scores[round_sym][:scores][:min])
line 307: if(scores[round_sym][:scores][:avg]!=nil)
line 598: elsif AssignmentParticipant.where(parent_id: self.assignment.id, handle: self.user.handle).length > 0

After Refactoring

line 148: if(!round.nil?)
line 185: if(!scores[round_sym][:scores][:max].nil? && scores[review_sym][:scores][:max]>scores[round_sym][:scores][:max])
line 188: if(!scores[round_sym][:scores][:min].nil? && scores[review_sym][:scores][:min]>scores[round_sym][:scores][:min])
line 191: if(!scores[round_sym][:scores][:avg].nil?)
line 406: elsif AssignmentParticipant.where(parent_id: self.assignment.id, handle: self.user.handle).length.any?

The detailed change-set for this refactor can be seen here <ref>our github commit</ref>.

References

<references/>