CSC/ECE 517 Spring 2023 - E2336. Reimplement response.rb: Difference between revisions
(Mixin issues) |
mNo edit summary |
||
Line 14: | Line 14: | ||
===Issue 3: Make the method in ReviewCommentMixin to be an instance method === | ===Issue 3: Make the method in ReviewCommentMixin to be an instance method === | ||
====Problems | ====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. | * 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. | * 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. | 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. | ||
Line 26: | Line 26: | ||
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. | 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 | ====Problems==== | ||
* Not DRY : The following piece of code is used in 2 functions - calculate_total_score and maximum_score | * Not DRY : The following piece of code is used in 2 functions - calculate_total_score and maximum_score | ||
Line 60: | Line 60: | ||
``` | ``` | ||
====Solutions | ====Solutions==== | ||
* The common function would involve calculating the sum of weight. | * The common function would involve calculating the sum of weight. | ||
Revision as of 23:47, 7 April 2023
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 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.
``` question = Question.find(s.question_id) ```
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
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.