CSC/ECE 517 Fall 2017/E1770 Refactor assignment participant.rb: Difference between revisions

From Expertiza_Wiki
Jump to navigation Jump to search
 
(12 intermediate revisions by the same user not shown)
Line 1: Line 1:
'''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>
'''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.
This page provides a brief description of the '''Expertiza''' project. The project is aimed at using TFD(Test First Development) to refactor 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.
 
We used the TDF(Test First Development) method to refactor the AssignmentParticipant model. With the  RSpec tool, we wrote 38 test cases for 24 methods in '''assignment_participant_spec.rb''' first. After finishing these test cases, we executed them, and most of them failed. And then we refactored the methods, '''scores''', '''files''', '''self.import''', '''find_by''', '''where.first''', and executed the test cases again, finally they all passed.


==Introduction to Expertiza==
==Introduction to Expertiza==
Line 48: Line 50:
** Write failing tests first.
** Write failing tests first.


==Test Cases==
== Test Cases ==
Because the project is aimed at using TFD(Test First Development) to refactor the AssignmentParticipant model.
We wrote 38 test cases for 24 methods in '''assignment_participant_spec.rb''' first, with the  RSpec tool.
 
The introduction video to our project<ref>Introduction video to our project https://www.youtube.com/watch?v=sJdmN-S2Voo&feature=youtu.be</ref>
  describe '#dir_path' do
    it 'returns the directory path of current assignment' do
      expect(participant.dir_path).to eq "final_test"
    end
  end
 
  describe '#assign_quiz' do
    it 'creates a new QuizResponseMap record' do
      allow(QuizQuestionnaire).to receive(:find_by).with(instructor_id: 1).and_return(quiz_questionaire) # WHY CAN NOT DELETE THIS SENTENCE
      expect { participant.assign_quiz(participant, participant2, nil) }.to change { QuizResponseMap.count }.from(0).to(1)
      expect(participant.assign_quiz(participant, participant2, nil)).to be_an_instance_of(QuizResponseMap)
    end
  end
 
  describe '#reviewers' do
    it 'returns all the participants in this assignment who have reviewed the team where this participant belongs' do
      allow(ReviewResponseMap).to receive(:where).with(any_args).and_return([response_map]) # differences with .with(parameter)
      allow(AssignmentParticipant).to receive(:find).with(any_args).and_return(participant2)
      expect(participant.reviewers).to eq([participant2])
    end
  end
 
  describe '#review_score' do
    it 'returns the review score' do
      # diao yong de method tai duo le,er qie bu zhi dao sha yi si
      allow(review_questionnaire).to receive(:get_assessments_for).with(any_args).and_return([response])
      allow(review_questionnaire).to receive(:questions).and_return(question)
      allow(Answer).to receive(:compute_scores).with([response], question).and_return(avg: 100)
      allow(review_questionnaire).to receive(:max_possible_score).and_return(100)
      expect(participant.review_score).to eq(100)
    end
  end
 
  describe '#scores' do
    context 'when assignment is not varying rubric by round and not an microtask' do
      it 'calculates scores that this participant has been given' do
        expect(participant).to receive(:assignment_questionnaires)
        expect(assignment).to receive(:compute_total_score)
        allow(assignment).to receive(:varying_rubrics_by_round?).and_return(false)
        allow(assignment).to receive(:is_microtask?).and_return(false)
        expect(assignment).to receive(:compute_total_score)
        expect(participant).to receive(:caculate_scores)
        participant.scores(question)
      end
    end
 
    context 'when assignment is varying rubric by round but not an microtask' do
      it 'calculates scores that this participant has been given' do
        expect(participant).to receive(:assignment_questionnaires)
        expect(assignment).to receive(:compute_total_score)
        allow(assignment).to receive(:varying_rubrics_by_round?).and_return(true)
        expect(participant).to receive(:merge_scores)
        allow(assignment).to receive(:is_microtask?).and_return(false)
        expect(assignment).to receive(:compute_total_score)
        expect(participant).to receive(:caculate_scores)
        participant.scores(question)
      end
    end
 
    context 'when assignment is not varying rubric by round but an microtask' do
      it 'calculates scores that this participant has been given' do
        expect(participant).to receive(:assignment_questionnaires)
        expect(assignment).to receive(:compute_total_score)
        allow(assignment).to receive(:varying_rubrics_by_round?).and_return(false)
        allow(assignment).to receive(:is_microtask?).and_return(true)
        expect(participant).to receive(:topic_total_scores)
        expect(assignment).to receive(:compute_total_score)
        expect(participant).to receive(:caculate_scores)
        participant.scores(question)
      end
    end
  end
 
  # included method
  describe '#assignment_questionnaires' do
    context 'when the round of questionnaire is nil' do
      it 'record the result as review scores' do
        scores = {}
        question_hash = {review: question}
        score_map = {max: 100, min: 100, avg: 100}
        allow(AssignmentQuestionnaire).to receive(:find_by).with(any_args).and_return(assignment_questionnaire)
        allow(review_questionnaire).to receive(:get_assessments_for).with(any_args).and_return([response])
        allow(Answer).to receive(:compute_scores).with(any_args).and_return(score_map)
        participant.assignment_questionnaires(question_hash, scores)
        expect(scores[:review][:assessments]).to eq([response])
        expect(scores[:review][:scores]).to eq(score_map)
      end
    end
 
    context 'when the round of questionnaire is not nil' do
      it 'record the result as review#{n} scores' do
        scores = {}
        question_hash = {review1: question}
        score_map = {max: 100, min: 100, avg: 100}
        allow(AssignmentQuestionnaire).to receive(:find_by).and_return(assignment_questionnaire2)
        allow(review_questionnaire).to receive(:get_assessments_round_for).with(any_args).and_return([response])
        allow(Answer).to receive(:compute_scores).with(any_args).and_return(score_map)
        participant.assignment_questionnaires(question_hash, scores)
        expect(scores[:review1][:assessments]).to eq([response])
        expect(scores[:review1][:scores]).to eq(score_map)
      end
    end
  end
 
  describe '#merge_scores' do
    context 'when all of the review_n are nil' do
      it 'set max, min, avg of review score as 0' do
        scores = {}
        allow(assignment).to receive(:num_review_rounds).and_return(1)
        participant.merge_scores(scores)
        expect(scores[:review][:scores][:max]).to eq(0)
        expect(scores[:review][:scores][:min]).to eq(0)
        expect(scores[:review][:scores][:min]).to eq(0)
      end
    end
 
    context 'when the review_n is not nil' do
      it 'merge the score of review_n to the score of review' do
        score_map = {max: 100, min: 100, avg: 100}
        scores = {review1: {scores: score_map, assessments: [response]}}
        allow(assignment).to receive(:num_review_rounds).and_return(1)
        participant.merge_scores(scores)
        expect(scores[:review][:scores][:max]).to eq(100)
        expect(scores[:review][:scores][:min]).to eq(100)
        expect(scores[:review][:scores][:min]).to eq(100)
      end
    end
  end
 
  describe '#topic_total_scores' do
    it 'set total_score and max_pts_available of score when topic is not nil' do
      scores = {total_score: 100}
      allow(SignUpTopic).to receive(:find_by).with(any_args).and_return(topic)
      participant.topic_total_scores(scores)
      expect(scores[:total_score]).to eq(0)
      expect(scores[:max_pts_available]).to eq(0)
    end
  end
 
  describe '#caculate_scores' do
    context 'when the participant has the grade' do
      it 'his total scores equals his grade' do
        scores = {}
        expect(participant2.caculate_scores(scores)).to eq(100.0)
      end
    end
    context 'when the participant has the grade and the total score more than 100' do
      it 'return the score of a given participant with total score 100' do
        scores = {total_score: 110}
        expect(participant.caculate_scores(scores)).to eq(total_score: 100)
      end
    end
    context 'when the participant has the grade and the total score less than 100' do
      it 'return the score of a given participant with total score' do
        scores = {total_score: 90}
        expect(participant.caculate_scores(scores)).to eq(total_score: 90)
      end
    end
  end
 
  describe '#copy' do
    it 'copies assignment participants to a certain course' do
      expect(participant.copy(517)).to be_an_instance_of(CourseParticipant)
    end
  end
 
  describe '#feedback' do
    it 'returns corrsponding author feedback responses given by current participant' do
      expect(participant.feedback).to eq([response])
    end
  end
 
  describe '#reviews' do
    it 'returns corrsponding peer review responses given by current team' do
      expect(participant.reviews).to eq([response])
    end
  end
 
  describe '#reviews_by_reviewer' do
    it 'returns corrsponding peer review responses given by certain reviewer' do
      allow(ReviewResponseMap).to receive(:get_reviewer_assessments_for).with(team, participant2).and_return([response])
      expect(participant.reviews_by_reviewer(participant2)).to eq([response])
    end
  end
 
  describe '#quizzes_taken' do
    it 'returns corrsponding quiz responses given by current participant' do
      expect(participant.quizzes_taken).to eq([response])
    end
  end
 
  describe '#metareviews' do
    it 'returns corrsponding metareview responses given by current participant' do
      expect(participant.metareviews).to eq([response])
    end
  end
 
  describe '#teammate_reviews' do
    it 'returns corrsponding teammate review responses given by current participant' do
      expect(participant.teammate_reviews).to eq([response])
    end
  end
 
  describe '#bookmark_reviews' do
    it 'returns corrsponding bookmark review responses given by current participant' do
      expect(participant.bookmark_reviews).to eq([response])
    end
  end
 
  describe '#files' do
    context 'when there is not subdirectory in current directory' do
      it 'returns all files in current directory' do
        allow(Dir).to receive(:[]).with("a/*").and_return(["a/k.rb"])
        allow(File).to receive(:directory?).with("a/k.rb").and_return(false)
        expect(participant.files("a")).to eq(["a/k.rb"])
      end
    end
 
    context 'when there is subdirectory in current directory' do
      it 'recursively returns all files in current directory' do
        allow(Dir).to receive(:[]).with("a/*").and_return(["a/b"])
        allow(File).to receive(:directory?).with("a/b").and_return(true)
        allow(Dir).to receive(:[]).with("a/b/*").and_return(["a/b/k.rb"])
        allow(File).to receive(:directory?).with("a/b/k.rb").and_return(false)
        expect(participant.files("a")).to eq(["a/b/k.rb", "a/b"])
      end
    end
  end
 
  describe ".import" do
    context 'when record is empty' do
      it 'raises an ArgumentError' do
        row = []
        allow(AssignmentParticipant).to receive(:check_info_and_create).with(any_args).and_raise("No user id has been specified.")
        expect(AssignmentParticipant).to receive(:check_info_and_create)
        expect { AssignmentParticipant.import(row, nil, nil, nil) }.to raise_error("No user id has been specified.")
      end
    end
 
    context 'when no user is found by offered username' do
      context 'when the record has less than 4 items' do
        it 'raises an ArgumentError' do
          row = ["user_name", "user_fullname", "name@email.com"]
          allow(AssignmentParticipant).
            to receive(:check_info_and_create).with(any_args).and_raise("The record containing #{row[0]} does not have enough items.")
          expect(AssignmentParticipant).to receive(:check_info_and_create)
          expect { AssignmentParticipant.import(row, nil, nil, nil) }.to raise_error("The record containing #{row[0]} does not have enough items.")
        end
      end
 
      context 'when the record has more than 4 items' do
        context 'when certain assignment cannot be found' do
          it 'creates a new user based on import information and raises an ImportError' do
            row = ["user_name", "user_fullname", "name@email.com", "user_role_name", "user_parent_name"]
            session = {user: participant}
            allow(User).to receive(:find_by).with(any_args).and_return(nil)
            allow(Assignment).to receive(:find).with(2).and_return(nil)
            expect { AssignmentParticipant.import(row, nil, session, 2) }.
              to raise_error("The assignment with id \"2\" was not found.").and change { User.count }.by(1)
          end
        end
 
        context 'when certain assignment can be found and assignment participant does not exists' do
          it 'creates a new user, new participant and raises an ImportError' do
            row = ["user_name", "user_fullname", "name@email.com", "user_role_name", "user_parent_name"]
            session = {user: participant}
            allow(User).to receive(:find_by_name).with(any_args).and_return(nil)
            allow(Assignment).to receive(:find).with(2).and_return(assignment)
            allow(AssignmentParticipant).to receive(:exists?).and_return(false)
            expect { AssignmentParticipant.import(row, nil, session, 2) }.to change { User.count }.by(1).and change { AssignmentParticipant.count }.by(1)
          end
        end
      end
    end
  end
 
  describe '.export' do
    it 'exports all participants in current assignment' do
      csv = []
      expect(AssignmentParticipant).to receive_message_chain(:where, :find_each).with(any_args).and_yield(participant)
      expect(AssignmentParticipant.export(csv, 1, any_args)).
        to eq([["student2064", "2064, student", "expertiza@mailinator.com", "Student", "instructor6", true, true, true, "handle"]])
    end
  end
 
  describe '#set_handle' do
    context 'when the user of current participant does not have handle' do
      it 'sets the user name as the handle of current participant' do
        allow(student).to receive_message_chain(:handle, :nil?).and_return(true)
        allow(student).to receive(:handle).and_return("")
        participant.set_handle
        expect(participant.handle).to eq("student2064")
      end
    end
 
    context 'when current assignment exists participants with same handle as the one of current user' do
      it 'sets the user name as the name of current participant' do
        allow(AssignmentParticipant).to receive(:exists?).with(any_args).and_return(true)
        participant.set_handle
        expect(participant.handle).to eq("student2064")
      end
    end
 
    context 'when current assignment does not have participants with same handle as the one of current user' do
      it 'sets the user name as the handle of current participant' do
        participant.set_handle
        expect(participant.handle).to eq("handle")
      end
    end
  end
 
  describe '#review_file_path' do
    it 'returns the file path for reviewer to upload files during peer review' do
      allow(ResponseMap).to receive(:find).with(any_args).and_return(response_map)
      allow(TeamsUser).to receive_message_chain(:find_by, :user_id).with(any_args).and_return(1)
      allow(Participant).to receive(:find_by).with(any_args).and_return(participant)
      file_path = Rails.root.to_s + "/pg_data/instructor6/csc517/test/final_test/0_review/1"
      expect(participant.review_file_path(1)).to eq(file_path)
    end
  end
 
  describe '#current_stage' do
    it 'returns stage of current assignment' do
      allow(assignment).to receive(:get_current_stage).with(1).and_return("Finished")
      expect(participant.current_stage).to eq("Finished")
    end
  end
 
  describe '#stage_deadline' do
    context 'when stage of current assignment is not Finished' do
      it 'returns current stage' do
        allow(assignment).to receive(:stage_deadline).with(1).and_return("Unknow")
        expect(participant.stage_deadline).to eq("Unknow")
      end
    end
 
    context 'when stage of current assignment not Finished' do
      context 'current assignment is not a staggered deadline assignment' do
        it 'returns the due date of current assignment' do
          allow(assignment).to receive(:stage_deadline).with(1).and_return("Finished")
          allow(assignment).to receive(:staggered_deadline?).and_return(false)
          allow(assignment).to receive_message_chain(:due_dates, :last, :due_at).and_return(1)
          expect(participant.stage_deadline).to eq("1")
        end
      end
 
      context 'current assignment is a staggered deadline assignment' do
        it 'returns the due date of current topic' do
          allow(assignment).to receive(:stage_deadline).with(1).and_return("Finished")
          allow(assignment).to receive(:staggered_deadline?).and_return(true)
          allow(TopicDueDate).to receive_message_chain(:find_by, :last, :due_at).and_return(1)
          expect(participant.stage_deadline).to eq("1")
        end
      end
    end
  end


