CSC/ECE 517 Fall 2017/E1770 Refactor assignment participant.rb

E1770. [Test First Development] Refactor assignment_participant.rb<ref>Project Description document https://docs.google.com/document/d/1rdolBAHxVGI9I0N-cT866AqnfORM2L1_m_bo2gRYRrI/edit#heading=h.s18ldps2kz5e</ref>

This page provides a brief description of the Expertiza project. The project is aimed at refactoring the AssignmentParticipant model which is subclass of Participant model. This model is used to maintain the list of students/users participating in a given assignment. For any new or existing assignments, this model manages the entire list of users assigned to that particular assignment. It primarily includes method for scores calculations, assignment submission paths, reviews, etc. As part of this project, some of the methods have been removed which were not being used anywhere, some have been moved to their appropriate helper module and some have been refactored into smaller ones.

Introduction to Expertiza

Expertiza is a peer review based system which provides incremental learning from the class. This project has been developed together by faculty and students using Ruby on Rails framework. Expertiza allows the instructor to create, edit and delete assignments, create new assignment topics, assign them to a particular class or selected students, have students work on teams and then review each other's assignments at the end. For the students, they can signup for topics, form teams, and submit their projects and assignments. Students then review the work done by other students and give suggestions to improve. Teams after reviews are allotted scores and they can refer to the peer comments to further improve their work. It also supports submission of different file types for assignments, including the URLs and wiki pages.

Why refactoring?

Code Refactoring<ref>Refactoring https://en.wikipedia.org/wiki/Code_refactoring</ref> is the process of restructuring existing computer code—changing the factoring—without changing its external behavior.

Refactoring improves nonfunctional attributes of the software. Advantages include improved code readability and reduced complexity; these can improve source-code maintainability and create a more expressive internal architecture or object model to improve extensibility. Typically, refactoring applies a series of standardised basic micro-refactorings, each of which is (usually) a tiny change in a computer program's source code that either preserves the behaviour of the software, or at least does not modify its conformance to functional requirements. Many development environments provide automated support for performing the mechanical aspects of these basic refactorings. If done extremely well, code refactoring may also resolve hidden, dormant, or undiscovered bugs or vulnerabilities in the system by simplifying the underlying logic and eliminating unnecessary levels of complexity. If done poorly it may fail the requirement that external functionality not be changed, introduce new bugs, or both.

Test-driven development

Test-driven development (TDD)<ref>Refactoring https://en.wikipedia.org/wiki/Test-driven_development</ref> is a software development process that relies on the repetition of a very short development cycle: Requirements are turned into very specific test cases, then the software is improved to pass the new tests, only. This is opposed to software development that allows software to be added that is not proven to meet requirements.

Project Description

Refactor AssignmentParticipant model which is a subclass of Participant model. The following tasks have been performed as per the requirements.

• Refactor scores method.
  • Write failing tests first.
  • Split into several simpler methods and assign reasonable names.
  • Extract duplicated code into separate methods.
  • Replace the conditional with the relevant method calls.

• Method files is exactly the same as assignment_team.rb L103.
  • Write failing tests first.
  • Solve the duplication, extract method to a new file or delete useless one.

• Method self.import is exact the same as course_participant.rb L21.
  • Write failing tests first.
  • Solve the duplication, extract method to a new file or delete useless one.

• Use find_by instead of dynamic method.
  • Write failing tests first.

• Use find_by instead of where.first.
  • Write failing tests first.

Refactoring

Scores method

The method scores has been converted to smaller methods scores, assignment_questionnaires, merge_scores, calculate_scores. It is a good practice to keep the methods not extremely long as they tend to complicate the functionality and the readability.

Before

After

