CSC/ECE 517 Fall 2024 - E2451. Reimplement feedback response map.rb

This page contains the details of OSS project E2451: Reimplement feedback response map done for Expertiza in Fall 2024.

Expertiza and Project Overview

Background

Expertiza is an open-source application used in CSC 517 (Object Oriented Design and Development) as a platform for creating teams, assigning programs, and reviewing submissions, among other tasks. Expertiza is also used to help students develop a working application in order to mimic how students will work on programs in the workplace.

Project Overview

The goal of this project is to refactor the feedback_response_map.rb file from the Expertiza repository and integrate it into the reimplementation-back-end repository. Within Expertiza, when a user submits a review, an instance of the Response class is created, associated with a ResponseMap. Each ResponseMap links a reviewer (reviewer_id), reviewee (reviewee_id), and the reviewed item (reviewed_object_id). FeedbackResponseMap, a subclass of ResponseMap, represents the feedback provided by a reviewee on the feedback received for their submission.

This class has significant room for improvement, especially with respect to object-oriented principles. Refactoring feedback_response_map.rb will streamline functionality, enhance maintainability, and improve adherence to coding standards.

Current Implementation

The current feedback_response_map.rb in the Expertiza repository has several issues. It includes the following problems:

• Poor adherence to SOLID and DRY principles: The current code structure introduces unnecessary repetition and complexity.
• feedback_response_report Method Issues: This critical method is highly complex, relying on custom data structures and separate containers for each review round, resulting in a DRY violation.
• Improper Responsibility Assignment: The email method does not belong within this class and would benefit from refactoring out into a separate class using the Visitor pattern. This pattern is suggested to streamline functionality, though alternative design patterns may be considered if more suitable.
• Memory Inefficiencies: The method currently creates unnecessary memory through inefficient structures, including multiple separate arrays for different rounds of review, which could be simplified into a 2D array structure.
• Code Smells: The code has redundant methods, unclear variable names, inefficient loops, and overly long method names.

Objectives & Requirements

1. Refactor the feedback_response_report Method

• Address the DRY violation by consolidating the containers for review rounds.
• Generalize the code to handle both cases—when rubrics vary by round and when they do not—without duplicating code.
• Introduce efficient data structures, like a 2D array, to replace multiple arrays and reduce memory usage.

2. Separate Responsibilities Using Visitor Pattern (stretch goal)

• Extract the email method using the Visitor pattern to modularize email functionality. Alternatively, select a design pattern better suited to organizing this method if available. Note that the email method is not properly used in its current location and is better suited to the Response class rather than ResponseMap.

3. Eliminate Redundant Methods

• Remove methods that do not serve a clear purpose and improve method naming for clarity.
• Aim to address any redundant meta-review functionality, as indicated by the professor's preference to remove these from response_map.rb.

4. Refactor for Code Clarity and Efficiency

• Improve variable names, reduce unnecessary loops, and replace long method names with concise, intuitive alternatives.
• Ensure that each method has a single, clearly defined purpose, improving code readability.

5. Testing and Documentation

• Write comprehensive tests for all newly implemented and modified methods to ensure reliability.
• Add detailed comments to facilitate code maintenance and understanding for future developers.

Design

Design Principles

• Single Responsibility Principle (SRP): Each method and class should have a single, well-defined purpose.
• Don’t Repeat Yourself (DRY): Eliminate redundancy by creating general solutions that handle multiple scenarios.
• Ruby Readability Guidelines (Week 9 Notes):
  • Guideline: Variable names should be neither too short nor too long.
  • Guideline: Names should not be redundant.
  • Guideline: A method should do only one thing and do it well.
• Visitor Pattern: Utilize this pattern to separate emailing functionalities from the main class.

Implementation Plan

The refactoring will proceed as follows:

1. Analyze and Refactor Current Code Structure

• Review and understand the existing structure and dependencies within feedback_response_map.rb.
• Identify areas of redundancy, complexity, and inefficiency, particularly in feedback_response_report, to create a clear plan for refactoring.
• Try to reduce character count and improve readability with aliasing (this way we do not break any code that relies on being able to call specific methods).