== Refactoring ==
== Refactoring ==
Line 272: Line 634:
     end
     end
     files
     files
  end
===Self.import  method===
The self.import method of AssignmentParticipant model is exactly the same as '''course_participant.rb, L21''', so we move it to a module named Class_method in '''lib/TFD1770_refactor.rb'''.
The below two lines are added to the AssignmentParticipant model and CourseParticipant model.
require 'TFD1770_refactor'
include Class_method
  def self.import(row, _row_header = nil, session, id)
    user = AssignmentParticipant.check_info_and_create(row, _row_header = nil, session)
    raise ImportError, "The assignment with id \"" + id.to_s + "\" was not found." if Assignment.find(id).nil?
    unless AssignmentParticipant.exists?(user_id: user.id, parent_id: id)
      new_part = AssignmentParticipant.create(user_id: user.id, parent_id: id)
      new_part.set_handle
    end
  end
* module Class_method
  def check_info_and_create(row, _row_header = nil, session)
    raise ArgumentError, "No user id has been specified." if row.empty?
    user = User.find_by(name: row[0])
    if user.nil?
      raise ArgumentError, "The record containing #{row[0]} does not have enough items." if row.length < 4
      attributes = ImportFileHelper.define_attributes(row)
      user = ImportFileHelper.create_new_user(attributes, session)
    end
   end
   end


