CSC/ECE 517 Spring 2015/oss E1505 xzl: Difference between revisions

From Expertiza_Wiki
Jump to navigation Jump to search
No edit summary
No edit summary
 
(13 intermediate revisions by the same user not shown)
Line 1: Line 1:
==Expertiza - Refactoring AssignmentParticipant model==
==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>[https://github.com/expertiza/expertiza Expertiza on GitHub]</ref><ref>[http://wikis.lib.ncsu.edu/index.php/Expertiza 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 <b>StudentQuiz controller</b>. In this Wiki Page, we would be explaining the changes that we have made for the same.
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>[https://github.com/expertiza/expertiza Expertiza on GitHub]</ref><ref>[http://wikis.lib.ncsu.edu/index.php/Expertiza 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 <b>assignment_participant model</b>. In this Wiki Page, we will explain the changes that we have made for the same.
__TOC__
__TOC__


==Project Description==
==Project Description==
The <b>AssignmentParticipant model</b> is subclass of Participant model and is used to maintain the list of students/users participating in a given assignment.
The <b>assignment_participant model</b> is subclass of Participant model and is used to maintain the list of students/users participating in a given assignment.
<br>The changes that are needed to be done are described as follows:<ref>[https://docs.google.com/a/ncsu.edu/document/d/1J0WUBtYV_MDhpEQ-50z8gXE-OrvVkpEaZxvn_9RCAsM/edit# GoogleDoc for our project requirements]</ref>
<br>The changes that are needed to be done are described as follows:<ref>[https://docs.google.com/a/ncsu.edu/document/d/1J0WUBtYV_MDhpEQ-50z8gXE-OrvVkpEaZxvn_9RCAsM/edit# GoogleDoc for our project requirements]</ref>
<ol>
<ol>
Line 22: Line 22:
<pre>
<pre>
get_reviewers => reviewers
get_reviewers => reviewers
get_scores => scores
get_scores(questions) => scores(questions)
get_members => members
get_members => members
get_team_hyperlinks => team_hyper_links
get_team_hyperlinks => team_hyper_links
Line 29: Line 29:
get_feedback => feedback
get_feedback => feedback
get_reviews => reviews
get_reviews => reviews
self.get_export_fields => self.export_fields
self.get_export_fields(options) => self.export_fields(options)
get_path => path
get_path => path
get_current_stage => current_stage
get_current_stage => current_stage
def get_stage_deadline => def stage_deadline
get_stage_deadline => stage_deadline
</pre>
</pre>


Line 47: Line 47:
===Remove unused methods===
===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. <br>
Methods from below are not getting invoked from anywhere, so they should be removed from this model. <br>
The following method are removed:
The following method are removed:
<pre>
<pre>
Line 69: Line 69:
===Use good Ruby style guidelines in the code===
===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:-
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>
<br>
<br>
==== Use `&&` and `||` rather than `and` and `or` to keep boolean precedence  ====
==== Use `&&` and `||` rather than `and` and `or` to keep boolean precedence  ====
Line 107: Line 107:
</pre>
</pre>


==== @users_team.size == 0 should be @users_team.zero? ====
==== Use 'key:value' rather than ':key=>value'====
----
----
<b>Before Refactoring</b>
<b>Before Refactoring</b>
Line 163: Line 163:
</pre>
</pre>


The detailed change-set for this refactor can be seen here <ref>[https://github.com/z-Xie/expertiza/commit/c0d696476616f3f1ea429bda7eb8b52ccd10cdb2?diff=unified our github commit]</ref>.
==References==
==References==
<references/><br>
<references/><br>

Latest revision as of 21:40, 23 March 2015

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