def scores(questions) scores = {} scores[:participant] = self self.assignment.questionnaires.each do |questionnaire| round = AssignmentQuestionnaire.find_by_assignment_id_and_questionnaire_id(self.assignment.id, questionnaire.id).used_in_round #create symbol for "varying rubrics" feature -Yang if(round!=nil) questionnaire_symbol = (questionnaire.symbol.to_s+round.to_s).to_sym else questionnaire_symbol = questionnaire.symbol end scores[questionnaire_symbol] = {} if round==nil scores[questionnaire_symbol][:assessments] = questionnaire.get_assessments_for(self) else scores[questionnaire_symbol][:assessments] = questionnaire.get_assessments_round_for(self,round) end scores[questionnaire_symbol][:scores] = Answer.compute_scores(scores[questionnaire_symbol][:assessments], questions[questionnaire_symbol]) end scores[:total_score] = self.assignment.compute_total_score(scores) #merge scores[review#] (for each round) to score[review] -Yang if self.assignment.varying_rubrics_by_round? review_sym = "review".to_sym scores[review_sym] = Hash.new scores[review_sym][:assessments] = Array.new scores[review_sym][:scores] = Hash.new scores[review_sym][:scores][:max] = -999999999 scores[review_sym][:scores][:min] = 999999999 scores[review_sym][:scores][:avg] = 0 total_score = 0 for i in 1..self.assignment.get_review_rounds round_sym = ("review"+i.to_s).to_sym if scores[round_sym][:assessments].nil? || scores[round_sym][:assessments].length==0 next end length_of_assessments=scores[round_sym][:assessments].length.to_f scores[review_sym][:assessments]+=scores[round_sym][:assessments] if(scores[round_sym][:scores][:max]!=nil && scores[review_sym][:scores][:max]<scores[round_sym][:scores][:max]) scores[review_sym][:scores][:max]= scores[round_sym][:scores][:max] end if(scores[round_sym][:scores][:min]!= nil && scores[review_sym][:scores][:min]>scores[round_sym][:scores][:min]) scores[review_sym][:scores][:min]= scores[round_sym][:scores][:min] end if(scores[round_sym][:scores][:avg]!=nil) total_score += scores[round_sym][:scores][:avg]*length_of_assessments end end if scores[review_sym][:scores][:max] == -999999999 && scores[review_sym][:scores][:min] == 999999999 scores[review_sym][:scores][:max] = 0 scores[review_sym][:scores][:min] = 0 end scores[review_sym][:scores][:avg] = total_score/scores[review_sym][:assessments].length.to_f end # In the event that this is a microtask, we need to scale the score accordingly and record the total possible points # PS: I don't like the fact that we are doing this here but it is difficult to make it work anywhere else if assignment.is_microtask? topic = SignUpTopic.find_by_assignment_id(assignment.id) if !topic.nil? scores[:total_score] *= (topic.micropayment.to_f / 100.to_f) scores[:max_pts_available] = topic.micropayment end end # for all quiz questionnaires (quizzes) taken by the participant quiz_responses = Array.new quiz_response_mappings = QuizResponseMap.where(reviewer_id: self.id) quiz_response_mappings.each do |qmapping| if (qmapping.response) quiz_responses << qmapping.response end end #scores[:quiz] = Hash.new #scores[:quiz][:assessments] = quiz_responses #scores[:quiz][:scores] = Answer.compute_quiz_scores(scores[:quiz][:assessments]) scores[:total_score] = assignment.compute_total_score(scores) #scores[:total_score] += compute_quiz_scores(scores) # move lots of calculation from view(_participant.html.erb) to model if self.grade scores[:total_score] = self.grade else total_score = scores[:total_score] if total_score > 100 total_score = 100 end scores[:total_score] = total_score scores end end