Line 328: Line 718:
==Resources==
==Resources==
* Expertiza Github repository<ref>Expertiza Github repository https://github.com/expertiza/expertiza</ref>
* 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>
* Our Github repository<ref>Our Github repository https://github.com/terryliu1995/expertiza</ref>
* Our pull request<ref>Our pull request https://github.com/expertiza/expertiza/pull/1049</ref>


== References ==
== References ==
<references/>
<references/>

Latest revision as of 12:52, 3 November 2017

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 using TFD(Test First Development) to refactor 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.

We used the TDF(Test First Development) method to refactor the AssignmentParticipant model. With the RSpec tool, we wrote 38 test cases for 24 methods in assignment_participant_spec.rb first. After finishing these test cases, we executed them, and most of them failed. And then we refactored the methods, scores, files, self.import, find_by, where.first, and executed the test cases again, finally they all passed.

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.

Why Test-driven development?

Test-driven development (TDD)<ref>Test-driven development (TDD) 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. The followings are several benefits of Test-driven development (TDD).

  • Maintainable, Flexible, Easily Extensible.
  • Unparalleled Test Coverage & Streamlined Codebase.
  • Clean Interface.
  • Refactoring Encourages Improvements.
  • Executable Documentation.

Introduction to RSpec

RSpec<ref>RSpec https://en.wikipedia.org/wiki/RSpec</ref> is a 'Domain Specific Language' (DSL) testing tool written in Ruby to test Ruby code. It is a behavior-driven development (BDD) framework which is extensively used in the production applications. The basic idea behind this concept is that of Test Driven Development(TDD) where the tests are written first and the development is based on writing just enough code that will fulfill those tests followed by refactoring. It contains its own mocking framework that is fully integrated into the framework based upon JMock. The simplicity in the RSpec syntax makes it one of the popular testing tools for Ruby applications. The RSpec tool can be used by installing the rspec gem which consists of 3 other gems namely rspec-core , rspec-expectation and rspec-mock.

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.

