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>
- Rename methods prefixed with “get”.
- Move methods to appropriate file helper classes.
- Move methods to appropriate models.
- Remove unused methods
- 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(questions) => scores(questions) 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(options) => self.export_fields(options) get_path => path get_current_stage => current_stage get_stage_deadline => 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 from below are not getting invoked from anywhere, so they should be removed from this model.
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/>