scores(questions) method def scores(questions) scores = {} scores[:participant] = self assignment_questionnaires(questions, scores) scores[:total_score] = self.assignment.compute_total_score(scores) merge_scores(scores) # In the event that this is a microtask, we need to scale the score accordingly and record the total possible points # PS: I don't like the fact that we are doing this here but it is difficult to make it work anywhere else if assignment.is_microtask? topic = SignUpTopic.find_by_assignment_id(assignment.id) if !topic.nil? scores[:total_score] *= (topic.micropayment.to_f / 100.to_f) scores[:max_pts_available] = topic.micropayment end end # for all quiz questionnaires (quizzes) taken by the participant quiz_responses = Array.new quiz_response_mappings = QuizResponseMap.where(reviewer_id: self.id) quiz_response_mappings.each do |qmapping| if (qmapping.response) quiz_responses << qmapping.response end end #scores[:quiz] = Hash.new #scores[:quiz][:assessments] = quiz_responses #scores[:quiz][:scores] = Answer.compute_quiz_scores(scores[:quiz][:assessments]) scores[:total_score] = assignment.compute_total_score(scores) #scores[:total_score] += compute_quiz_scores(scores) calculate_scores(scores) end assignment_questionnaires(questions, scores) method def assignment_questionnaires(questions, scores) self.assignment.questionnaires.each do |questionnaire| round = AssignmentQuestionnaire.find_by_assignment_id_and_questionnaire_id(self.assignment.id, questionnaire.id).used_in_round #create symbol for "varying rubrics" feature -Yang if(round!=nil) questionnaire_symbol = (questionnaire.symbol.to_s+round.to_s).to_sym else questionnaire_symbol = questionnaire.symbol end scores[questionnaire_symbol] = {} if round==nil scores[questionnaire_symbol][:assessments] = questionnaire.get_assessments_for(self) else scores[questionnaire_symbol][:assessments] = questionnaire.get_assessments_round_for(self,round) end scores[questionnaire_symbol][:scores] = Answer.compute_scores(scores[questionnaire_symbol][:assessments], questions[questionnaire_symbol]) end end merge_scores(scores) method def merge_scores(scores) #merge scores[review#] (for each round) to score[review] -Yang if self.assignment.varying_rubrics_by_round? review_sym = "review".to_sym scores[review_sym] = Hash.new scores[review_sym][:assessments] = Array.new scores[review_sym][:scores] = Hash.new scores[review_sym][:scores][:max] = -999999999 scores[review_sym][:scores][:min] = 999999999 scores[review_sym][:scores][:avg] = 0 total_score = 0 for i in 1..self.assignment.get_review_rounds round_sym = ("review"+i.to_s).to_sym if scores[round_sym][:assessments].nil? || scores[round_sym][:assessments].length==0 next end length_of_assessments=scores[round_sym][:assessments].length.to_f scores[review_sym][:assessments]+=scores[round_sym][:assessments] if(scores[round_sym][:scores][:max]!=nil && scores[review_sym][:scores][:max]<scores[round_sym][:scores][:max]) scores[review_sym][:scores][:max]= scores[round_sym][:scores][:max] end if(scores[round_sym][:scores][:min]!= nil && scores[review_sym][:scores][:min]>scores[round_sym][:scores][:min]) scores[review_sym][:scores][:min]= scores[round_sym][:scores][:min] end if(scores[round_sym][:scores][:avg]!=nil) total_score += scores[round_sym][:scores][:avg]*length_of_assessments end end if scores[review_sym][:scores][:max] == -999999999 && scores[review_sym][:scores][:min] == 999999999 scores[review_sym][:scores][:max] = 0 scores[review_sym][:scores][:min] = 0 end scores[review_sym][:scores][:avg] = total_score/scores[review_sym][:assessments].length.to_f end end calculate_scores(scores) method def calculate_scores(scores) # move lots of calculation from view(_participant.html.erb) to model if self.grade scores[:total_score] = self.grade else total_score = scores[:total_score] if total_score > 100 total_score = 100 end scores[:total_score] = total_score scores end end

set_handle method

The set_handle method had an unncessary ELSIF statement which was performing the same action self.handle = self.user.name. This has been clubbed as an OR condition to the IF statement and the first ELSIF statement has been removed.