Test Cases

Because the project is aimed at using TFD(Test First Development) to refactor the AssignmentParticipant model. We wrote 38 test cases for 24 methods in assignment_participant_spec.rb first, with the RSpec tool.

The introduction video to our project<ref>Introduction video to our project https://www.youtube.com/watch?v=sJdmN-S2Voo&feature=youtu.be</ref>

 describe '#dir_path' do
   it 'returns the directory path of current assignment' do
     expect(participant.dir_path).to eq "final_test"
   end
 end
 describe '#assign_quiz' do
   it 'creates a new QuizResponseMap record' do
     allow(QuizQuestionnaire).to receive(:find_by).with(instructor_id: 1).and_return(quiz_questionaire) # WHY CAN NOT DELETE THIS SENTENCE
     expect { participant.assign_quiz(participant, participant2, nil) }.to change { QuizResponseMap.count }.from(0).to(1)
     expect(participant.assign_quiz(participant, participant2, nil)).to be_an_instance_of(QuizResponseMap)
   end
 end
 describe '#reviewers' do
   it 'returns all the participants in this assignment who have reviewed the team where this participant belongs' do
     allow(ReviewResponseMap).to receive(:where).with(any_args).and_return([response_map]) # differences with .with(parameter)
     allow(AssignmentParticipant).to receive(:find).with(any_args).and_return(participant2)
     expect(participant.reviewers).to eq([participant2])
   end
 end
 describe '#review_score' do
   it 'returns the review score' do
     # diao yong de method tai duo le,er qie bu zhi dao sha yi si
     allow(review_questionnaire).to receive(:get_assessments_for).with(any_args).and_return([response])
     allow(review_questionnaire).to receive(:questions).and_return(question)
     allow(Answer).to receive(:compute_scores).with([response], question).and_return(avg: 100)
     allow(review_questionnaire).to receive(:max_possible_score).and_return(100)
     expect(participant.review_score).to eq(100)
   end
 end
 describe '#scores' do
   context 'when assignment is not varying rubric by round and not an microtask' do
     it 'calculates scores that this participant has been given' do
       expect(participant).to receive(:assignment_questionnaires)
       expect(assignment).to receive(:compute_total_score)
       allow(assignment).to receive(:varying_rubrics_by_round?).and_return(false)
       allow(assignment).to receive(:is_microtask?).and_return(false)
       expect(assignment).to receive(:compute_total_score)
       expect(participant).to receive(:caculate_scores)
       participant.scores(question)
     end
   end
   context 'when assignment is varying rubric by round but not an microtask' do
     it 'calculates scores that this participant has been given' do
       expect(participant).to receive(:assignment_questionnaires)
       expect(assignment).to receive(:compute_total_score)
       allow(assignment).to receive(:varying_rubrics_by_round?).and_return(true)
       expect(participant).to receive(:merge_scores)
       allow(assignment).to receive(:is_microtask?).and_return(false)
       expect(assignment).to receive(:compute_total_score)
       expect(participant).to receive(:caculate_scores)
       participant.scores(question)
     end
   end
   context 'when assignment is not varying rubric by round but an microtask' do
     it 'calculates scores that this participant has been given' do
       expect(participant).to receive(:assignment_questionnaires)
       expect(assignment).to receive(:compute_total_score)
       allow(assignment).to receive(:varying_rubrics_by_round?).and_return(false)
       allow(assignment).to receive(:is_microtask?).and_return(true)
       expect(participant).to receive(:topic_total_scores)
       expect(assignment).to receive(:compute_total_score)
       expect(participant).to receive(:caculate_scores)
       participant.scores(question)
     end
   end
 end
 # included method
 describe '#assignment_questionnaires' do
   context 'when the round of questionnaire is nil' do
     it 'record the result as review scores' do
       scores = {}
       question_hash = {review: question}
       score_map = {max: 100, min: 100, avg: 100}
       allow(AssignmentQuestionnaire).to receive(:find_by).with(any_args).and_return(assignment_questionnaire)
       allow(review_questionnaire).to receive(:get_assessments_for).with(any_args).and_return([response])
       allow(Answer).to receive(:compute_scores).with(any_args).and_return(score_map)
       participant.assignment_questionnaires(question_hash, scores)
       expect(scores[:review][:assessments]).to eq([response])
       expect(scores[:review][:scores]).to eq(score_map)
     end
   end
   context 'when the round of questionnaire is not nil' do
     it 'record the result as review#{n} scores' do
       scores = {}
       question_hash = {review1: question}
       score_map = {max: 100, min: 100, avg: 100}
       allow(AssignmentQuestionnaire).to receive(:find_by).and_return(assignment_questionnaire2)
       allow(review_questionnaire).to receive(:get_assessments_round_for).with(any_args).and_return([response])
       allow(Answer).to receive(:compute_scores).with(any_args).and_return(score_map)
       participant.assignment_questionnaires(question_hash, scores)
       expect(scores[:review1][:assessments]).to eq([response])
       expect(scores[:review1][:scores]).to eq(score_map)
     end
   end
 end
 describe '#merge_scores' do
   context 'when all of the review_n are nil' do
     it 'set max, min, avg of review score as 0' do
       scores = {}
       allow(assignment).to receive(:num_review_rounds).and_return(1)
       participant.merge_scores(scores)
       expect(scores[:review][:scores][:max]).to eq(0)
       expect(scores[:review][:scores][:min]).to eq(0)
       expect(scores[:review][:scores][:min]).to eq(0)
     end
   end
   context 'when the review_n is not nil' do
     it 'merge the score of review_n to the score of review' do
       score_map = {max: 100, min: 100, avg: 100}
       scores = {review1: {scores: score_map, assessments: [response]}}
       allow(assignment).to receive(:num_review_rounds).and_return(1)
       participant.merge_scores(scores)
       expect(scores[:review][:scores][:max]).to eq(100)
       expect(scores[:review][:scores][:min]).to eq(100)
       expect(scores[:review][:scores][:min]).to eq(100)
     end
   end
 end
 describe '#topic_total_scores' do
   it 'set total_score and max_pts_available of score when topic is not nil' do
     scores = {total_score: 100}
     allow(SignUpTopic).to receive(:find_by).with(any_args).and_return(topic)
     participant.topic_total_scores(scores)
     expect(scores[:total_score]).to eq(0)
     expect(scores[:max_pts_available]).to eq(0)
   end
 end
 describe '#caculate_scores' do
   context 'when the participant has the grade' do
     it 'his total scores equals his grade' do
       scores = {}
       expect(participant2.caculate_scores(scores)).to eq(100.0)
     end
   end
   context 'when the participant has the grade and the total score more than 100' do
     it 'return the score of a given participant with total score 100' do
       scores = {total_score: 110}
       expect(participant.caculate_scores(scores)).to eq(total_score: 100)
     end
   end
   context 'when the participant has the grade and the total score less than 100' do
     it 'return the score of a given participant with total score' do
       scores = {total_score: 90}
       expect(participant.caculate_scores(scores)).to eq(total_score: 90)
     end
   end
 end
 describe '#copy' do
   it 'copies assignment participants to a certain course' do
     expect(participant.copy(517)).to be_an_instance_of(CourseParticipant)
   end
 end
 describe '#feedback' do
   it 'returns corrsponding author feedback responses given by current participant' do
     expect(participant.feedback).to eq([response])
   end
 end
 describe '#reviews' do
   it 'returns corrsponding peer review responses given by current team' do
     expect(participant.reviews).to eq([response])
   end
 end
 describe '#reviews_by_reviewer' do
   it 'returns corrsponding peer review responses given by certain reviewer' do
     allow(ReviewResponseMap).to receive(:get_reviewer_assessments_for).with(team, participant2).and_return([response])
     expect(participant.reviews_by_reviewer(participant2)).to eq([response])
   end
 end
 describe '#quizzes_taken' do
   it 'returns corrsponding quiz responses given by current participant' do
     expect(participant.quizzes_taken).to eq([response])
   end
 end
 describe '#metareviews' do
   it 'returns corrsponding metareview responses given by current participant' do
     expect(participant.metareviews).to eq([response])
   end
 end
 describe '#teammate_reviews' do
   it 'returns corrsponding teammate review responses given by current participant' do
     expect(participant.teammate_reviews).to eq([response])
   end
 end
 describe '#bookmark_reviews' do
   it 'returns corrsponding bookmark review responses given by current participant' do
     expect(participant.bookmark_reviews).to eq([response])
   end
 end
 describe '#files' do
   context 'when there is not subdirectory in current directory' do
     it 'returns all files in current directory' do
       allow(Dir).to receive(:[]).with("a/*").and_return(["a/k.rb"])
       allow(File).to receive(:directory?).with("a/k.rb").and_return(false)
       expect(participant.files("a")).to eq(["a/k.rb"])
     end
   end
   context 'when there is subdirectory in current directory' do
     it 'recursively returns all files in current directory' do
       allow(Dir).to receive(:[]).with("a/*").and_return(["a/b"])
       allow(File).to receive(:directory?).with("a/b").and_return(true)
       allow(Dir).to receive(:[]).with("a/b/*").and_return(["a/b/k.rb"])
       allow(File).to receive(:directory?).with("a/b/k.rb").and_return(false)
       expect(participant.files("a")).to eq(["a/b/k.rb", "a/b"])
     end
   end
 end
 describe ".import" do
   context 'when record is empty' do
     it 'raises an ArgumentError' do
       row = []
       allow(AssignmentParticipant).to receive(:check_info_and_create).with(any_args).and_raise("No user id has been specified.")
       expect(AssignmentParticipant).to receive(:check_info_and_create)
       expect { AssignmentParticipant.import(row, nil, nil, nil) }.to raise_error("No user id has been specified.")
     end
   end
   context 'when no user is found by offered username' do
     context 'when the record has less than 4 items' do
       it 'raises an ArgumentError' do
         row = ["user_name", "user_fullname", "name@email.com"]
         allow(AssignmentParticipant).
           to receive(:check_info_and_create).with(any_args).and_raise("The record containing #{row[0]} does not have enough items.")
         expect(AssignmentParticipant).to receive(:check_info_and_create)
         expect { AssignmentParticipant.import(row, nil, nil, nil) }.to raise_error("The record containing #{row[0]} does not have enough items.")
       end
     end
     context 'when the record has more than 4 items' do
       context 'when certain assignment cannot be found' do
         it 'creates a new user based on import information and raises an ImportError' do
           row = ["user_name", "user_fullname", "name@email.com", "user_role_name", "user_parent_name"]
           session = {user: participant}
           allow(User).to receive(:find_by).with(any_args).and_return(nil)
           allow(Assignment).to receive(:find).with(2).and_return(nil)
           expect { AssignmentParticipant.import(row, nil, session, 2) }.
             to raise_error("The assignment with id \"2\" was not found.").and change { User.count }.by(1)
         end
       end
       context 'when certain assignment can be found and assignment participant does not exists' do
         it 'creates a new user, new participant and raises an ImportError' do
           row = ["user_name", "user_fullname", "name@email.com", "user_role_name", "user_parent_name"]
           session = {user: participant}
           allow(User).to receive(:find_by_name).with(any_args).and_return(nil)
           allow(Assignment).to receive(:find).with(2).and_return(assignment)
           allow(AssignmentParticipant).to receive(:exists?).and_return(false)
           expect { AssignmentParticipant.import(row, nil, session, 2) }.to change { User.count }.by(1).and change { AssignmentParticipant.count }.by(1)
         end
       end
     end
   end
 end
 describe '.export' do
   it 'exports all participants in current assignment' do
     csv = []
     expect(AssignmentParticipant).to receive_message_chain(:where, :find_each).with(any_args).and_yield(participant)
     expect(AssignmentParticipant.export(csv, 1, any_args)).
       to eq("student2064", "2064, student", "expertiza@mailinator.com", "Student", "instructor6", true, true, true, "handle")
   end
 end
 describe '#set_handle' do
   context 'when the user of current participant does not have handle' do
     it 'sets the user name as the handle of current participant' do
       allow(student).to receive_message_chain(:handle, :nil?).and_return(true)
       allow(student).to receive(:handle).and_return("")
       participant.set_handle
       expect(participant.handle).to eq("student2064")
     end
   end
   context 'when current assignment exists participants with same handle as the one of current user' do
     it 'sets the user name as the name of current participant' do
       allow(AssignmentParticipant).to receive(:exists?).with(any_args).and_return(true)
       participant.set_handle
       expect(participant.handle).to eq("student2064")
     end
   end
   context 'when current assignment does not have participants with same handle as the one of current user' do
     it 'sets the user name as the handle of current participant' do
       participant.set_handle
       expect(participant.handle).to eq("handle")
     end
   end
 end
 describe '#review_file_path' do
   it 'returns the file path for reviewer to upload files during peer review' do
     allow(ResponseMap).to receive(:find).with(any_args).and_return(response_map)
     allow(TeamsUser).to receive_message_chain(:find_by, :user_id).with(any_args).and_return(1)
     allow(Participant).to receive(:find_by).with(any_args).and_return(participant)
     file_path = Rails.root.to_s + "/pg_data/instructor6/csc517/test/final_test/0_review/1"
     expect(participant.review_file_path(1)).to eq(file_path)
   end
 end
 describe '#current_stage' do
   it 'returns stage of current assignment' do
     allow(assignment).to receive(:get_current_stage).with(1).and_return("Finished")
     expect(participant.current_stage).to eq("Finished")
   end
 end
 describe '#stage_deadline' do
   context 'when stage of current assignment is not Finished' do
     it 'returns current stage' do
       allow(assignment).to receive(:stage_deadline).with(1).and_return("Unknow")
       expect(participant.stage_deadline).to eq("Unknow")
     end
   end
   context 'when stage of current assignment not Finished' do
     context 'current assignment is not a staggered deadline assignment' do
       it 'returns the due date of current assignment' do
         allow(assignment).to receive(:stage_deadline).with(1).and_return("Finished")
         allow(assignment).to receive(:staggered_deadline?).and_return(false)
         allow(assignment).to receive_message_chain(:due_dates, :last, :due_at).and_return(1)
         expect(participant.stage_deadline).to eq("1")
       end
     end
     context 'current assignment is a staggered deadline assignment' do
       it 'returns the due date of current topic' do
         allow(assignment).to receive(:stage_deadline).with(1).and_return("Finished")
         allow(assignment).to receive(:staggered_deadline?).and_return(true)
         allow(TopicDueDate).to receive_message_chain(:find_by, :last, :due_at).and_return(1)
         expect(participant.stage_deadline).to eq("1")
       end
     end
   end
 end