2. Refactor the feedback_response_report Method

• Consolidate containers for each review round to address DRY violations. By using a single structure to store review data, we will eliminate hard-coded repetition.
• Generalize logic to accommodate both scenarios where rubrics vary by round and where they do not, without duplicating code paths.
• Replace inefficient structures, such as multiple arrays, with more flexible and memory-efficient options, like a 2D array or a hash, to reduce memory usage and improve scalability.

3. Introduce Helper Methods for Reusable Logic (for feedback_response_report)

• Extract frequently used or complex code blocks into helper methods, reducing the size of primary methods and enhancing reusability.
• Use helper methods for specific actions, like handling rounds or retrieving assignment data, to make the code cleaner and better organized.
• Emphasis on adhering to the guideline: A method should do only one thing and do it well.

4. Research Future Refactoring for email Method

• The email method is convoluted and the functionality should be refactored into a different class in the future.
• This is because this visitor pattern violation is present in many classes, and is beyond the scope of a one-class re-implementation.

5. Improve Variable Naming and Structure

• Replace ambiguous variable names with clearer alternatives that convey purpose, such as renaming @temp_review_responses and @temp_response_map_ids to more meaningful names.
• Simplify long method names to make the code more readable and approachable for new contributors.

Implementation

1. Analyze and Refactor Current Code Structure

Before refactoring, the FeedbackResponseMap class contains 6 methods, with the majority of the code in self.feedback_response_report:

We aim to change that by splitting up this method into three helper methods, as seen in the following:

To go into more detail, here is the code for the feedback_response_report method, and how we intend to split it up:

The red section of code will be self.get_feedback_authors, yellow will be self.varying_rubrics_report, and green will be self.static_rubrics_report. Each separated group will be its own method, and self.varying_rubrics_report and self.static_rubrics_report (the yellow and green methods) will be returned at the end of the new feedback_response_report.

2. Refactor the feedback_response_report Method

The following are the original arrays used to statically represent the review rounds.

We can take these multiple arrays, and use dictionaries instead, where each round is called by its key value (integer value).

The code found in the feedback_response_report method that use the arrays were moved to the two helper methods, seen as follows:

Additionally, Rubocop was used to analyze the code and adhere to proper Ruby Guidelines. The command below was used to search for any code smells that we missed.

  rubocop app/models/feedback_response_map.rb

Originally, there were only three offenses, but this largely grew as we continued to refactor the code. All of the offenses can be seen below:

All of these offenses were fixed. The following three offenses were the original ones. For the first and third offenses, the colon after the method headers were simply removed. For the second offense, the “return” keyword caused errors, thus, it was removed, because a ruby method will return its last statement. The two other private methods containing “return” also had that keyword removed.

Lastly, commenting was added throughout the files, to improve readability.

3. Introduce Helper Methods for Reusable Logic (for feedback_response_report)

As planned in step 1, the feedback_response_report method was split up into 3 private helper methods: get_feedback_authors(id), varying_rubrics_report(review_responses, response_map_ids), and static_rubrics_report(review_responses, response_map_ids). One change we made that was different from the UML diagram was the addition of an id parameter in the get_feedback_authors method, in order to access id, since it was a parameter given to feedback_response_report. Each helper method was planned in order to stick to the guideline of “A method should do only one thing and do it well.”

Final UML for FeedbackResponseMap

Authors

This image contains comments showing what the implementation looked like before:

This image shows the construction of the private method we implemented to replace it, reducing complexity and improving readability in the process:

Report

This image contains comments showing what the implementation looked like before (this image is out of date, the return functionality for the varying rubric report rounds has since been generalized, please see the repo for the updated version) (The updated version does not include the old version comments, which is why this screenshot is used):

These images show the construction of the private methods we implemented to replace it, reducing complexity and improving readability in the process:

Notice that in both the original self.feedback_response_report method and the private methods, comments are included to explain each non-obvious step of the process.

Complexity changes

