CSC/ECE 517 Spring 2023 - E2336. Reimplement response.rb
Project Overview
There are several issues we plan to address as part of this project.
- Refactor the get_all_review_comments method as an instance method in response.rb file
- Refactor the self.get_all_responses method as an instance method in response_map.rb file.
- Add more descriptive comments for the tests.
Issue 1: Refactor the self.get_all_review_comments method
Currently, the get_all_review_comments method is a class method that returns a data structure containing all the review comments for all responses. This violates the principles of good design as it creates a large data structure that the calling method needs to break apart, leading to poor performance and increased complexity.
To address this issue and come up with a much more elegant design, we plan to refactor as a new implementation that should convert the get_all_review_comments method into an instance method that takes a review object as an argument and returns the comments for that review only.
Issue 2: Refactoring significant_difference?
While the significant_difference method does not necessarily need to be refactored, it is the only method with a call for the avg_scores_and_count_for_prev_reviews method, which does need to be refactored it both is a class method and returns a data structure.
def self.avg_scores_and_count_for_prev_reviews(existing_responses, current_response)
scores_assigned = []
count = 0
existing_responses.each do |existing_response|
unless existing_response.id == current_response.id # the current_response is also in existing_responses array
count += 1
scores_assigned << existing_response.aggregate_questionnaire_score.to_f / existing_response.maximum_score
end
end
[scores_assigned.sum / scores_assigned.size.to_f, count]
end
Because of this we decided that we should simply refactor significant_difference instead and simply remove avg_scores_and_count_for_prev_reviews.
# compare the current response score with other scores on the same artifact, and test if the difference # is significant enough to notify instructor. # Precondition: the response object is associated with a ReviewResponseMap ### "map_class.assessments_for" method need to be refactored def significant_difference? map_class = map.class existing_responses = map_class.assessments_for(map.reviewee) average_score_on_same_artifact_from_others, count = Response.avg_scores_and_count_for_prev_reviews(existing_responses, self) # if this response is the first on this artifact, there's no grade conflict return false if count.zero? # This score has already skipped the unfilled scorable question(s) score = aggregate_questionnaire_score.to_f / maximum_score questionnaire = questionnaire_by_answer(scores.first) assignment = map.assignment assignment_questionnaire = AssignmentQuestionnaire.find_by(assignment_id: assignment.id, questionnaire_id: questionnaire.id) # notification_limit can be specified on 'Rubrics' tab on assignment edit page. allowed_difference_percentage = assignment_questionnaire.notification_limit.to_f # the range of average_score_on_same_artifact_from_others and score is [0,1] # the range of allowed_difference_percentage is [0, 100] (average_score_on_same_artifact_from_others - score).abs * 100 > allowed_difference_percentage end
Issue 3: Make the method in ReviewCommentMixin to be an instance method
Problems
- The ReviewCommentMixin has one single method volume_of_review_comments and it is currently present as a class method. There is no need for it to be a class method and it should instead be an instance method.
- Volume of review comments is a metric, which belongs in metrics.rb, not in a mixin for responses.
Solutions
The change is quite straight forward. We should move this method to be an instance method in metrics.rb file. A side note is that, this method calls the get_all_review_comments method from response.rb. This method will be changed as part of this project (See above points). Any change to that function's method call must be reflected here too.
Issue 4: Simplify + Refactor methods in ScorableMixin
This mixin has methods that are related to scoring. https://expertiza.csc.ncsu.edu/index.php/Scoring_%26_Grading_Methods_(Fall_%2721) is a good resource to understand how scoring and grading works.
Problems
- Not DRY : The following piece of code is used in 2 functions - calculate_total_score and maximum_score
```
scores.each do |s|
question = Question.find(s.question_id)
< Perform a calculation >
end
```
Basically, we iterate through the score and calculate something. The common calculation between both these functions is total_weight calculation.
What problem does this cause? If the logic to calculate the total_weight changes in the future, we will have to make changes to both these places. A developer who is relatively new to the project might miss out one changing it in a location. It is always good to have a single point of change by 'DRY'ing our code.
- Not optimized: Another change related to the above snippet has to do with how the queries are made and the number of queries.
```
scores.each do |s|
question = Question.find(s.question_id)
<Other lines irrelevant to this case >
end
```
What problem does this cause? This is done inside a loop, meaning we perform a select query to each time -> N queries for N objects. Our solution aims to optimize this.
- Checking the class type of an object to perform some business logic
This issue is present in the questionnaire_by_answer method. The following snippet shows it:
```
assignment = if map.is_a? ReviewResponseMap
map.assignment
else
Participant.find(map.reviewer_id).assignment
end
```
Solutions
- The common function would involve calculating the sum of weight.
```
def calculate_total_weight()
scores.each do |s|
question = Question.find(s.question_id)
total_weight += question.weight unless s.answer.nil? || !question.is_a?(ScoredQuestion)
end
```
And this can be called inside the calculate_total_score and maximum_score.
- Instead of calling Question.find inside the loop, we can get the set of question_ids and use a where function.
```
question_ids = scores.map(&:question_id)
questions = Question.where(question_id: question_ids)
< Make the calculation here >
```
This way, we only make one database call instead of N calls.
- To solve checking the object type, we can write a method in ResponseMap and ReviewResponseMap. This way, we can just call that method in the object and depending on what object it is, we would get the corresponding answer.
* In ReviewResponseMap, the method should return it's assignment * In ResponseMap, the method should return Participant.find(map.reviewer_id).assignment
A visual representation of this change would look like this. The snippet uses an 'OR' condition to pick the appropriate assignment. Our change would remove the need for this check.
Issue 5: Refactor the self.get_all_responses method
The self.get_all_responses method which is currently in the response.rb file would be much cleaner as an instance method in response_map.rb. We plan to refactor the self.get_all_responses method in response.rb to an instance method in response_map.rb.
Issue 6: Add more descriptive comments for the tests
The existing tests need more descriptive comments. We plan to add more detailed comments for all the test cases.
Test Plan
As part of this section, we track the set of files that are to be changed for tests and scenarios that are to be tested.
Files
- Issue 3: Write tests for volume_of_review_comments method
- Issue 4: Modify tests in response_spec.rb
Scenarios
- Test the score calculation cases - maximum possible total score for all scores, average score across all of the instances and total score (weighted sum). Ideally average score calculation should cover all the scenarios, that is, if the maximum score is zero, it uses the total score(weighted).