Refactoring

Scores method

The method scores has been converted to smaller methods scores, assignment_questionnaires, merge_scores, topic_total_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: self.assignment.id, questionnaire_id: questionnaire.id).used_in_round
     # create symbol for "varying rubrics" feature -Yang
     questionnaire_symbol = if round.nil?
                              questionnaire.symbol
                            else
                              (questionnaire.symbol.to_s + round.to_s).to_sym
                            end
     scores[questionnaire_symbol] = {}
     scores[questionnaire_symbol][:assessments] = if round.nil?
                                                    questionnaire.get_assessments_for(self)
                                                  else
                                                    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] = {}
     scores[review_sym][:assessments] = []
     scores[review_sym][:scores] = {}
     scores[review_sym][:scores][:max] = -999_999_999
     scores[review_sym][:scores][:min] = 999_999_999
     scores[review_sym][:scores][:avg] = 0
     total_score = 0
     for i in 1..self.assignment.num_review_rounds
       round_sym = ("review" + i.to_s).to_sym
       if scores[round_sym].nil? || scores[round_sym][:assessments].nil? || scores[round_sym][:assessments].empty?
         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
       unless scores[round_sym][:scores][:avg].nil?
         total_score += scores[round_sym][:scores][:avg] * length_of_assessments
       end
     end
     if scores[review_sym][:scores][:max] == -999_999_999 && scores[review_sym][:scores][:min] == 999_999_999
       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)
     unless 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 = []
   # quiz_response_mappings = QuizResponseMap.where(reviewer_id: self.id)
   # quiz_response_mappings.each do |qmapping|
   #   quiz_responses << qmapping.response if qmapping.response
   # 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
     scores[:total_score] = 100 if scores[:total_score] > 100
     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[review#] (for each round) to score[review]  -Yang
   merge_scores(scores) if self.assignment.varying_rubrics_by_round?
   # 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
   topic_total_scores(scores) if self.assignment.is_microtask?
   # for all quiz questionnaires (quizzes) taken by the participant
   # quiz_responses = []
   # quiz_response_mappings = QuizResponseMap.where(reviewer_id: self.id)
   # quiz_response_mappings.each do |qmapping|
   #   quiz_responses << qmapping.response if qmapping.response
   # 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
   caculate_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: self.assignment.id, questionnaire_id: questionnaire.id).used_in_round
     # create symbol for "varying rubrics" feature -Yang
     questionnaire_symbol = if round.nil?
                              questionnaire.symbol
                            else
                              (questionnaire.symbol.to_s + round.to_s).to_sym
                            end
     scores[questionnaire_symbol] = {}
     scores[questionnaire_symbol][:assessments] = if round.nil?
                                                    questionnaire.get_assessments_for(self)
                                                  else
                                                    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)
   review_sym = "review".to_sym
   scores[review_sym] = {}
   scores[review_sym][:assessments] = []
   scores[review_sym][:scores] = {max: -999_999_999, min: 999_999_999, avg: 0}
   total_score = 0
   for i in 1..self.assignment.num_review_rounds
     round_sym = ("review" + i.to_s).to_sym
     if scores[round_sym].nil? || scores[round_sym][:assessments].nil? || scores[round_sym][:assessments].empty?
       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
     unless scores[round_sym][:scores][:avg].nil?
       total_score += scores[round_sym][:scores][:avg] * length_of_assessments
     end
   end
   if scores[review_sym][:scores][:max] == -999_999_999 && scores[review_sym][:scores][:min] == 999_999_999
     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
  • topic_total_scores(scores) method
 def topic_total_scores(scores)
   topic = SignUpTopic.find_by(assignment_id: self.assignment.id)
   unless topic.nil?
     scores[:total_score] *= (topic.micropayment.to_f / 100.to_f)
     scores[:max_pts_available] = topic.micropayment
   end
 end
  • calculate_scores(scores) method
 def caculate_scores(scores)
   if self.grade
     scores[:total_score] = self.grade
   else
     scores[:total_score] = 100 if scores[:total_score] > 100
     scores
   end
 end