Previously, Rubocop reported the following failures on behalf of the self.feedback_response_report method:

 Metrics/PerceivedComplexity: Perceived complexity for feedback_response_report is too high. [14/8]
 Metrics/CyclomaticComplexity: Cyclomatic complexity for feedback_response_report is too high. [12/7]
 Metrics/AbcSize: Assignment Branch Condition size for feedback_response_report is too high. [<14, 46, 16> 50.68/20]

Now, neither self.feedback_response_report nor any of its associated private methods trigger any of these Metric violations! (Note that the email method does trigger a AbcSize violation when the protective comments are removed, but adjusting that method is beyond the scope of this project for reasons listed in the Implementation Plan)

4. Research Future Refactoring for email Method

As discussed in the implementation plan, the email method was left as is. Any changes would require the addition of a new class and/or major refactoring for the functionality.

5. Improve Variable Naming and Structure

The two sets of variables discussed in the plan, @temp_response_map_ids and @temp_review_responses were modified as needed. First, the @temp_response_map_ids was moved to the helper methods varying_rubrics_report and static_rubrics_report, to be initialized, and changed to response_map_ids.

Secondly, @temp_review_responses was changed to review_responses in the helper methods that call it: varying_rubrics_report and static_rubrics_report

Testing

1. Adapt Existing Tests for Refactored Methods

• Any new helper methods will be private, and so by the "Magic Tricks of Testing" guide, should not be tested (both query and command messages sent to self are ignored in the test plan).
• Use mocks to isolate components during testing, especially for external dependencies like Mailer, to ensure that email functionality works as expected without direct integration.

2. Document Code for Clarity

• Write detailed comments explaining the purpose of each method, input and output, and any critical logic that might be complex for future developers to understand.
• Create or update documentation and diagrams to illustrate the newly designed structure, making it easier for new developers to contribute or modify functionality.

Documentation

Ensure all code is well-documented, with comments explaining purpose, inputs, outputs, and any complex logic. Maintain updated documentation for all modified and newly added classes and methods, allowing for ease of understanding and maintenance.

Files

The files that were added or modified to our repository are shown below.

Models

The following files were copied from the expertiza/expertiza repository:

• analytic/assignment_team_analytic.rb
• assignment_team.rb
• feedback_response_map.rb

Added assignment_team.rb and assignment_team_analytic.rb as the existing spec tests depended on those 2 files. No changes were made to these files other than commenting out include Scoring in assignment_team.rb

feedback_response_map.rb

Many comments were placed throughout the document in order to make code more readable.

The main changes were taking feedback_response_report method and scanning it, to find different functionalities that could be taken out of it and turned into the following private helper methods: get_feedback_authors, varying_rubrics_report, static_rubrics_report.

Additionally, the variable @temp_response_map_ids was changed into response_map_ids both inside and outside the helper methods that used this variable. The variable @temp_review_responses was not changed except for being called review_responses in the helper methods.

Database

migrate/20241203083135_add_missing_reponse_map_fields.rb

One class is added, called AddMissingResponseMapFields, with a change of adding columns to response_maps and type and calibrate_to.

db/schema.rb

The only things changed in this file were a version change, as well as the addition of a t.string of "type" and a t.integer of "calibrate_to".

Spec

factories.rb

This file was modified to use single quotation marks instead of double quotation marks to adhere to RuboCop.

factories/factories.rb

We populated the factory with necessary factories such as response, course, institution, and role, as well as fixing the factory formatting to function properly.

feedback_response_map.rb

The code was updated to adhere to RuboCop guidelines. Secondly, tests and parts of tests were commented out if they were reliant on functionality that was not yet implemented in reimplementation-back-end.

Video

https://youtu.be/SbmDoJNm4Q4

Pull Request

https://github.com/expertiza/reimplementation-back-end/pull/144

Conclusion

By refactoring feedback_response_map.rb, this project enhances Expertiza’s backend maintainability and readability, ensuring a modular, efficient, and more secure implementation.

Team Information

Mentor

• Chaitanya Srusti (crsrusti@ncsu.edu)

Members

• Sarah Daniel (sdaniel8@ncsu.edu)

• William Grochocinski (wagrocho@ncsu.edu)

• Tipton Middleton (tgmiddl2@ncsu.edu)

• ChatGPT (Honorary Member)