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 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.

Solution:

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.