Files method

The files method of AssignmentParticipant model is exactly the same as assignment_team.rb, L103, so we move it to a module named Instance_method in lib/TFD1770_refactor.rb. The below two lines are added to the AssignmentParticipant model and AssignmentTeam model.

require 'TFD1770_refactor'

include Instance_method

  • module Instance_method
 def files(directory)
   files_list = Dir[directory + "/*"]
   files = []
   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

Self.import method

The self.import method of AssignmentParticipant model is exactly the same as course_participant.rb, L21, so we move it to a module named Class_method in lib/TFD1770_refactor.rb. The below two lines are added to the AssignmentParticipant model and CourseParticipant model.

require 'TFD1770_refactor'

include Class_method

 def self.import(row, _row_header = nil, session, id)
   user = AssignmentParticipant.check_info_and_create(row, _row_header = nil, session)
   raise ImportError, "The assignment with id \"" + id.to_s + "\" was not found." if Assignment.find(id).nil?
   unless AssignmentParticipant.exists?(user_id: user.id, parent_id: id)
     new_part = AssignmentParticipant.create(user_id: user.id, parent_id: id)
     new_part.set_handle
   end
 end
  • module Class_method
 def check_info_and_create(row, _row_header = nil, session)
   raise ArgumentError, "No user id has been specified." if row.empty?
   user = User.find_by(name: row[0])
   if user.nil?
     raise ArgumentError, "The record containing #{row[0]} does not have enough items." if row.length < 4
     attributes = ImportFileHelper.define_attributes(row)
     user = ImportFileHelper.create_new_user(attributes, session)
   end
 end

Find_by method

Use find_by instead of dynamic method

Before After
  • assignment_particpant.rb L32

quiz = QuizQuestionnaire.find_by_instructor_id(contributor.id)

  • assignment_particpant.rb L113

topic = SignUpTopic.find_by_assignment_id(assignment.id)

  • assignment_particpant.rb L196

user = User.find_by_name(row[0])

  • assignment_particpant.rb L34

quiz = QuizQuestionnaire.find_by(instructor_id: contributor.id)

  • assignment_particpant.rb L134

topic = SignUpTopic.find_by(assignment_id: self.assignment.id)

  • TFD1770_refactor.rb L20

user = User.find_by(name:row[0])

Use find_by instead of where.first

Before After
  • assignment_particpant.rb L267

first_user_id = TeamsUser.where(team_id: response_map.reviewee_id).first.user_id

  • assignment_particpant.rb L267

participant = Participant.where(parent_id: response_map.reviewed_object_id, user_id: first_user_id).first

  • assignment_particpant.rb L258

first_user_id = TeamsUser.find_by(team_id: response_map.reviewee_id).user_id

  • assignment_particpant.rb L259

participant = Participant.find_by(parent_id: response_map.reviewed_object_id, user_id: first_user_id)

Resources

References

<references/>