Before	After
def set_handle if self.user.handle == nil or self.user.handle == "" self.handle = self.user.name elsif AssignmentParticipant.where(parent_id: self.assignment.id, handle: self.user.handle).length > 0 self.handle = self.user.name else self.handle = self.user.handle end self.save! end	def set_handle if self.user.handle == nil or self.user.handle == "" or AssignmentParticipant.where(parent_id: self.assignment.id, handle: self.user.handle).length > 0 self.handle = self.user.name else self.handle = self.user.handle end self.save! end

File and directory methods

The files and directory methods from AssignmentParticipant model have been moved to the FileHelper module. The methods files, submitted_files and dir_path are appropriate for the FileHelper' module and hence been moved there. The below line was added to the AssignmentParticipant model. include FileHelper

The AssignmentParticipant class would look like

 class AssignmentParticipant < Participant
   require 'wiki_helper'
   include FileHelper

Methods moved to FileHelper:

• submitted_files

 def submitted_files
   files(self.path) if self.directory_num
 end

• files(directory)

 def files(directory)
   files_list = Dir[directory + "/*"]
   files = Array.new

   files_list.each do |file|
     if File.directory?(file)
       dir_files = files(file)
       dir_files.each{|f| files << f}
     end
     files << file
   end
   files
 end

• submitted_files()

 def submitted_files()
   files = Array.new
   if(self.directory_num)
     files = files(self.path)
   end
   return files
 end

• dir_path

 def dir_path
   assignment.try :directory_path
 end

Remove unused methods

The following methods have been removed as they were not being used by the application.

def average_score_per_assignment(assignment_id)

def average_score_per_assignment(assignment_id)
   return 0 if self.response_maps.size == 0

   sum_of_scores = 0

   self.response_maps.metareview_response_maps.each do |metaresponse_map|
     if !metaresponse_map.response.empty? && response_map == assignment_id then
       sum_of_scores = sum_of_scores + response_map.response.last.average_score
     end
   end

   (sum_of_scores / self.response_maps.size).to_i
 end

def quiz_taken_by?(contributor, reviewer)

 def quiz_taken_by?(contributor, reviewer)
   quiz_id = QuizQuestionnaire.find_by_instructor_id(contributor.id)
   return QuizResponseMap.where(['reviewee_id = ? AND reviewer_id = ? AND reviewed_object_id = ?',
                                 self.id, reviewer.id, quiz_id]).count > 0
 end

def has_quiz?

 def has_quiz?
   return !QuizQuestionnaire.find_by_instructor_id(self.id).nil?
 end

def reviewees

 def reviewees
   reviewees = []
   rmaps = ResponseMap.all(conditions: ["reviewer_id = #{self.id} && type = 'ReviewResponseMap'"])
       rmaps.each { |rm| reviewees.concat(AssignmentTeam.find(rm.reviewee_id).participants) }

   reviewees
 end

def two_node_cycles

 def two_node_cycles
   cycles = []
   self.reviewers.each do |ap|
     if ap.reviewers.include?(self)
       self.reviews_by_reviewer(ap).nil? ? next : s01 = self.reviews_by_reviewer(ap).get_total_score
       ap.reviews_by_reviewer(self).nil? ? next : s10 = ap.reviews_by_reviewer(self).get_total_score
       cycles.push([[self, s01], [ap, s10]])
     end
   end
   cycles
 end

def three_node_cycles

 def three_node_cycles
   cycles = []
   self.reviewers.each do |ap1|
     ap1.reviewers.each do |ap2|
       if ap2.reviewers.include?(self)
         self.reviews_by_reviewer(ap1).nil? ? next : s01 = self.reviews_by_reviewer(ap1).get_total_score
         ap1.reviews_by_reviewer(ap2).nil? ? next : s12 = ap1.reviews_by_reviewer(ap2).get_total_score
         ap2.reviews_by_reviewer(self).nil? ? next : s20 = ap2.reviews_by_reviewer(self).get_total_score
         cycles.push([[self, s01], [ap1, s12], [ap2, s20]])
       end
     end
   end
   cycles
 end

def four_node_cycles

 def four_node_cycles
   cycles = []
   self.reviewers.each do |ap1|
     ap1.reviewers.each do |ap2|
       ap2.reviewers.each do |ap3|
         if ap3.reviewers.include?(self)
           self.reviews_by_reviewer(ap1).nil? ? next : s01 = self.reviews_by_reviewer(ap1).get_total_score
           ap1.reviews_by_reviewer(ap2).nil? ? next : s12 = ap1.reviews_by_reviewer(ap2).get_total_score
           ap2.reviews_by_reviewer(ap3).nil? ? next : s23 = ap2.reviews_by_reviewer(ap3).get_total_score
           ap3.reviews_by_reviewer(self).nil? ? next : s30 = ap3.reviews_by_reviewer(self).get_total_score
           cycles.push([[self, s01], [ap1, s12], [ap2, s23], [ap3, s30]])
         end
       end
     end
   end
   cycles
 end

def cycle_similarity_score(cycle)

 def cycle_similarity_score(cycle)
   similarity_score = 0.0
   count = 0.0

   0 ... cycle.size-1.each do |pivot|
     pivot_score = cycle[pivot][1]
     similarity_score = similarity_score + (pivot_score - cycle[other][1]).abs
     count = count + 1.0
   end

   similarity_score = similarity_score / count unless count == 0.0
   similarity_score
 end

def cycle_deviation_score(cycle)

 def cycle_deviation_score(cycle)
   deviation_score = 0.0
   count = 0.0

   0 ... cycle.size.each do |member|
     participant = AssignmentParticipant.find(cycle[member][0].id)
     total_score = participant.get_review_score
     deviation_score = deviation_score + (total_score - cycle[member][1]).abs
     count = count + 1.0
   end

   deviation_score = deviation_score / count unless count == 0.0
   deviation_score
 end

def compute_quiz_scores(scores)

 def compute_quiz_scores(scores)
   total = 0
   if scores[:quiz][:scores][:avg]
     return scores[:quiz][:scores][:avg] * 100  / 100.to_f
   else
     return 0
   end
   return total
 end

def review_response_maps

 def review_response_maps
     participant = Participant.find(id)
     team_id = TeamsUser.team_id(participant.parent_id, participant.user_id)
     ReviewResponseMap.where(reviewee_id: team_id, reviewed_object_id: assignment.id)
 end

def members

 def members
   team.try :participants
 end

def get_hash(time_stamp)

 def get_hash(time_stamp)
   # first generate a hash from the assignment name itself
   hash_data = Digest::SHA1.digest(self.assignment.name.to_s)

   # second generate a hash from the first hash plus the user name and time stamp
   sign = hash_data + self.user.name.to_s + time_stamp.strftime("%Y-%m-%d %H:%M:%S")
   Digest::SHA1.digest(sign)
 end

Review methods

• The bookmark_reviews() method from AssignmentParticipant was being called once in get_assessments_for method of BookmarkRatingQuestionnaire class. It has been replaced by the method statement directly since it only had a single line of code.

Before	After
def bookmark_reviews BookmarkRatingResponseMap.get_assessments_for(self) end	def get_assessments_for(participant) BookmarkRatingResponseMap.get_assessments_for(participant) # participant.bookmark_reviews() end

• The teammate_reviews() method from AssignmentParticipant was being called once in get_assessments_for method of TeammateReviewQuestionnaire class. It has been replaced by the method statement directly since it only had a single line of code.

Before	After
def teammate_reviews TeammateReviewResponseMap.get_assessments_for(self) end	def get_assessments_for(participant) TeammateReviewResponseMap.get_assessments_for(participant) #participant.teammate_reviews() end

Resources

• Expertiza Github repository<ref>Expertiza Github repository https://github.com/expertiza/expertiza</ref>
• Our Github repository<ref>Our Github Repository https://github.com/terryliu1995/expertiza</ref>

References

<references/>