Expertiza_Wiki - User contributions [en]

CSC/ECE 517 Spring 2025 - E2524 Reimplement student review controller

2025-04-22T23:42:03Z

Spatel68: /* Controller Diagram */

== Introduction ==
The goal of this project is to reimplement the `StudentReviewController` and its associated views. The reimplementation will introduce a new service layer, streamline logic, and remove obsolete code (e.g., references to metareviews). The new design prioritizes clarity, modularity, and maintainability, following Rails best practices and RESTful conventions.

=== Purpose and Scope ===
This reimplementation addresses longstanding design issues in the original controller, including overly complex methods, redundant logic, and tight coupling between controller and view layers.

=== Objectives ===
* Redesign the controller to delegate all business logic to a new service class
* Eliminate obsolete functionality (e.g., metareviews)
* Improve testability by enabling unit tests on the service layer
* Simplify the view templates by preparing data in the controller beforehand
* Adhere to Rails RESTful conventions and best practices.
* Improve maintainability, localization support, and scalability.

== Requirements ==

Reimplement the `StudentReviewController` and ensure all associated components function correctly. The components involved include:

=== Models Used ===
* `AssignmentParticipant`
* `Assignment`
* `SignedUpTeam`
* `ReviewResponseMap`
* `SampleReview`
* `MetareviewResponseMap` (obsolete – all references to be removed)

==Existing Issues==
* '''Controller Complexity:'''
- The `list` method is too long and handles multiple responsibilities: data loading, logic, rendering setup, and redirects.
- Business logic is intertwined with HTTP concerns (e.g., redirection, instance variables).

* '''Redundant Logic:'''
- The `action_allowed?` method checks for student privileges multiple times.

* '''Hard-Coded Values:'''
- Literal strings like `"list"` and `"submitter"` are used instead of constants.

* '''Obsolete Code:'''
- The `MetareviewResponseMap` model is no longer relevant but still referenced.

* '''Lack of Separation of Concerns:'''
- Controller handles business logic that should reside in service/model layers.

== Design Principles Analysis ==

This section outlines how our application aligns with core software design principles, based on a detailed analysis of the <code>StudentReviewService</code>, <code>ApplicationController</code>, and <code>AuthenticationController</code>.

=== Single Responsibility Principle (SRP) ===
* '''Focused Responsibilities:'''
- <code>StudentReviewService</code> handles only review-related operations.
- <code>ApplicationController</code> manages cross-cutting concerns such as authorization and locale setting.
- Each class and method has one clear and dedicated purpose.

def calculate_review_progress
@num_reviews_total = @review_mappings.size
@num_reviews_completed = @review_mappings.count do |map|
!map.response.empty? && map.response.last.is_submitted
end
@num_reviews_in_progress = @num_reviews_total - @num_reviews_completed
end

=== DRY (Don’t Repeat Yourself) ===
* '''Centralized Logic:'''
- Shared methods like <code>load_participant_and_assignment</code> promote reuse.
- Authorization and locale-setting logic are abstracted into concerns.

def load_participant_and_assignment(participant_id)
@participant = AssignmentParticipant.find(participant_id)
@assignment = @participant.assignment
@topic_id = SignedUpTeam.topic_id(@assignment.id, @participant.user_id)
@review_phase = @assignment.current_stage(@topic_id)
end

=== Maintainability and Testability ===
* '''Modular Design:'''
- Business logic is placed in services to simplify future updates.
- Private methods are used to improve unit test isolation.
- Clear method names improve code readability.

def load_review_mappings
reviewer = @participant.get_reviewer
@review_mappings = reviewer ? ReviewResponseMap.where(...) : []
@review_mappings = @review_mappings.sort_by { |m| m.id % 5 } if @assignment.is_calibrated
end

=== RESTful Best Practices ===
* '''Rails-Conventional Controllers:'''
- RESTful routes and actions are followed.
- <code>AuthenticationController</code> includes a clear and secure login endpoint.

def login
user = User.find_by(name: params[:user_name]) || User.find_by(email: params[:user_name])
if user&.authenticate(params[:password])
token = JsonWebToken.encode({ id: user.id, ... }, 24.hours.from_now)
render json: { token: }, status: :ok
else
render json: { error: 'Invalid username / password' }, status: :unauthorized
end
end

=== Localization and Internationalization ===
* '''Locale Handling:'''
- Locale is determined from parameters or browser headers.
- Rails i18n support is properly configured and applied.

def set_locale
I18n.locale = extract_locale || I18n.default_locale
end

=== Summary Table ===

{| class="wikitable"
! Principle
! Status
! Notes
|-
| '''Single Responsibility'''
| Implemented
| Each class has one focused responsibility
|-
| '''DRY'''
| Implemented
| Shared logic centralized in service methods
|-
| '''Maintainability & Testability'''
| Implemented
| Modular code with helper methods and clear naming
|-
| '''RESTful Practices'''
| Implemented
| Follows Rails conventions
|-
| '''Localization/i18n'''
| Implemented
| Locale handled through params and headers
|}

=== Additional Strengths ===

* '''Service Object Pattern:'''
- Business logic is encapsulated in dedicated service classes, improving modularity and reuse.

* '''JWT Authentication:'''
- Secure authentication is implemented using JSON Web Tokens with proper token generation and expiration handling.

* '''Readable Method Naming:'''
- Method names clearly reflect their responsibilities, enhancing code readability and maintainability.

== Controller Diagram ==

[[File:controllerdiagram.png]]

== Design / Proposed Solution ==

=== Separation of Concerns ===
This reimplementation is based on a clear separation of responsibilities:
* The controller handles only routing, access validation, and view setup.
* The service object encapsulates all business logic and data handling.
* Obsolete functionality such as `MetareviewResponseMap` is eliminated to simplify the design.

=== Architectural Overview ===
The new design will be based on the following structure:
* A `StudentReviewController` that handles only HTTP responsibilities.
* A new `StudentReviewService` that encapsulates all business logic.
* Continued use of models like `AssignmentParticipant`, `Assignment`, and `ReviewResponseMap` (excluding `MetareviewResponseMap`).
* Views adjusted to work with service-driven data.

=== Breakdown of Controller Responsibilities ===
* Validate access using a simplified `action_allowed?` method
* Set locale using controller_locale
* Call `StudentReviewService.new(id, current_user).perform` to gather data
* Assign service output to instance variables for the view
* Redirect if bidding is enabled

=== Breakdown of Service Object Responsibilities ===
* Retrieve `AssignmentParticipant` and associated `Assignment`
* Calculate `topic_id` using `SignedUpTeam.topic_id`
* Determine review phase using `assignment.current_stage`
* Get reviewer via `participant.get_reviewer`
* Load and (if applicable) sort review mappings (`ReviewResponseMap`)
* Count completed and in-progress reviews
* Load sample reviews (`SampleReview`) and extract `response_ids`
* Check if bidding is enabled

=== Removal of Obsolete Code ===
* All logic involving `MetareviewResponseMap` will be deleted
* Related counters for metareview progress will be removed from both controller and views

=== action_allowed? Method ===
* Eliminate redundant privilege check: `|| current_user_has_student_privileges?`
* Use constants instead of literal strings (`LIST_ACTION = 'list'`)
* Restructure logic as:

<pre>
LIST_ACTION = 'list'.freeze
SUBMITTER_ROLE = 'submitter'.freeze

before_action :authorize_user, only: [LIST_ACTION.to_sym]
before_action :load_service, only: [:list]

private

def authorize_user
unless action_allowed?
render json: { error: 'Unauthorized' }, status: :unauthorized
return
end
end

# Quick-exit unless they're a student.
# Then, only allow the LIST_ACTION for users who also have the SUBMITTER_ROLE on this resource.
def action_allowed?
# guard clause: must be a student at all
return false unless current_user_has_student_privileges? # early return

# only permit “list” for submitters
action_name == LIST_ACTION &&
are_needed_authorizations_present?(params[:id], SUBMITTER_ROLE)
end</pre>

=== controller_locale Method ===

* Ensure the `controller_locale` method sets the locale appropriately for student users, supporting internationalization and accessibility. Adding the below code to ApplicationController Class.
<pre>
# 1) Set locale for every request
before_action :set_locale
# 2) Perform authorization
before_action :authorize

private

# Sets I18n.locale based on params, Accept-Language header, or default
def set_locale
I18n.locale = extract_locale || I18n.default_locale
end

# Returns a valid locale symbol or nil
def extract_locale
# 1) Param override
if params[:locale].present?
sym = params[:locale].to_sym
return sym if I18n.available_locales.include?(sym)
end

# 2) Accept-Language header fallback
header = request.env['HTTP_ACCEPT_LANGUAGE'].to_s
header
.split(',')
.map { |l| l[0..1].downcase.to_sym }
.find { |loc| I18n.available_locales.include?(loc) }
end
end
</pre>

=== list Method ===
The `list` method will be removed and replaced with a simplified call to the service:

<pre>
def list
return unless valid_participant?
@data = StudentReviewService.new(params[:id], current_user.id).perform
redirect_to bidding_path if @data[:bidding_enabled]
end
</pre>

== View Refactoring Plan ==

* `list.html.erb`: Simplify to only display data passed in from `@data`
* `_responses.html.erb`: Remove inline logic (e.g., topic ID lookups) and rely on passed-in values
* `_set_dynamic_*`: May be removed if dynamic review logic is relocated or rewritten
* View logic for metareviews will be removed
* Ensure views follow Rails i18n localization conventions by replacing any hardcoded strings with translation keys where appropriate.

== Test Coverage Summary ==
To ensure the integrity of the reimplementation, both the service objects and controller actions should have dedicated tests. It is recommended to generate test skeletons as soon as the methods are in place.

[[File:Screenshot_2025-04-22_at_12.29.17_PM.png|thumb|none|600px|RSpec Tests Report]]



{| class="wikitable"
! File Name
! Test Coverage
|-
| <code>app/controllers/api/v1/student_review_controller.rb</code>
| 86.67%
|-
| <code>app/controllers/application_controller.rb</code>
| 100.0%
|-
| <code>app/services/student_review_service.rb</code>
| 85.71%
|}



[[File:Ckecks.png|thumb|none|600px|All checks have passed]]



== Summary of Tests in StudentReviewController Spec ==

The test file provides comprehensive test coverage for the <code>Api::V1::StudentReviewController</code>. Here's a summary of what was tested:

=== Core Controller Functionality ===
* '''Controller Action Existence:'''
- Verifies that the <code>list</code> action is defined.
* '''Response Structure:'''
- Confirms the controller returns the expected JSON structure with all required fields.
* '''Authorization Logic:'''
- Ensures that proper authorization checks are in place and function correctly.

=== Authorization Tests ===
* '''Proper Authorization:'''
- Tests scenarios where a user is authorized to access resources.
* '''Unauthorized Access:'''
- Ensures unauthorized users receive a 401 response.
* '''Student Privileges:'''
- Tests the <code>action_allowed?</code> method with and without student privileges.
* '''Participant Authorization:'''
- Verifies the <code>authorized_participant?</code> method behavior.

=== Bidding Functionality ===
* '''Bidding Enabled:'''
- Verifies redirection occurs when bidding is enabled.
* '''Bidding Disabled:'''
- Ensures no redirection occurs when bidding is disabled.
* '''<code>check_bidding_redirect</code> Method:'''
- Tests the method independently to verify redirection logic.

=== Service Integration ===
* '''Service Loading:'''
- Tests for proper initialization of the <code>load_service</code> method.
* '''Service Data Usage:'''
- Confirms service output is correctly used in controller responses.

=== Reviewer-related Tests ===
* '''With Reviewer:'''
- Tests the flow when a participant has an associated reviewer.
* '''Without Reviewer:'''
- Tests behavior when no reviewer is present.

=== Special Assignment Types ===
* '''Calibrated Assignments:'''
- Tests functionality related to calibration logic.
* '''Topic ID Handling:'''
- Verifies accurate calculation of <code>topic_id</code>.

=== Internationalization ===
* '''Locale Setting:'''
- Tests <code>controller_locale</code> method functionality.
* '''User Locale Preference:'''
- Tests locale assignment when user has preferences.
* '''Default Locale Fallback:'''
- Ensures fallback to default locale works properly.

=== Edge Cases ===
* '''Missing Reviewer:'''
- Verifies behavior when the reviewer does not exist.
* '''Review Mappings Sorting:'''
- Tests sorting logic for review mappings under various conditions.

This comprehensive test suite ensures that all aspects of the <code>StudentReviewController</code> are thoroughly tested—from basic functionality to edge cases and error handling—achieving a test coverage rate of '''86.67%'''.

== Summary of Tests in ApplicationController Spec ==

The <code>application_controller_spec.rb</code> file provides thorough test coverage for the internationalization (i18n) functionality in the <code>ApplicationController</code>. Here's a summary of what was tested:

=== Locale Setting Logic ===
* '''Set Locale Method:'''
- Tests the <code>set_locale</code> method that configures the application's current locale.
* '''Invalid Locale Handling:'''
- Verifies that invalid locales passed in parameters are properly ignored.
* '''Default Locale Fallback:'''
- Ensures fallback to the default locale when no valid locale is found.

=== Locale Extraction ===
* '''Extract Locale Method:'''
- Tests the <code>extract_locale</code> method for determining the locale to use.
* '''Parameter-based Extraction:'''
- Verifies that the locale is correctly extracted from URL parameters.
* '''HTTP Header Extraction:'''
- Tests extraction of locale from the <code>Accept-Language</code> HTTP header.
* '''Validation:'''
- Confirms extracted locales are checked against <code>I18n.available_locales</code>.

=== HTTP Accept-Language Header Parsing ===
* '''Country-Specific Codes:'''
- Tests proper handling of country-specific language codes such as <code>en-US</code>.
* '''Quality Factors:'''
- Tests parsing of language quality preferences (e.g., <code>fr;q=0.9</code>).
* '''Multiple Languages:'''
- Verifies behavior when the header includes multiple language options.
* '''Invalid Languages:'''
- Ensures that unsupported languages are safely ignored.
* '''Empty/Nil Headers:'''
- Tests how the system behaves when the language header is empty or missing.

=== Edge Cases ===
* '''Multi-part Headers:'''
- Tests parsing of complex <code>Accept-Language</code> headers.
* '''Priority Handling:'''
- Ensures that language preferences are honored based on quality factors.
* '''Fallback Behavior:'''
- Verifies fallback logic when preferred languages are not available.

These tests use RSpec’s controller testing features with an anonymous controller inheriting from <code>ApplicationController</code> to isolate i18n behavior. Setup and teardown logic ensures that the original i18n configuration is restored after each test.

Overall, this test suite ensures robust support for multilingual scenarios, helping maintain internationalization integrity throughout the application.

== Summary of Tests in StudentReviewService Spec ==

The <code>student_review_service_spec.rb</code> file provides comprehensive test coverage for the <code>StudentReviewService</code> class. Here's a breakdown of what was tested:

=== Initialization and Setup ===
* '''Constructor Testing:'''
- Verifies proper initialization, including loading of the participant and assignment.
* '''Topic ID and Review Phase:'''
- Ensures that <code>topic_id</code> and <code>review_phase</code> are set correctly.
* '''Error Handling:'''
- Tests that appropriate exceptions are raised when the participant does not exist.

=== Core Functionality ===
* '''Bidding Enabled Check:'''
- Tests the <code>bidding_enabled?</code> method under both enabled and disabled conditions.
* '''Review Mappings:'''
- Verifies correct loading and sorting of review mappings.
* '''Response IDs:'''
- Confirms that response IDs are correctly extracted from the database.

=== Special Assignment Cases ===
* '''Calibrated Assignments:'''
- Tests sorting logic specific to calibrated assignments:
- Uses <code>id % 5</code> criteria.
- Covers specific IDs (1, 2, 5, 6, 7) to ensure sorting correctness.

=== Edge Cases ===
* '''No Reviewer:'''
- Ensures <code>review_mappings</code> is set to an empty array.
- Verifies that progress counters for reviews are zero.

=== Response Handling ===
* '''Response ID Loading:'''
- Confirms response IDs are correctly retrieved from <code>SampleReview</code>.
* '''Empty Response Lists:'''
- Tests proper handling when no responses are available.

=== Testing Approach ===
This test suite uses RSpec's mocking and stubbing features to isolate logic from external dependencies:
* Uses doubles for models like <code>User</code>, <code>Assignment</code>, and <code>Participant</code>.
* Stubs private methods when needed to focus tests on expected behavior.
* Mocks <code>SampleReview</code> for controlled data input.
* Uses RSpec's expectation syntax to assert correctness.

Overall, this test suite thoroughly validates the <code>StudentReviewService</code> class under both standard and edge-case scenarios, ensuring correct functionality for calibrated assignments, bidding, review progress, and reviewer absence.

== Team ==
'''Mentor'''
* Janice Uwujaren

'''Students'''
* Kenil Patel (kpatel47)
* Smit Patel (spatel68)
* Katerina Vilkomir (evilkom)

== Relevant Links ==

* '''Controller (Original Expertiza Repo):'''
https://github.com/expertiza/expertiza/blob/main/app/controllers/student_review_controller.rb

* '''Views (one controller action view and three partials):'''
https://github.com/expertiza/expertiza/tree/main/app/views/student_review

* '''StudentReviewService Implementation Repo (Kii4ka):'''
https://github.com/Kii4ka/reimplementation-back-end

* '''Pull Request to Expertiza Reimplementation Repo (#197):'''
https://github.com/expertiza/reimplementation-back-end/pull/197

File:Controllerdiagram.png

2025-04-22T23:39:20Z

Spatel68:

CSC/ECE 517 Spring 2025 - E2501: Refactor review mapping helper.rb

2025-03-25T00:54:25Z

Spatel68:

== Team ==
=== Mentor ===
* Janice Uwujaren

=== Team Members ===
* Katerina Vilkomir (evilkom)
* Smit Patel (spatel68)
* Kenil Patel (kpatel47)

=== Relevant Links ===
* '''Link to Expertiza Repository:''' https://github.com/expertiza/expertiza 

* '''Link to Forked Repository: ''' https://github.com/Kii4ka/expertiza 

* ''' Link to Pull Request: ''' 

== Expertiza Background==
Expertiza is a Ruby on Rails framework-based open-source web application. It is kept up by NC State University employees and students. With the help of this program, instructors can fully manage the assignments and responsibilities assigned in their classes. Expertiza provides many strong features, such as subject addition, group creation, and peer review administration. It is a flexible platform that can manage many kinds of tasks. Users can consult the Expertiza wiki to get more in-depth details about the many capabilities that Expertiza offers.

== About Helper ==
The review_mapping_helper is integral to the peer review process, handling tasks such as calculating review scores, managing submission statuses, generating reports, and displaying review metrics. However, the module is complex because it depends on numerous instance variables, like @assignment and @review_scores, and mixes business logic with view logic.

== Problem Statement ==
The tight coupling and unclear separation of concerns make the code difficult to understand, test, and manage. To improve the overall quality of the codebase, the goal of this project is to refactor to ensure that the required info is passed as parameters and to separate business logic from view-related logic.

== What Needs To Be Done ==
*''' Separation of Business Logic and Helpers.'''
** The get_data_for_review_report method in the ReviewMappingHelper module contains business logic. To improve separation of concerns, create a dedicated Review Report Service to handle calculations, allowing the helper class to focus on view-related logic.
** Similarly, the display_volume_metric_chart method includes business logic for generating chart data. A Chart Data Service should be introduced to manage these calculations, leaving the helper responsible for rendering.
** The Charts Module is tightly coupled with the helper class due to its reliance on initialize_chart_elements. To enhance modularity, a separate service object should handle chart initialization.

*'''Improving Method Clarity and Consistency'''
**Some methods use inconsistent naming conventions (determine, get, obtain). Align method names with Ruby conventions—"get" is generally preferred.
**Methods like get_data_for_review_report rely on instance variables such as @assignment, assuming they are set beforehand. To improve clarity and reduce potential errors, pass these variables as parameters instead of assuming their existence.

*'''Reducing Unnecessary Comments and Inline Styles'''
**Replace redundant comments with meaningful explanations that provide insight rather than restating what the code does.
**Remove these comments from _review_rounds.html.erb. They add no value. Also, remove any hardcoded values.
*** 
*** 
**The image_tag in _grade_and_comments_form.html.erb uses inline styles; these should be moved to a CSS stylesheet for better maintainability.
**The _reviewer_info.html.erb view also contains inline styles that should be extracted into a stylesheet.

*'''Removing Unused and Inefficient Code'''
**The ReviewMappingHelper class contains commented-out code. If this code is no longer needed, delete it.
**Identify opportunities to use guard clauses to break out of methods early, reducing unnecessary nesting and improving readability.
*'''Efficient Database Queries'''
**Several views, such as _response_maps.html.erb and _review_rounds.html.erb, execute database queries directly within the view, making multiple database calls. This logic should be moved to the model, with the Controller passing pre-fetched data to the view.
**The _reviewer_info.html.erb view accesses session data directly, which should instead be retrieved from the Controller to uphold the Separation of Concerns principle.
*'''Testing and Code Validation'''
**Ensure all new Service Classes created for report calculations, chart data, and database queries are thoroughly tested.
**Review existing tests and fix any failing tests to ensure correctness.
**Ensure full test coverage for all helper methods.
*'''Clarify Hardcoded Logic'''
**The _reviewer_info.html.erb view references reviewer.id == 1, but the significance of this ID is unclear. Either document its importance or store it in a constant that makes sense for better readability.

== Design Pattern ==
The Refactoring pattern is essential to enhance code readability, maintainability, and adherence to best practices. By systematically restructuring code components, eliminating redundancies, and applying standard conventions, the Refactoring pattern ensures improved code quality and easier future modifications.

==Plan of work==

We began our work by examining the <code>review_mapping_helper.rb</code> file to understand its connection to the rest of the codebase. We then had detailed discussions with our mentor about the project's flow and structure. We organized our project requirements by difficulty levels, assigned tasks among the team, and began refactoring existing methods and creating new files as needed. We made sure to preserve the integrity of existing test cases while adding new ones where needed.

==Implementation==

'''Issue #29:''' Removed redundant comments from <code>_review_rounds.html.erb</code>
*'''Solution:'''The solution involved cleaning up the code by removing redundant and outdated comments that were no longer necessary or too specific, such as references to team name handling and hard-coded logic for a particular instructor. The code was also slightly refactored for better readability, including consistent use of quotes in HTML attributes and ERB tags. 
*'''Changes'''
[[File:Removed_redundant_comments.jpg]]
 

----

'''Issue #24:'''Inline styles fix for <code>_grades_and_comments._form.html.erb</code> and <code>_reviewer_info.html.erb</code> 
*'''Solution:'''Removed inline styles from the image tag and added a CSS class called "text_macros". Properly closed the form_with block by adding the missing <% end %>.No major changes were made in _reviewer_info.html.erb, only some formatting improvements.. 
*'''Changes'''
 
[[File:grade_and_comments_form.jpg]]
 
[[File:reviewer_info.jpg]]
 
----

'''Issue #25:''' Move database queries to models in <code>_response_maps.html.erb</code> and <code>_review_rounds.html.erb</code>. 
*'''Solution:''' Moved database queries to models by replacing <code>Team.where(id: reviewer_map.reviewee_id).length > 0</code> with <code>Team.from_response_map(reviewer_map)</code>, encapsulating team retrieval logic in the model. Removed redundant <code>Team.find</code> calls and replaced <code>Response.exists?(map_id: reviewer_map.id)</code> with <code>reviewer_response_map.has_response?</code>. 
*'''Changes'''
[[File:Removed_redundant_comments.jpg]]
 
[[File:Response maps.jpg]]
 
----

'''Issue #27:''' Retrieved Session data from controller and Replace magic number with constant 

*'''Solution:''' Retrieved session data from the controller by replacing session[:ip] with @session_ip, it's set in the controller. Replaced the magic number reviewer.id == 1 with <code>user.super_admin?</code>, super_admin? is a model method defining the condition 
*'''Changes'''
[[File:Reviewer_info_constant.jpg]]
 

----
'''Issue #7:''' Several methods for feedback_response_maps, check docs to manage this confusion 
*'''Methods involved:''' <code> feedback_response_for_author()</code> 
*'''Previous Code:''' previous code had an ambiguous variable name 
*'''Problem:''' The purpose of the variable was unclear. 
*'''Solution:''' Rename the variable to enhance its clarity and provide comments to explain its purpose clearly. 
*'''Changes'''
[[File:Issue7.jpg]]
 
----

==Test Plan==
This project involves refactoring, meaning that the original functionality should remain unchanged. Tasks included adding comments to existing code, altering variable names, and similar adjustments that didn't affect the test cases. When method names were modified or moved to different files, corresponding changes were made in the relevant RSpec files. All test cases passed successfully, indicating that the refactoring didn't break the existing functionality.
 

As part of this project, the following actions were taken to ensure '''test accuracy and full functionality''':
* RSpec files were updated to reflect the new service class structure.
* The test suite was run after each change to confirm that behavior remained consistent across helper and service files.
* New service classes were fully tested, achieving '''100% test coverage'''.
* The `ReviewMappingHelper` was thoroughly reviewed and maintained a high coverage rate of '''87.85%''', confirming strong existing test quality.
* All helper methods dependent on parameters like `@assignment` were validated to ensure they functioned correctly after being decoupled from instance variables.
* No failing or broken tests were introduced during or after the refactor.
* '''Test skeletons''' (review_mapping_helper_spec.rb, chart_data_service_spec.rb, and review_report_service_spec.rb) were provided by '''Nitya Naga Sai Atluri natluri@ncsu.edu'''. These were used as the foundation for validating and extending test coverage.

{| class="wikitable" style="text-align:center"
! File
! % Covered
! Lines
! Relevant Lines
! Lines Covered
! Lines Missed
! Avg. Hits/Line
|-
| app/helpers/review_mapping_helper.rb
| 87.85%
| 319
| 181
| 159
| 22
| 3.5
|-
| app/services/chart_data_service.rb
| 100.00%
| 112
| 30
| 30
| 0
| 3.9
|-
| app/services/chart_initialization_service.rb
| 100.00%
| 34
| 22
| 22
| 0
| 243.4
|-
| app/services/review_report_service.rb
| 100.00%
| 53
| 26
| 26
| 0
| 11.9
|}

File:Reviewer info constant.jpg

2025-03-25T00:52:58Z

Spatel68:

CSC/ECE 517 Spring 2025 - E2501: Refactor review mapping helper.rb

2025-03-25T00:51:42Z

Spatel68:

== Team ==
=== Mentor ===
* Janice Uwujaren

=== Team Members ===
* Katerina Vilkomir (evilkom)
* Smit Patel (spatel68)
* Kenil Patel (kpatel47)

=== Relevant Links ===
* '''Link to Expertiza Repository:''' https://github.com/expertiza/expertiza 

* '''Link to Forked Repository: ''' https://github.com/Kii4ka/expertiza 

* ''' Link to Pull Request: ''' 

== Expertiza Background==
Expertiza is a Ruby on Rails framework-based open-source web application. It is kept up by NC State University employees and students. With the help of this program, instructors can fully manage the assignments and responsibilities assigned in their classes. Expertiza provides many strong features, such as subject addition, group creation, and peer review administration. It is a flexible platform that can manage many kinds of tasks. Users can consult the Expertiza wiki to get more in-depth details about the many capabilities that Expertiza offers.

== About Helper ==
The review_mapping_helper is integral to the peer review process, handling tasks such as calculating review scores, managing submission statuses, generating reports, and displaying review metrics. However, the module is complex because it depends on numerous instance variables, like @assignment and @review_scores, and mixes business logic with view logic.

== Problem Statement ==
The tight coupling and unclear separation of concerns make the code difficult to understand, test, and manage. To improve the overall quality of the codebase, the goal of this project is to refactor to ensure that the required info is passed as parameters and to separate business logic from view-related logic.

== What Needs To Be Done ==
*''' Separation of Business Logic and Helpers.'''
** The get_data_for_review_report method in the ReviewMappingHelper module contains business logic. To improve separation of concerns, create a dedicated Review Report Service to handle calculations, allowing the helper class to focus on view-related logic.
** Similarly, the display_volume_metric_chart method includes business logic for generating chart data. A Chart Data Service should be introduced to manage these calculations, leaving the helper responsible for rendering.
** The Charts Module is tightly coupled with the helper class due to its reliance on initialize_chart_elements. To enhance modularity, a separate service object should handle chart initialization.

*'''Improving Method Clarity and Consistency'''
**Some methods use inconsistent naming conventions (determine, get, obtain). Align method names with Ruby conventions—"get" is generally preferred.
**Methods like get_data_for_review_report rely on instance variables such as @assignment, assuming they are set beforehand. To improve clarity and reduce potential errors, pass these variables as parameters instead of assuming their existence.

*'''Reducing Unnecessary Comments and Inline Styles'''
**Replace redundant comments with meaningful explanations that provide insight rather than restating what the code does.
**Remove these comments from _review_rounds.html.erb. They add no value. Also, remove any hardcoded values.
*** 
*** 
**The image_tag in _grade_and_comments_form.html.erb uses inline styles; these should be moved to a CSS stylesheet for better maintainability.
**The _reviewer_info.html.erb view also contains inline styles that should be extracted into a stylesheet.

*'''Removing Unused and Inefficient Code'''
**The ReviewMappingHelper class contains commented-out code. If this code is no longer needed, delete it.
**Identify opportunities to use guard clauses to break out of methods early, reducing unnecessary nesting and improving readability.
*'''Efficient Database Queries'''
**Several views, such as _response_maps.html.erb and _review_rounds.html.erb, execute database queries directly within the view, making multiple database calls. This logic should be moved to the model, with the Controller passing pre-fetched data to the view.
**The _reviewer_info.html.erb view accesses session data directly, which should instead be retrieved from the Controller to uphold the Separation of Concerns principle.
*'''Testing and Code Validation'''
**Ensure all new Service Classes created for report calculations, chart data, and database queries are thoroughly tested.
**Review existing tests and fix any failing tests to ensure correctness.
**Ensure full test coverage for all helper methods.
*'''Clarify Hardcoded Logic'''
**The _reviewer_info.html.erb view references reviewer.id == 1, but the significance of this ID is unclear. Either document its importance or store it in a constant that makes sense for better readability.

== Design Pattern ==
The Refactoring pattern is essential to enhance code readability, maintainability, and adherence to best practices. By systematically restructuring code components, eliminating redundancies, and applying standard conventions, the Refactoring pattern ensures improved code quality and easier future modifications.

==Plan of work==

We began our work by examining the <code>review_mapping_helper.rb</code> file to understand its connection to the rest of the codebase. We then had detailed discussions with our mentor about the project's flow and structure. We organized our project requirements by difficulty levels, assigned tasks among the team, and began refactoring existing methods and creating new files as needed. We made sure to preserve the integrity of existing test cases while adding new ones where needed.

==Implementation==

'''Issue #29:''' Removed redundant comments from <code>_review_rounds.html.erb</code>
*'''Solution:'''The solution involved cleaning up the code by removing redundant and outdated comments that were no longer necessary or too specific, such as references to team name handling and hard-coded logic for a particular instructor. The code was also slightly refactored for better readability, including consistent use of quotes in HTML attributes and ERB tags. 
*'''Changes'''
[[File:Removed_redundant_comments.jpg]]
 

----

'''Issue #24:'''Inline styles fix for <code>_grades_and_comments._form.html.erb</code> and <code>_reviewer_info.html.erb</code> 
*'''Solution:'''Removed inline styles from the image tag and added a CSS class called "text_macros". Properly closed the form_with block by adding the missing <% end %>.No major changes were made in _reviewer_info.html.erb, only some formatting improvements.. 
*'''Changes'''
 
[[File:grade_and_comments_form.jpg]]
 
[[File:reviewer_info.jpg]]
 
----

'''Issue #25:''' Move database queries to models in <code>_response_maps.html.erb</code> and <code>_review_rounds.html.erb</code>. 
*'''Solution:''' Moved database queries to models by replacing <code>Team.where(id: reviewer_map.reviewee_id).length > 0</code> with <code>Team.from_response_map(reviewer_map)</code>, encapsulating team retrieval logic in the model. Removed redundant <code>Team.find</code> calls and replaced <code>Response.exists?(map_id: reviewer_map.id)</code> with <code>reviewer_response_map.has_response?</code>. 
*'''Changes'''
[[File:Removed_redundant_comments.jpg]]
 
[[File:Response maps.jpg]]
 
----

'''Issue #27:''' Retrieved Session data from controller and Replace magic number with constant 

*'''Solution:''' Retrieved session data from the controller by replacing session[:ip] with @session_ip, it's set in the controller. Replaced the magic number reviewer.id == 1 with <code>user.super_admin?</code>, super_admin? is a model method defining the condition 
*'''Changes'''
[[File:Issue7.jpg]]
 

----
'''Issue #7:''' Several methods for feedback_response_maps, check docs to manage this confusion 
*'''Methods involved:''' <code> feedback_response_for_author()</code> 
*'''Previous Code:''' previous code had an ambiguous variable name 
*'''Problem:''' The purpose of the variable was unclear. 
*'''Solution:''' Rename the variable to enhance its clarity and provide comments to explain its purpose clearly. 
*'''Changes'''
[[File:Issue7.jpg]]
 
----

==Test Plan==
This project involves refactoring, meaning that the original functionality should remain unchanged. Tasks included adding comments to existing code, altering variable names, and similar adjustments that didn't affect the test cases. When method names were modified or moved to different files, corresponding changes were made in the relevant RSpec files. All test cases passed successfully, indicating that the refactoring didn't break the existing functionality.
 

As part of this project, the following actions were taken to ensure '''test accuracy and full functionality''':
* RSpec files were updated to reflect the new service class structure.
* The test suite was run after each change to confirm that behavior remained consistent across helper and service files.
* New service classes were fully tested, achieving '''100% test coverage'''.
* The `ReviewMappingHelper` was thoroughly reviewed and maintained a high coverage rate of '''87.85%''', confirming strong existing test quality.
* All helper methods dependent on parameters like `@assignment` were validated to ensure they functioned correctly after being decoupled from instance variables.
* No failing or broken tests were introduced during or after the refactor.
* '''Test skeletons''' (review_mapping_helper_spec.rb, chart_data_service_spec.rb, and review_report_service_spec.rb) were provided by '''Nitya Naga Sai Atluri natluri@ncsu.edu'''. These were used as the foundation for validating and extending test coverage.

{| class="wikitable" style="text-align:center"
! File
! % Covered
! Lines
! Relevant Lines
! Lines Covered
! Lines Missed
! Avg. Hits/Line
|-
| app/helpers/review_mapping_helper.rb
| 87.85%
| 319
| 181
| 159
| 22
| 3.5
|-
| app/services/chart_data_service.rb
| 100.00%
| 112
| 30
| 30
| 0
| 3.9
|-
| app/services/chart_initialization_service.rb
| 100.00%
| 34
| 22
| 22
| 0
| 243.4
|-
| app/services/review_report_service.rb
| 100.00%
| 53
| 26
| 26
| 0
| 11.9
|}

CSC/ECE 517 Spring 2025 - E2501: Refactor review mapping helper.rb

2025-03-25T00:40:57Z

Spatel68:

== Team ==
=== Mentor ===
* Janice Uwujaren

=== Team Members ===
* Katerina Vilkomir (evilkom)
* Smit Patel (spatel68)
* Kenil Patel (kpatel47)

=== Relevant Links ===
* '''Link to Expertiza Repository:''' https://github.com/expertiza/expertiza 

* '''Link to Forked Repository: ''' https://github.com/Kii4ka/expertiza 

* ''' Link to Pull Request: ''' 

== Expertiza Background==
Expertiza is a Ruby on Rails framework-based open-source web application. It is kept up by NC State University employees and students. With the help of this program, instructors can fully manage the assignments and responsibilities assigned in their classes. Expertiza provides many strong features, such as subject addition, group creation, and peer review administration. It is a flexible platform that can manage many kinds of tasks. Users can consult the Expertiza wiki to get more in-depth details about the many capabilities that Expertiza offers.

== About Helper ==
The review_mapping_helper is integral to the peer review process, handling tasks such as calculating review scores, managing submission statuses, generating reports, and displaying review metrics. However, the module is complex because it depends on numerous instance variables, like @assignment and @review_scores, and mixes business logic with view logic.

== Problem Statement ==
The tight coupling and unclear separation of concerns make the code difficult to understand, test, and manage. To improve the overall quality of the codebase, the goal of this project is to refactor to ensure that the required info is passed as parameters and to separate business logic from view-related logic.

== What Needs To Be Done ==
*''' Separation of Business Logic and Helpers.'''
** The get_data_for_review_report method in the ReviewMappingHelper module contains business logic. To improve separation of concerns, create a dedicated Review Report Service to handle calculations, allowing the helper class to focus on view-related logic.
** Similarly, the display_volume_metric_chart method includes business logic for generating chart data. A Chart Data Service should be introduced to manage these calculations, leaving the helper responsible for rendering.
** The Charts Module is tightly coupled with the helper class due to its reliance on initialize_chart_elements. To enhance modularity, a separate service object should handle chart initialization.

*'''Improving Method Clarity and Consistency'''
**Some methods use inconsistent naming conventions (determine, get, obtain). Align method names with Ruby conventions—"get" is generally preferred.
**Methods like get_data_for_review_report rely on instance variables such as @assignment, assuming they are set beforehand. To improve clarity and reduce potential errors, pass these variables as parameters instead of assuming their existence.

*'''Reducing Unnecessary Comments and Inline Styles'''
**Replace redundant comments with meaningful explanations that provide insight rather than restating what the code does.
**Remove these comments from _review_rounds.html.erb. They add no value. Also, remove any hardcoded values.
*** 
*** 
**The image_tag in _grade_and_comments_form.html.erb uses inline styles; these should be moved to a CSS stylesheet for better maintainability.
**The _reviewer_info.html.erb view also contains inline styles that should be extracted into a stylesheet.

*'''Removing Unused and Inefficient Code'''
**The ReviewMappingHelper class contains commented-out code. If this code is no longer needed, delete it.
**Identify opportunities to use guard clauses to break out of methods early, reducing unnecessary nesting and improving readability.
*'''Efficient Database Queries'''
**Several views, such as _response_maps.html.erb and _review_rounds.html.erb, execute database queries directly within the view, making multiple database calls. This logic should be moved to the model, with the Controller passing pre-fetched data to the view.
**The _reviewer_info.html.erb view accesses session data directly, which should instead be retrieved from the Controller to uphold the Separation of Concerns principle.
*'''Testing and Code Validation'''
**Ensure all new Service Classes created for report calculations, chart data, and database queries are thoroughly tested.
**Review existing tests and fix any failing tests to ensure correctness.
**Ensure full test coverage for all helper methods.
*'''Clarify Hardcoded Logic'''
**The _reviewer_info.html.erb view references reviewer.id == 1, but the significance of this ID is unclear. Either document its importance or store it in a constant that makes sense for better readability.

== Design Pattern ==
The Refactoring pattern is essential to enhance code readability, maintainability, and adherence to best practices. By systematically restructuring code components, eliminating redundancies, and applying standard conventions, the Refactoring pattern ensures improved code quality and easier future modifications.

==Plan of work==

We began our work by examining the <code>review_mapping_helper.rb</code> file to understand its connection to the rest of the codebase. We then had detailed discussions with our mentor about the project's flow and structure. We organized our project requirements by difficulty levels, assigned tasks among the team, and began refactoring existing methods and creating new files as needed. We made sure to preserve the integrity of existing test cases while adding new ones where needed.

==Implementation==

'''Issue #29:''' Removed redundant comments from <code>_review_rounds.html.erb</code>
*'''Solution:'''The solution involved cleaning up the code by removing redundant and outdated comments that were no longer necessary or too specific, such as references to team name handling and hard-coded logic for a particular instructor. The code was also slightly refactored for better readability, including consistent use of quotes in HTML attributes and ERB tags. 
*'''Changes'''
[[File:Removed_redundant_comments.jpg]]
 

----

'''Issue #24:'''Inline styles fix for <code>_grades_and_comments._form.html.erb</code> and <code>_reviewer_info.html.erb</code> 
*'''Solution:'''Removed inline styles from the image tag and added a CSS class called "text_macros". Properly closed the form_with block by adding the missing <% end %>.No major changes were made in _reviewer_info.html.erb, only some formatting improvements.. 
*'''Changes'''
 
[[File:grade_and_comments_form.jpg]]
 
[[File:reviewer_info.jpg]]
 
----

'''Issue #6:''' Move database queries to models in <code>_response_maps.html.erb</code> and <code>_review_rounds.html.erb</code>. 
*'''Solution:''' Moved database queries to models by replacing <code>Team.where(id: reviewer_map.reviewee_id).length > 0</code> with <code>Team.from_response_map(reviewer_map)</code>, encapsulating team retrieval logic in the model. Removed redundant <code>Team.find</code> calls and replaced <code>Response.exists?(map_id: reviewer_map.id)</code> with <code>reviewer_response_map.has_response?</code>. 
*'''Changes'''
[[File:Removed_redundant_comments.jpg]]
 
[[Response maps.jpg]]
 
----

'''Issue #7:''' Several methods for feedback_response_maps, check docs to manage this confusion 
*'''Methods involved:''' <code> feedback_response_for_author()</code> 
*'''Previous Code:''' previous code had an ambiguous variable name 
*'''Problem:''' The purpose of the variable was unclear. 
*'''Solution:''' Rename the variable to enhance its clarity and provide comments to explain its purpose clearly. 
*'''Changes'''
[[File:Issue7.jpg]]
 
----

==Test Plan==
This project involves refactoring, meaning that the original functionality should remain unchanged. Tasks included adding comments to existing code, altering variable names, and similar adjustments that didn't affect the test cases. When method names were modified or moved to different files, corresponding changes were made in the relevant RSpec files. All test cases passed successfully, indicating that the refactoring didn't break the existing functionality.
 

As part of this project, the following actions were taken to ensure '''test accuracy and full functionality''':
* RSpec files were updated to reflect the new service class structure.
* The test suite was run after each change to confirm that behavior remained consistent across helper and service files.
* New service classes were fully tested, achieving '''100% test coverage'''.
* The `ReviewMappingHelper` was thoroughly reviewed and maintained a high coverage rate of '''87.85%''', confirming strong existing test quality.
* All helper methods dependent on parameters like `@assignment` were validated to ensure they functioned correctly after being decoupled from instance variables.
* No failing or broken tests were introduced during or after the refactor.
* '''Test skeletons''' (review_mapping_helper_spec.rb, chart_data_service_spec.rb, and review_report_service_spec.rb) were provided by '''Nitya Naga Sai Atluri natluri@ncsu.edu'''. These were used as the foundation for validating and extending test coverage.

{| class="wikitable" style="text-align:center"
! File
! % Covered
! Lines
! Relevant Lines
! Lines Covered
! Lines Missed
! Avg. Hits/Line
|-
| app/helpers/review_mapping_helper.rb
| 87.85%
| 319
| 181
| 159
| 22
| 3.5
|-
| app/services/chart_data_service.rb
| 100.00%
| 112
| 30
| 30
| 0
| 3.9
|-
| app/services/chart_initialization_service.rb
| 100.00%
| 34
| 22
| 22
| 0
| 243.4
|-
| app/services/review_report_service.rb
| 100.00%
| 53
| 26
| 26
| 0
| 11.9
|}

CSC/ECE 517 Spring 2025 - E2501: Refactor review mapping helper.rb

2025-03-25T00:40:02Z

Spatel68:

== Team ==
=== Mentor ===
* Janice Uwujaren

=== Team Members ===
* Katerina Vilkomir (evilkom)
* Smit Patel (spatel68)
* Kenil Patel (kpatel47)

=== Relevant Links ===
* '''Link to Expertiza Repository:''' https://github.com/expertiza/expertiza 

* '''Link to Forked Repository: ''' https://github.com/Kii4ka/expertiza 

* ''' Link to Pull Request: ''' 

== Expertiza Background==
Expertiza is a Ruby on Rails framework-based open-source web application. It is kept up by NC State University employees and students. With the help of this program, instructors can fully manage the assignments and responsibilities assigned in their classes. Expertiza provides many strong features, such as subject addition, group creation, and peer review administration. It is a flexible platform that can manage many kinds of tasks. Users can consult the Expertiza wiki to get more in-depth details about the many capabilities that Expertiza offers.

== About Helper ==
The review_mapping_helper is integral to the peer review process, handling tasks such as calculating review scores, managing submission statuses, generating reports, and displaying review metrics. However, the module is complex because it depends on numerous instance variables, like @assignment and @review_scores, and mixes business logic with view logic.

== Problem Statement ==
The tight coupling and unclear separation of concerns make the code difficult to understand, test, and manage. To improve the overall quality of the codebase, the goal of this project is to refactor to ensure that the required info is passed as parameters and to separate business logic from view-related logic.

== What Needs To Be Done ==
*''' Separation of Business Logic and Helpers.'''
** The get_data_for_review_report method in the ReviewMappingHelper module contains business logic. To improve separation of concerns, create a dedicated Review Report Service to handle calculations, allowing the helper class to focus on view-related logic.
** Similarly, the display_volume_metric_chart method includes business logic for generating chart data. A Chart Data Service should be introduced to manage these calculations, leaving the helper responsible for rendering.
** The Charts Module is tightly coupled with the helper class due to its reliance on initialize_chart_elements. To enhance modularity, a separate service object should handle chart initialization.

*'''Improving Method Clarity and Consistency'''
**Some methods use inconsistent naming conventions (determine, get, obtain). Align method names with Ruby conventions—"get" is generally preferred.
**Methods like get_data_for_review_report rely on instance variables such as @assignment, assuming they are set beforehand. To improve clarity and reduce potential errors, pass these variables as parameters instead of assuming their existence.

*'''Reducing Unnecessary Comments and Inline Styles'''
**Replace redundant comments with meaningful explanations that provide insight rather than restating what the code does.
**Remove these comments from _review_rounds.html.erb. They add no value. Also, remove any hardcoded values.
*** 
*** 
**The image_tag in _grade_and_comments_form.html.erb uses inline styles; these should be moved to a CSS stylesheet for better maintainability.
**The _reviewer_info.html.erb view also contains inline styles that should be extracted into a stylesheet.

*'''Removing Unused and Inefficient Code'''
**The ReviewMappingHelper class contains commented-out code. If this code is no longer needed, delete it.
**Identify opportunities to use guard clauses to break out of methods early, reducing unnecessary nesting and improving readability.
*'''Efficient Database Queries'''
**Several views, such as _response_maps.html.erb and _review_rounds.html.erb, execute database queries directly within the view, making multiple database calls. This logic should be moved to the model, with the Controller passing pre-fetched data to the view.
**The _reviewer_info.html.erb view accesses session data directly, which should instead be retrieved from the Controller to uphold the Separation of Concerns principle.
*'''Testing and Code Validation'''
**Ensure all new Service Classes created for report calculations, chart data, and database queries are thoroughly tested.
**Review existing tests and fix any failing tests to ensure correctness.
**Ensure full test coverage for all helper methods.
*'''Clarify Hardcoded Logic'''
**The _reviewer_info.html.erb view references reviewer.id == 1, but the significance of this ID is unclear. Either document its importance or store it in a constant that makes sense for better readability.

== Design Pattern ==
The Refactoring pattern is essential to enhance code readability, maintainability, and adherence to best practices. By systematically restructuring code components, eliminating redundancies, and applying standard conventions, the Refactoring pattern ensures improved code quality and easier future modifications.

==Plan of work==

We began our work by examining the <code>review_mapping_helper.rb</code> file to understand its connection to the rest of the codebase. We then had detailed discussions with our mentor about the project's flow and structure. We organized our project requirements by difficulty levels, assigned tasks among the team, and began refactoring existing methods and creating new files as needed. We made sure to preserve the integrity of existing test cases while adding new ones where needed.

==Implementation==

'''Issue #29:''' Removed redundant comments from <code>_review_rounds.html.erb</code>
*'''Solution:'''The solution involved cleaning up the code by removing redundant and outdated comments that were no longer necessary or too specific, such as references to team name handling and hard-coded logic for a particular instructor. The code was also slightly refactored for better readability, including consistent use of quotes in HTML attributes and ERB tags. 
*'''Changes'''
[[File:Removed_redundant_comments.jpg]]
 

----

'''Issue #24:'''Inline styles fix for <code>_grades_and_comments._form.html.erb</code> and <code>_reviewer_info.html.erb</code> 
*'''Solution:'''Removed inline styles from the image tag and added a CSS class called "text_macros". Properly closed the form_with block by adding the missing <% end %>.No major changes were made in _reviewer_info.html.erb, only some formatting improvements.. 
*'''Changes'''
 
[[File:grade_and_comments_form.jpg]]
 
[[File:reviewer_info.jpg]]
 
----

'''Issue #6:''' Move database queries to models in <code>_response_maps.html.erb</code> and <code>_review_rounds.html.erb</code>. 
*'''Solution:''' Moved database queries to models by replacing <code>Team.where(id: reviewer_map.reviewee_id).length > 0</code> with <code>Team.from_response_map(reviewer_map)</code>, encapsulating team retrieval logic in the model. Removed redundant <code>Team.find</code> calls and replaced <code>Response.exists?(map_id: reviewer_map.id)</code> with <code>reviewer_response_map.has_response?</code>. 
*'''Changes'''
[[File:Removed_redundant_comments.jpg]]
 
[[response_maps.jpg]]
 
----

'''Issue #7:''' Several methods for feedback_response_maps, check docs to manage this confusion 
*'''Methods involved:''' <code> feedback_response_for_author()</code> 
*'''Previous Code:''' previous code had an ambiguous variable name 
*'''Problem:''' The purpose of the variable was unclear. 
*'''Solution:''' Rename the variable to enhance its clarity and provide comments to explain its purpose clearly. 
*'''Changes'''
[[File:Issue7.jpg]]
 
----

==Test Plan==
This project involves refactoring, meaning that the original functionality should remain unchanged. Tasks included adding comments to existing code, altering variable names, and similar adjustments that didn't affect the test cases. When method names were modified or moved to different files, corresponding changes were made in the relevant RSpec files. All test cases passed successfully, indicating that the refactoring didn't break the existing functionality.
 

As part of this project, the following actions were taken to ensure '''test accuracy and full functionality''':
* RSpec files were updated to reflect the new service class structure.
* The test suite was run after each change to confirm that behavior remained consistent across helper and service files.
* New service classes were fully tested, achieving '''100% test coverage'''.
* The `ReviewMappingHelper` was thoroughly reviewed and maintained a high coverage rate of '''87.85%''', confirming strong existing test quality.
* All helper methods dependent on parameters like `@assignment` were validated to ensure they functioned correctly after being decoupled from instance variables.
* No failing or broken tests were introduced during or after the refactor.
* '''Test skeletons''' (review_mapping_helper_spec.rb, chart_data_service_spec.rb, and review_report_service_spec.rb) were provided by '''Nitya Naga Sai Atluri natluri@ncsu.edu'''. These were used as the foundation for validating and extending test coverage.

{| class="wikitable" style="text-align:center"
! File
! % Covered
! Lines
! Relevant Lines
! Lines Covered
! Lines Missed
! Avg. Hits/Line
|-
| app/helpers/review_mapping_helper.rb
| 87.85%
| 319
| 181
| 159
| 22
| 3.5
|-
| app/services/chart_data_service.rb
| 100.00%
| 112
| 30
| 30
| 0
| 3.9
|-
| app/services/chart_initialization_service.rb
| 100.00%
| 34
| 22
| 22
| 0
| 243.4
|-
| app/services/review_report_service.rb
| 100.00%
| 53
| 26
| 26
| 0
| 11.9
|}

File:Response maps.jpg

2025-03-25T00:39:05Z

Spatel68:

File:Reviewer info.jpg

2025-03-24T21:01:51Z

Spatel68:

File:Grade and comments form.jpg

2025-03-24T21:01:31Z

Spatel68:

CSC/ECE 517 Spring 2025 - E2501: Refactor review mapping helper.rb

2025-03-24T20:57:06Z

Spatel68:

== Team ==
=== Mentor ===
* Janice Uwujaren

=== Team Members ===
* Katerina Vilkomir (evilkom)
* Smit Patel (spatel68)
* Kenil Patel (kpatel47)

=== Relevant Links ===
* '''Link to Expertiza Repository:''' https://github.com/expertiza/expertiza 

* '''Link to Forked Repository: ''' https://github.com/Kii4ka/expertiza 

* ''' Link to Pull Request: ''' 

== Expertiza Background==
Expertiza is a Ruby on Rails framework-based open-source web application. It is kept up by NC State University employees and students. With the help of this program, instructors can fully manage the assignments and responsibilities assigned in their classes. Expertiza provides many strong features, such as subject addition, group creation, and peer review administration. It is a flexible platform that can manage many kinds of tasks. Users can consult the Expertiza wiki to get more in-depth details about the many capabilities that Expertiza offers.

== About Helper ==
The review_mapping_helper is integral to the peer review process, handling tasks such as calculating review scores, managing submission statuses, generating reports, and displaying review metrics. However, the module is complex because it depends on numerous instance variables, like @assignment and @review_scores, and mixes business logic with view logic.

== Problem Statement ==
The tight coupling and unclear separation of concerns make the code difficult to understand, test, and manage. To improve the overall quality of the codebase, the goal of this project is to refactor to ensure that the required info is passed as parameters and to separate business logic from view-related logic.

== What Needs To Be Done ==
*''' Separation of Business Logic and Helpers.'''
** The get_data_for_review_report method in the ReviewMappingHelper module contains business logic. To improve separation of concerns, create a dedicated Review Report Service to handle calculations, allowing the helper class to focus on view-related logic.
** Similarly, the display_volume_metric_chart method includes business logic for generating chart data. A Chart Data Service should be introduced to manage these calculations, leaving the helper responsible for rendering.
** The Charts Module is tightly coupled with the helper class due to its reliance on initialize_chart_elements. To enhance modularity, a separate service object should handle chart initialization.

*'''Improving Method Clarity and Consistency'''
**Some methods use inconsistent naming conventions (determine, get, obtain). Align method names with Ruby conventions—"get" is generally preferred.
**Methods like get_data_for_review_report rely on instance variables such as @assignment, assuming they are set beforehand. To improve clarity and reduce potential errors, pass these variables as parameters instead of assuming their existence.

*'''Reducing Unnecessary Comments and Inline Styles'''
**Replace redundant comments with meaningful explanations that provide insight rather than restating what the code does.
**Remove these comments from _review_rounds.html.erb. They add no value. Also, remove any hardcoded values.
*** 
*** 
**The image_tag in _grade_and_comments_form.html.erb uses inline styles; these should be moved to a CSS stylesheet for better maintainability.
**The _reviewer_info.html.erb view also contains inline styles that should be extracted into a stylesheet.

*'''Removing Unused and Inefficient Code'''
**The ReviewMappingHelper class contains commented-out code. If this code is no longer needed, delete it.
**Identify opportunities to use guard clauses to break out of methods early, reducing unnecessary nesting and improving readability.
*'''Efficient Database Queries'''
**Several views, such as _response_maps.html.erb and _review_rounds.html.erb, execute database queries directly within the view, making multiple database calls. This logic should be moved to the model, with the Controller passing pre-fetched data to the view.
**The _reviewer_info.html.erb view accesses session data directly, which should instead be retrieved from the Controller to uphold the Separation of Concerns principle.
*'''Testing and Code Validation'''
**Ensure all new Service Classes created for report calculations, chart data, and database queries are thoroughly tested.
**Review existing tests and fix any failing tests to ensure correctness.
**Ensure full test coverage for all helper methods.
*'''Clarify Hardcoded Logic'''
**The _reviewer_info.html.erb view references reviewer.id == 1, but the significance of this ID is unclear. Either document its importance or store it in a constant that makes sense for better readability.

== Design Pattern ==
The Refactoring pattern is essential to enhance code readability, maintainability, and adherence to best practices. By systematically restructuring code components, eliminating redundancies, and applying standard conventions, the Refactoring pattern ensures improved code quality and easier future modifications.

==Plan of work==

We began our work by examining the <code>review_mapping_helper.rb</code> file to understand its connection to the rest of the codebase. We then had detailed discussions with our mentor about the project's flow and structure. We organized our project requirements by difficulty levels, assigned tasks among the team, and began refactoring existing methods and creating new files as needed. We made sure to preserve the integrity of existing test cases while adding new ones where needed.

==Implementation==

'''Issue #29:''' Removed redundant comments from <code>_review_rounds.html.erb</code>
*'''Solution:'''The solution involved cleaning up the code by removing redundant and outdated comments that were no longer necessary or too specific, such as references to team name handling and hard-coded logic for a particular instructor. The code was also slightly refactored for better readability, including consistent use of quotes in HTML attributes and ERB tags. 
*'''Changes'''
[[File:Removed_redundant_comments.jpg]]
 

----

'''Issue #5:'''Inline styles fix for <code>_grades_and_comments._form.html.erb</code> and <code>_reviewer_info.html.erb</code> 
*'''Solution:'''Removed inline styles from the image tag and added a CSS class called "text_macros". Properly closed the form_with block by adding the missing <% end %>.No major changes were made in _reviewer_info.html.erb, only some formatting improvements.. 
*'''Changes'''
[[File:5_1.jpg]]t
 
[[File:Issue_Metrics5.jpg]]
 
----

'''Issue #6:''' make a separate file for for chart related elements in <code>review_mapping_helper.rb</code> 
*'''Methods involved:''' <code>display_volume_metric_chart()</code>, <code>display_tagging_interval_chart()</code> 
*'''Previous Code:''' The methods mentioned used to be included in the review_mapping_helper.rb file, but they should have been in their own helper file. 
*'''Problem:''' These techniques carry out comparable functions in creating graphs and can be implemented as either distinct modules or mixins. Additionally, each method contains more than 30 lines, making them excessively lengthy. 
*'''Solution:''' Created new module named 'Charts' withinthe same file. This way the chart helper methods are separated from review_mapping_helper where they are out of place and difficult for any developer to find. 
*'''Changes'''
[[File:Issue6.jpg]] 
[[File:Issue6.2.jpg]]
 
----

'''Issue #7:''' Several methods for feedback_response_maps, check docs to manage this confusion 
*'''Methods involved:''' <code> feedback_response_for_author()</code> 
*'''Previous Code:''' previous code had an ambiguous variable name 
*'''Problem:''' The purpose of the variable was unclear. 
*'''Solution:''' Rename the variable to enhance its clarity and provide comments to explain its purpose clearly. 
*'''Changes'''
[[File:Issue7.jpg]]
 
----

==Test Plan==
This project involves refactoring, meaning that the original functionality should remain unchanged. Tasks included adding comments to existing code, altering variable names, and similar adjustments that didn't affect the test cases. When method names were modified or moved to different files, corresponding changes were made in the relevant RSpec files. All test cases passed successfully, indicating that the refactoring didn't break the existing functionality.
 

As part of this project, the following actions were taken to ensure '''test accuracy and full functionality''':
* RSpec files were updated to reflect the new service class structure.
* The test suite was run after each change to confirm that behavior remained consistent across helper and service files.
* New service classes were fully tested, achieving '''100% test coverage'''.
* The `ReviewMappingHelper` was thoroughly reviewed and maintained a high coverage rate of '''87.85%''', confirming strong existing test quality.
* All helper methods dependent on parameters like `@assignment` were validated to ensure they functioned correctly after being decoupled from instance variables.
* No failing or broken tests were introduced during or after the refactor.
* '''Test skeletons''' (review_mapping_helper_spec.rb, chart_data_service_spec.rb, and review_report_service_spec.rb) were provided by '''Nitya Naga Sai Atluri natluri@ncsu.edu'''. These were used as the foundation for validating and extending test coverage.

{| class="wikitable" style="text-align:center"
! File
! % Covered
! Lines
! Relevant Lines
! Lines Covered
! Lines Missed
! Avg. Hits/Line
|-
| app/helpers/review_mapping_helper.rb
| 87.85%
| 319
| 181
| 159
| 22
| 3.5
|-
| app/services/chart_data_service.rb
| 100.00%
| 112
| 30
| 30
| 0
| 3.9
|-
| app/services/chart_initialization_service.rb
| 100.00%
| 34
| 22
| 22
| 0
| 243.4
|-
| app/services/review_report_service.rb
| 100.00%
| 53
| 26
| 26
| 0
| 11.9
|}

File:Removed redundant comments.jpg

2025-03-23T21:51:21Z

Spatel68: Spatel68 uploaded a new version of File:Removed redundant comments.jpg

CSC/ECE 517 Spring 2025 - E2501: Refactor review mapping helper.rb

2025-03-23T21:50:48Z

Spatel68:

== Team ==
=== Mentor ===
* Janice Uwujaren

=== Team Members ===
* Katerina Vilkomir (evilkom)
* Smit Patel (spatel68)
* Kenil Patel (kpatel47)

=== Relevant Links ===
* '''Link to Expertiza Repository:''' https://github.com/expertiza/expertiza 

* '''Link to Forked Repository: ''' https://github.com/Kii4ka/expertiza 

* ''' Link to Pull Request: ''' 

== Expertiza Background==
Expertiza is a Ruby on Rails framework-based open-source web application. It is kept up by NC State University employees and students. With the help of this program, instructors can fully manage the assignments and responsibilities assigned in their classes. Expertiza provides many strong features, such as subject addition, group creation, and peer review administration. It is a flexible platform that can manage many kinds of tasks. Users can consult the Expertiza wiki to get more in-depth details about the many capabilities that Expertiza offers.

== About Helper ==
The review_mapping_helper is integral to the peer review process, handling tasks such as calculating review scores, managing submission statuses, generating reports, and displaying review metrics. However, the module is complex because it depends on numerous instance variables, like @assignment and @review_scores, and mixes business logic with view logic.

== Problem Statement ==
The tight coupling and unclear separation of concerns make the code difficult to understand, test, and manage. To improve the overall quality of the codebase, the goal of this project is to refactor to ensure that the required info is passed as parameters and to separate business logic from view-related logic.

== What Needs To Be Done ==
*''' Separation of Business Logic and Helpers.'''
** The get_data_for_review_report method in the ReviewMappingHelper module contains business logic. To improve separation of concerns, create a dedicated Review Report Service to handle calculations, allowing the helper class to focus on view-related logic.
** Similarly, the display_volume_metric_chart method includes business logic for generating chart data. A Chart Data Service should be introduced to manage these calculations, leaving the helper responsible for rendering.
** The Charts Module is tightly coupled with the helper class due to its reliance on initialize_chart_elements. To enhance modularity, a separate service object should handle chart initialization.

*'''Improving Method Clarity and Consistency'''
**Some methods use inconsistent naming conventions (determine, get, obtain). Align method names with Ruby conventions—"get" is generally preferred.
**Methods like get_data_for_review_report rely on instance variables such as @assignment, assuming they are set beforehand. To improve clarity and reduce potential errors, pass these variables as parameters instead of assuming their existence.

*'''Reducing Unnecessary Comments and Inline Styles'''
**Replace redundant comments with meaningful explanations that provide insight rather than restating what the code does.
**Remove these comments from _review_rounds.html.erb. They add no value. Also, remove any hardcoded values.
*** 
*** 
**The image_tag in _grade_and_comments_form.html.erb uses inline styles; these should be moved to a CSS stylesheet for better maintainability.
**The _reviewer_info.html.erb view also contains inline styles that should be extracted into a stylesheet.

*'''Removing Unused and Inefficient Code'''
**The ReviewMappingHelper class contains commented-out code. If this code is no longer needed, delete it.
**Identify opportunities to use guard clauses to break out of methods early, reducing unnecessary nesting and improving readability.
*'''Efficient Database Queries'''
**Several views, such as _response_maps.html.erb and _review_rounds.html.erb, execute database queries directly within the view, making multiple database calls. This logic should be moved to the model, with the Controller passing pre-fetched data to the view.
**The _reviewer_info.html.erb view accesses session data directly, which should instead be retrieved from the Controller to uphold the Separation of Concerns principle.
*'''Testing and Code Validation'''
**Ensure all new Service Classes created for report calculations, chart data, and database queries are thoroughly tested.
**Review existing tests and fix any failing tests to ensure correctness.
**Ensure full test coverage for all helper methods.
*'''Clarify Hardcoded Logic'''
**The _reviewer_info.html.erb view references reviewer.id == 1, but the significance of this ID is unclear. Either document its importance or store it in a constant that makes sense for better readability.

== Design Pattern ==
The Refactoring pattern is essential to enhance code readability, maintainability, and adherence to best practices. By systematically restructuring code components, eliminating redundancies, and applying standard conventions, the Refactoring pattern ensures improved code quality and easier future modifications.

==Plan of work==

We began our work by examining the <code>review_mapping_helper.rb</code> file to understand its connection to the rest of the codebase. We then had detailed discussions with our mentor about the project's flow and structure. We organized our project requirements by difficulty levels, assigned tasks among the team, and began refactoring existing methods and creating new files as needed. We made sure to preserve the integrity of existing test cases while adding new ones where needed.

==Implementation==

'''Issue #29:''' Removed redundant comments from <code>_review_rounds.html.erb</code>
*'''Solution:'''The solution involved cleaning up the code by removing redundant and outdated comments that were no longer necessary or too specific, such as references to team name handling and hard-coded logic for a particular instructor. The code was also slightly refactored for better readability, including consistent use of quotes in HTML attributes and ERB tags. 
*'''Changes'''
[[File:Removed_redundant_comments.jpg]]
 

----

'''Issue #5:''' The method <code>sort_reviewer_by_review_volume_desc</code> should be generalized so that it can sort by any metric, not just review volume. Other metrics might include number of suggestions, or number of suggestions + number of problems detected. This method should not be counting the number of review rounds! Since other places in the code will need to know the number of review rounds, it should be calculated somewhere else in the system. 
*'''Methods involved:''' <code>sort_reviewer_by_review_volume_desc</code> 
*'''Problem:''' The current function, sort_reviewer_by_review_volume_desc, arranges reviewers in descending order based solely on review volume. This limited approach means it can't be applied to sorting by other metrics. 
*'''Solution:'''The idea is to create a more versatile approach by changing the name of <code>sort_reviewer_by_review_volume_desc</code> to <code>sort_reviewer_desc</code> and allowing a metric argument for sorting reviewers, as demonstrated below. This modification was also applied to the <code>review_report</code> section where it was originally used. 
*'''Changes'''
[[File:5_1.jpg]]
 
[[File:Issue_Metrics5.jpg]]
 
----

'''Issue #6:''' make a separate file for for chart related elements in <code>review_mapping_helper.rb</code> 
*'''Methods involved:''' <code>display_volume_metric_chart()</code>, <code>display_tagging_interval_chart()</code> 
*'''Previous Code:''' The methods mentioned used to be included in the review_mapping_helper.rb file, but they should have been in their own helper file. 
*'''Problem:''' These techniques carry out comparable functions in creating graphs and can be implemented as either distinct modules or mixins. Additionally, each method contains more than 30 lines, making them excessively lengthy. 
*'''Solution:''' Created new module named 'Charts' withinthe same file. This way the chart helper methods are separated from review_mapping_helper where they are out of place and difficult for any developer to find. 
*'''Changes'''
[[File:Issue6.jpg]] 
[[File:Issue6.2.jpg]]
 
----

'''Issue #7:''' Several methods for feedback_response_maps, check docs to manage this confusion 
*'''Methods involved:''' <code> feedback_response_for_author()</code> 
*'''Previous Code:''' previous code had an ambiguous variable name 
*'''Problem:''' The purpose of the variable was unclear. 
*'''Solution:''' Rename the variable to enhance its clarity and provide comments to explain its purpose clearly. 
*'''Changes'''
[[File:Issue7.jpg]]
 
----

==Test Plan==
This project involves refactoring, meaning that the original functionality should remain unchanged. Tasks included adding comments to existing code, altering variable names, and similar adjustments that didn't affect the test cases. When method names were modified or moved to different files, corresponding changes were made in the relevant RSpec files. All test cases passed successfully, indicating that the refactoring didn't break the existing functionality.
 

As part of this project, the following actions were taken to ensure '''test accuracy and full functionality''':
* RSpec files were updated to reflect the new service class structure.
* The test suite was run after each change to confirm that behavior remained consistent across helper and service files.
* New service classes were fully tested, achieving '''100% test coverage'''.
* The `ReviewMappingHelper` was thoroughly reviewed and maintained a high coverage rate of '''87.85%''', confirming strong existing test quality.
* All helper methods dependent on parameters like `@assignment` were validated to ensure they functioned correctly after being decoupled from instance variables.
* No failing or broken tests were introduced during or after the refactor.

 The image below shows the test cases are passing as well as most of the code climate issues were resolved. 
[[File:Wiki5.jpg]]

{| class="wikitable" style="text-align:center"
! File
! % Covered
! Lines
! Relevant Lines
! Lines Covered
! Lines Missed
! Avg. Hits/Line
|-
| app/helpers/review_mapping_helper.rb
| 87.85%
| 319
| 181
| 159
| 22
| 3.5
|-
| app/services/chart_data_service.rb
| 100.00%
| 112
| 30
| 30
| 0
| 3.9
|-
| app/services/chart_initialization_service.rb
| 100.00%
| 34
| 22
| 22
| 0
| 243.4
|-
| app/services/review_report_service.rb
| 100.00%
| 53
| 26
| 26
| 0
| 11.9
|}

CSC/ECE 517 Spring 2025 - E2501: Refactor review mapping helper.rb

2025-03-23T21:29:46Z

Spatel68:

== Team ==
=== Mentor ===
* Janice Uwujaren

=== Team Members ===
* Katerina Vilkomir (evilkom)
* Smit Patel (spatel68)
* Kenil Patel (kpatel47)

=== Relevant Links ===
* '''Link to Expertiza Repository:''' https://github.com/expertiza/expertiza 

* '''Link to Forked Repository: ''' https://github.com/Kii4ka/expertiza 

* ''' Link to Pull Request: ''' 

== Expertiza Background==
Expertiza is a Ruby on Rails framework-based open-source web application. It is kept up by NC State University employees and students. With the help of this program, instructors can fully manage the assignments and responsibilities assigned in their classes. Expertiza provides many strong features, such as subject addition, group creation, and peer review administration. It is a flexible platform that can manage many kinds of tasks. Users can consult the Expertiza wiki to get more in-depth details about the many capabilities that Expertiza offers.

== About Helper ==
The review_mapping_helper is integral to the peer review process, handling tasks such as calculating review scores, managing submission statuses, generating reports, and displaying review metrics. However, the module is complex because it depends on numerous instance variables, like @assignment and @review_scores, and mixes business logic with view logic.

== Problem Statement ==
The tight coupling and unclear separation of concerns make the code difficult to understand, test, and manage. To improve the overall quality of the codebase, the goal of this project is to refactor to ensure that the required info is passed as parameters and to separate business logic from view-related logic.

== What Needs To Be Done ==
*''' Separation of Business Logic and Helpers.'''
** The get_data_for_review_report method in the ReviewMappingHelper module contains business logic. To improve separation of concerns, create a dedicated Review Report Service to handle calculations, allowing the helper class to focus on view-related logic.
** Similarly, the display_volume_metric_chart method includes business logic for generating chart data. A Chart Data Service should be introduced to manage these calculations, leaving the helper responsible for rendering.
** The Charts Module is tightly coupled with the helper class due to its reliance on initialize_chart_elements. To enhance modularity, a separate service object should handle chart initialization.

*'''Improving Method Clarity and Consistency'''
**Some methods use inconsistent naming conventions (determine, get, obtain). Align method names with Ruby conventions—"get" is generally preferred.
**Methods like get_data_for_review_report rely on instance variables such as @assignment, assuming they are set beforehand. To improve clarity and reduce potential errors, pass these variables as parameters instead of assuming their existence.

*'''Reducing Unnecessary Comments and Inline Styles'''
**Replace redundant comments with meaningful explanations that provide insight rather than restating what the code does.
**Remove these comments from _review_rounds.html.erb. They add no value. Also, remove any hardcoded values.
*** 
*** 
**The image_tag in _grade_and_comments_form.html.erb uses inline styles; these should be moved to a CSS stylesheet for better maintainability.
**The _reviewer_info.html.erb view also contains inline styles that should be extracted into a stylesheet.

*'''Removing Unused and Inefficient Code'''
**The ReviewMappingHelper class contains commented-out code. If this code is no longer needed, delete it.
**Identify opportunities to use guard clauses to break out of methods early, reducing unnecessary nesting and improving readability.
*'''Efficient Database Queries'''
**Several views, such as _response_maps.html.erb and _review_rounds.html.erb, execute database queries directly within the view, making multiple database calls. This logic should be moved to the model, with the Controller passing pre-fetched data to the view.
**The _reviewer_info.html.erb view accesses session data directly, which should instead be retrieved from the Controller to uphold the Separation of Concerns principle.
*'''Testing and Code Validation'''
**Ensure all new Service Classes created for report calculations, chart data, and database queries are thoroughly tested.
**Review existing tests and fix any failing tests to ensure correctness.
**Ensure full test coverage for all helper methods.
*'''Clarify Hardcoded Logic'''
**The _reviewer_info.html.erb view references reviewer.id == 1, but the significance of this ID is unclear. Either document its importance or store it in a constant that makes sense for better readability.

== Design Pattern ==
The Refactoring pattern is essential to enhance code readability, maintainability, and adherence to best practices. By systematically restructuring code components, eliminating redundancies, and applying standard conventions, the Refactoring pattern ensures improved code quality and easier future modifications.

==Plan of work==

We began our work by examining the <code>review_mapping_helper.rb</code> file to understand its connection to the rest of the codebase. We then had detailed discussions with our mentor about the project's flow and structure. We organized our project requirements by difficulty levels, assigned tasks among the team, and began refactoring existing methods and creating new files as needed. We made sure to preserve the integrity of existing test cases while adding new ones where needed.

==Implementation==

'''Issue #22:''' Removed redundant comments from <code>_review_rounds.html.erb</code>
*'''Solution:'''The solution involved cleaning up the code by removing redundant and outdated comments that were no longer necessary or too specific, such as references to team name handling and hard-coded logic for a particular instructor. The code was also slightly refactored for better readability, including consistent use of quotes in HTML attributes and ERB tags. Additionally, the hard-coded check for a specific instructor and question ID was replaced with a more general conditional using a <code>hyperlink_question_id</code> variable, making the code more maintainable and reusable without changing its original functionality. 
*'''Changes'''
[[File:Remove_redundant_comments.jpg]]
 

----

'''Issue #5:''' The method <code>sort_reviewer_by_review_volume_desc</code> should be generalized so that it can sort by any metric, not just review volume. Other metrics might include number of suggestions, or number of suggestions + number of problems detected. This method should not be counting the number of review rounds! Since other places in the code will need to know the number of review rounds, it should be calculated somewhere else in the system. 
*'''Methods involved:''' <code>sort_reviewer_by_review_volume_desc</code> 
*'''Problem:''' The current function, sort_reviewer_by_review_volume_desc, arranges reviewers in descending order based solely on review volume. This limited approach means it can't be applied to sorting by other metrics. 
*'''Solution:'''The idea is to create a more versatile approach by changing the name of <code>sort_reviewer_by_review_volume_desc</code> to <code>sort_reviewer_desc</code> and allowing a metric argument for sorting reviewers, as demonstrated below. This modification was also applied to the <code>review_report</code> section where it was originally used. 
*'''Changes'''
[[File:5_1.jpg]]
 
[[File:Issue_Metrics5.jpg]]
 
----

'''Issue #6:''' make a separate file for for chart related elements in <code>review_mapping_helper.rb</code> 
*'''Methods involved:''' <code>display_volume_metric_chart()</code>, <code>display_tagging_interval_chart()</code> 
*'''Previous Code:''' The methods mentioned used to be included in the review_mapping_helper.rb file, but they should have been in their own helper file. 
*'''Problem:''' These techniques carry out comparable functions in creating graphs and can be implemented as either distinct modules or mixins. Additionally, each method contains more than 30 lines, making them excessively lengthy. 
*'''Solution:''' Created new module named 'Charts' withinthe same file. This way the chart helper methods are separated from review_mapping_helper where they are out of place and difficult for any developer to find. 
*'''Changes'''
[[File:Issue6.jpg]] 
[[File:Issue6.2.jpg]]
 
----

'''Issue #7:''' Several methods for feedback_response_maps, check docs to manage this confusion 
*'''Methods involved:''' <code> feedback_response_for_author()</code> 
*'''Previous Code:''' previous code had an ambiguous variable name 
*'''Problem:''' The purpose of the variable was unclear. 
*'''Solution:''' Rename the variable to enhance its clarity and provide comments to explain its purpose clearly. 
*'''Changes'''
[[File:Issue7.jpg]]
 
----

==Test Plan==
This project involves refactoring, meaning that the original functionality should remain unchanged. Tasks included adding comments to existing code, altering variable names, and similar adjustments that didn't affect the test cases. When method names were modified or moved to different files, corresponding changes were made in the relevant RSpec files. All test cases passed successfully, indicating that the refactoring didn't break the existing functionality.
 

As part of this project, the following actions were taken to ensure '''test accuracy and full functionality''':
* RSpec files were updated to reflect the new service class structure.
* The test suite was run after each change to confirm that behavior remained consistent across helper and service files.
* New service classes were fully tested, achieving '''100% test coverage'''.
* The `ReviewMappingHelper` was thoroughly reviewed and maintained a high coverage rate of '''87.85%''', confirming strong existing test quality.
* All helper methods dependent on parameters like `@assignment` were validated to ensure they functioned correctly after being decoupled from instance variables.
* No failing or broken tests were introduced during or after the refactor.

 The image below shows the test cases are passing as well as most of the code climate issues were resolved. 
[[File:Wiki5.jpg]]

{| class="wikitable" style="text-align:center"
! File
! % Covered
! Lines
! Relevant Lines
! Lines Covered
! Lines Missed
! Avg. Hits/Line
|-
| app/helpers/review_mapping_helper.rb
| 87.85%
| 319
| 181
| 159
| 22
| 3.5
|-
| app/services/chart_data_service.rb
| 100.00%
| 112
| 30
| 30
| 0
| 3.9
|-
| app/services/chart_initialization_service.rb
| 100.00%
| 34
| 22
| 22
| 0
| 243.4
|-
| app/services/review_report_service.rb
| 100.00%
| 53
| 26
| 26
| 0
| 11.9
|}

File:Removed redundant comments.jpg

2025-03-23T21:28:36Z

Spatel68:

CSC/ECE 517 Spring 2025 - E2501: Refactor review mapping helper.rb

2025-03-23T21:27:42Z

Spatel68:

== Team ==
=== Mentor ===
* Janice Uwujaren

=== Team Members ===
* Katerina Vilkomir (evilkom)
* Smit Patel (spatel68)
* Kenil Patel (kpatel47)

=== Relevant Links ===
* '''Link to Expertiza Repository:''' https://github.com/expertiza/expertiza 

* '''Link to Forked Repository: ''' https://github.com/Kii4ka/expertiza 

* ''' Link to Pull Request: ''' 

== Expertiza Background==
Expertiza is a Ruby on Rails framework-based open-source web application. It is kept up by NC State University employees and students. With the help of this program, instructors can fully manage the assignments and responsibilities assigned in their classes. Expertiza provides many strong features, such as subject addition, group creation, and peer review administration. It is a flexible platform that can manage many kinds of tasks. Users can consult the Expertiza wiki to get more in-depth details about the many capabilities that Expertiza offers.

== About Helper ==
The review_mapping_helper is integral to the peer review process, handling tasks such as calculating review scores, managing submission statuses, generating reports, and displaying review metrics. However, the module is complex because it depends on numerous instance variables, like @assignment and @review_scores, and mixes business logic with view logic.

== Problem Statement ==
The tight coupling and unclear separation of concerns make the code difficult to understand, test, and manage. To improve the overall quality of the codebase, the goal of this project is to refactor to ensure that the required info is passed as parameters and to separate business logic from view-related logic.

== What Needs To Be Done ==
*''' Separation of Business Logic and Helpers.'''
** The get_data_for_review_report method in the ReviewMappingHelper module contains business logic. To improve separation of concerns, create a dedicated Review Report Service to handle calculations, allowing the helper class to focus on view-related logic.
** Similarly, the display_volume_metric_chart method includes business logic for generating chart data. A Chart Data Service should be introduced to manage these calculations, leaving the helper responsible for rendering.
** The Charts Module is tightly coupled with the helper class due to its reliance on initialize_chart_elements. To enhance modularity, a separate service object should handle chart initialization.

*'''Improving Method Clarity and Consistency'''
**Some methods use inconsistent naming conventions (determine, get, obtain). Align method names with Ruby conventions—"get" is generally preferred.
**Methods like get_data_for_review_report rely on instance variables such as @assignment, assuming they are set beforehand. To improve clarity and reduce potential errors, pass these variables as parameters instead of assuming their existence.

*'''Reducing Unnecessary Comments and Inline Styles'''
**Replace redundant comments with meaningful explanations that provide insight rather than restating what the code does.
**Remove these comments from _review_rounds.html.erb. They add no value. Also, remove any hardcoded values.
*** 
*** 
**The image_tag in _grade_and_comments_form.html.erb uses inline styles; these should be moved to a CSS stylesheet for better maintainability.
**The _reviewer_info.html.erb view also contains inline styles that should be extracted into a stylesheet.

*'''Removing Unused and Inefficient Code'''
**The ReviewMappingHelper class contains commented-out code. If this code is no longer needed, delete it.
**Identify opportunities to use guard clauses to break out of methods early, reducing unnecessary nesting and improving readability.
*'''Efficient Database Queries'''
**Several views, such as _response_maps.html.erb and _review_rounds.html.erb, execute database queries directly within the view, making multiple database calls. This logic should be moved to the model, with the Controller passing pre-fetched data to the view.
**The _reviewer_info.html.erb view accesses session data directly, which should instead be retrieved from the Controller to uphold the Separation of Concerns principle.
*'''Testing and Code Validation'''
**Ensure all new Service Classes created for report calculations, chart data, and database queries are thoroughly tested.
**Review existing tests and fix any failing tests to ensure correctness.
**Ensure full test coverage for all helper methods.
*'''Clarify Hardcoded Logic'''
**The _reviewer_info.html.erb view references reviewer.id == 1, but the significance of this ID is unclear. Either document its importance or store it in a constant that makes sense for better readability.

== Design Pattern ==
The Refactoring pattern is essential to enhance code readability, maintainability, and adherence to best practices. By systematically restructuring code components, eliminating redundancies, and applying standard conventions, the Refactoring pattern ensures improved code quality and easier future modifications.

==Plan of work==

We began our work by examining the <code>review_mapping_helper.rb</code> file to understand its connection to the rest of the codebase. We then had detailed discussions with our mentor about the project's flow and structure. We organized our project requirements by difficulty levels, assigned tasks among the team, and began refactoring existing methods and creating new files as needed. We made sure to preserve the integrity of existing test cases while adding new ones where needed.

==Implementation==

'''Issue #22:''' Removed redundant comments from <code>_review_rounds.html.erb</code>
*'''Solution:'''The solution involved cleaning up the code by removing redundant and outdated comments that were no longer necessary or too specific, such as references to team name handling and hard-coded logic for a particular instructor. The code was also slightly refactored for better readability, including consistent use of quotes in HTML attributes and ERB tags. Additionally, the hard-coded check for a specific instructor and question ID was replaced with a more general conditional using a <code>hyperlink_question_id</code> variable, making the code more maintainable and reusable without changing its original functionality. 
*'''Changes'''
[[File:Issue4.jpg]]
 

----

'''Issue #5:''' The method <code>sort_reviewer_by_review_volume_desc</code> should be generalized so that it can sort by any metric, not just review volume. Other metrics might include number of suggestions, or number of suggestions + number of problems detected. This method should not be counting the number of review rounds! Since other places in the code will need to know the number of review rounds, it should be calculated somewhere else in the system. 
*'''Methods involved:''' <code>sort_reviewer_by_review_volume_desc</code> 
*'''Problem:''' The current function, sort_reviewer_by_review_volume_desc, arranges reviewers in descending order based solely on review volume. This limited approach means it can't be applied to sorting by other metrics. 
*'''Solution:'''The idea is to create a more versatile approach by changing the name of <code>sort_reviewer_by_review_volume_desc</code> to <code>sort_reviewer_desc</code> and allowing a metric argument for sorting reviewers, as demonstrated below. This modification was also applied to the <code>review_report</code> section where it was originally used. 
*'''Changes'''
[[File:5_1.jpg]]
 
[[File:Issue_Metrics5.jpg]]
 
----

'''Issue #6:''' make a separate file for for chart related elements in <code>review_mapping_helper.rb</code> 
*'''Methods involved:''' <code>display_volume_metric_chart()</code>, <code>display_tagging_interval_chart()</code> 
*'''Previous Code:''' The methods mentioned used to be included in the review_mapping_helper.rb file, but they should have been in their own helper file. 
*'''Problem:''' These techniques carry out comparable functions in creating graphs and can be implemented as either distinct modules or mixins. Additionally, each method contains more than 30 lines, making them excessively lengthy. 
*'''Solution:''' Created new module named 'Charts' withinthe same file. This way the chart helper methods are separated from review_mapping_helper where they are out of place and difficult for any developer to find. 
*'''Changes'''
[[File:Issue6.jpg]] 
[[File:Issue6.2.jpg]]
 
----

'''Issue #7:''' Several methods for feedback_response_maps, check docs to manage this confusion 
*'''Methods involved:''' <code> feedback_response_for_author()</code> 
*'''Previous Code:''' previous code had an ambiguous variable name 
*'''Problem:''' The purpose of the variable was unclear. 
*'''Solution:''' Rename the variable to enhance its clarity and provide comments to explain its purpose clearly. 
*'''Changes'''
[[File:Issue7.jpg]]
 
----

==Test Plan==
This project involves refactoring, meaning that the original functionality should remain unchanged. Tasks included adding comments to existing code, altering variable names, and similar adjustments that didn't affect the test cases. When method names were modified or moved to different files, corresponding changes were made in the relevant RSpec files. All test cases passed successfully, indicating that the refactoring didn't break the existing functionality.
 

As part of this project, the following actions were taken to ensure '''test accuracy and full functionality''':
* RSpec files were updated to reflect the new service class structure.
* The test suite was run after each change to confirm that behavior remained consistent across helper and service files.
* New service classes were fully tested, achieving '''100% test coverage'''.
* The `ReviewMappingHelper` was thoroughly reviewed and maintained a high coverage rate of '''87.85%''', confirming strong existing test quality.
* All helper methods dependent on parameters like `@assignment` were validated to ensure they functioned correctly after being decoupled from instance variables.
* No failing or broken tests were introduced during or after the refactor.

 The image below shows the test cases are passing as well as most of the code climate issues were resolved. 
[[File:Wiki5.jpg]]

{| class="wikitable" style="text-align:center"
! File
! % Covered
! Lines
! Relevant Lines
! Lines Covered
! Lines Missed
! Avg. Hits/Line
|-
| app/helpers/review_mapping_helper.rb
| 87.85%
| 319
| 181
| 159
| 22
| 3.5
|-
| app/services/chart_data_service.rb
| 100.00%
| 112
| 30
| 30
| 0
| 3.9
|-
| app/services/chart_initialization_service.rb
| 100.00%
| 34
| 22
| 22
| 0
| 243.4
|-
| app/services/review_report_service.rb
| 100.00%
| 53
| 26
| 26
| 0
| 11.9
|}

CSC/ECE 517 Spring 2025 - E2501: Refactor review mapping helper.rb

2025-03-23T20:59:11Z

Spatel68:

== Team ==
=== Mentor ===
* Janice Uwujaren

=== Team Members ===
* Katerina Vilkomir (evilkom)
* Smit Patel (spatel68)
* Kenil Patel (kpatel47)

=== Relevant Links ===
* '''Link to Expertiza Repository:''' https://github.com/expertiza/expertiza 

* '''Link to Forked Repository: ''' https://github.com/Kii4ka/expertiza 

* ''' Link to Pull Request: ''' 

== Expertiza Background==
Expertiza is a Ruby on Rails framework-based open-source web application. It is kept up by NC State University employees and students. With the help of this program, instructors can fully manage the assignments and responsibilities assigned in their classes. Expertiza provides many strong features, such as subject addition, group creation, and peer review administration. It is a flexible platform that can manage many kinds of tasks. Users can consult the Expertiza wiki to get more in-depth details about the many capabilities that Expertiza offers.

== About Helper ==
The review_mapping_helper is integral to the peer review process, handling tasks such as calculating review scores, managing submission statuses, generating reports, and displaying review metrics. However, the module is complex because it depends on numerous instance variables, like @assignment and @review_scores, and mixes business logic with view logic.

== Problem Statement ==
The tight coupling and unclear separation of concerns make the code difficult to understand, test, and manage. To improve the overall quality of the codebase, the goal of this project is to refactor to ensure that the required info is passed as parameters and to separate business logic from view-related logic.

== What Needs To Be Done ==
*''' Separation of Business Logic and Helpers.'''
** The get_data_for_review_report method in the ReviewMappingHelper module contains business logic. To improve separation of concerns, create a dedicated Review Report Service to handle calculations, allowing the helper class to focus on view-related logic.
** Similarly, the display_volume_metric_chart method includes business logic for generating chart data. A Chart Data Service should be introduced to manage these calculations, leaving the helper responsible for rendering.
** The Charts Module is tightly coupled with the helper class due to its reliance on initialize_chart_elements. To enhance modularity, a separate service object should handle chart initialization.

*'''Improving Method Clarity and Consistency'''
**Some methods use inconsistent naming conventions (determine, get, obtain). Align method names with Ruby conventions—"get" is generally preferred.
**Methods like get_data_for_review_report rely on instance variables such as @assignment, assuming they are set beforehand. To improve clarity and reduce potential errors, pass these variables as parameters instead of assuming their existence.

*'''Reducing Unnecessary Comments and Inline Styles'''
**Replace redundant comments with meaningful explanations that provide insight rather than restating what the code does.
**Remove these comments from _review_rounds.html.erb. They add no value. Also, remove any hardcoded values.
*** 
*** 
**The image_tag in _grade_and_comments_form.html.erb uses inline styles; these should be moved to a CSS stylesheet for better maintainability.
**The _reviewer_info.html.erb view also contains inline styles that should be extracted into a stylesheet.

*'''Removing Unused and Inefficient Code'''
**The ReviewMappingHelper class contains commented-out code. If this code is no longer needed, delete it.
**Identify opportunities to use guard clauses to break out of methods early, reducing unnecessary nesting and improving readability.
*'''Efficient Database Queries'''
**Several views, such as _response_maps.html.erb and _review_rounds.html.erb, execute database queries directly within the view, making multiple database calls. This logic should be moved to the model, with the Controller passing pre-fetched data to the view.
**The _reviewer_info.html.erb view accesses session data directly, which should instead be retrieved from the Controller to uphold the Separation of Concerns principle.
*Testing and Code Validation
**Ensure all new Service Classes created for report calculations, chart data, and database queries are thoroughly tested.
**Review existing tests and fix any failing tests to ensure correctness.
**Ensure full test coverage for all helper methods.
*'''Clarify Hardcoded Logic'''
**The _reviewer_info.html.erb view references reviewer.id == 1, but the significance of this ID is unclear. Either document its importance or store it in a constant that makes sense for better readability.

== Implementation ==

=== Phase 1 ===

==== Refactor the <code>`get_data_for_review_report`</code> method ====
[[File:t1.jpg|750px]]

==== Refactor the <code>`color-related`</code> methods ====
[[File:t2.jpg|750px]]

==== Rename methods starting with <code>`get`</code> to adhere to Ruby conventions methods ====
[[File:t3.jpg|750px]]

== Design Pattern ==
The Refactoring pattern is essential to enhance code readability, maintainability, and adherence to best practices. By systematically restructuring code components, eliminating redundancies, and applying standard conventions, the Refactoring pattern ensures improved code quality and easier future modifications.

==Plan of work==

We began our work by examining the <code>review_mapping_helper.rb</code> file to understand its connection to the rest of the codebase. We then had detailed discussions with our mentor about the project's flow and structure. We organized our project requirements by difficulty levels, assigned tasks among the team, and began refactoring existing methods and creating new files as needed. We made sure to preserve the integrity of existing test cases while adding new ones where needed.

=='''Project 4 - DESIGN DOC'''==
We have been assigned below-cited changes to do after project 3

===4. Refactoring get_awarded_review_score Method:===
* Standardize the score-calculation logic to ensure consistency and readability. 
* Break down complex calculations into smaller, more understandable components. 
* Improve method documentation and clarity to enhance understanding. 

===5. Generalizing sort_reviewer_by_review_volume_desc:===
* Modify the method to accept flexible sorting metrics, enabling customization based on various criteria. 
* Update method documentation to reflect the changes and provide usage guidelines. 

===6. Code Organization Enhancements:===
* Extract chart-generating methods into separate modules or classes for better organization and reusability. 
* Evaluate the necessity of list_review_submissions method based on insights from the Expertiza wiki and refactor accordingly. 

===7. Documentation and Clarity Improvements:===
* Review methods related to feedback_response_maps and add comments or refactor small classes to enhance clarity and documentation. 
* Ensure consistent naming conventions and adherence to coding standards throughout the codebase. 

'''This section contains changes and contributions to Project 4'''
----

'''Issue #4:''' Method <code>get_awarded_review_score</code> needs to be refactored
*'''Methods involved:''' <code>get_awarded_review_score</code> 
*'''Solution:'''After addressing Issue 2, the method was renamed to <code>determine_awarded_review_score</code> to align with Ruby conventions. Additionally, redundant instance variable settings were moved inside the loop to streamline the code. The explicit nil or -1.0 checks were eliminated and integrated into the <code>instance_variable_set</code> line. 
*'''Changes'''
[[File:Issue4.jpg]]
 

----

'''Issue #5:''' The method <code>sort_reviewer_by_review_volume_desc</code> should be generalized so that it can sort by any metric, not just review volume. Other metrics might include number of suggestions, or number of suggestions + number of problems detected. This method should not be counting the number of review rounds! Since other places in the code will need to know the number of review rounds, it should be calculated somewhere else in the system. 
*'''Methods involved:''' <code>sort_reviewer_by_review_volume_desc</code> 
*'''Problem:''' The current function, sort_reviewer_by_review_volume_desc, arranges reviewers in descending order based solely on review volume. This limited approach means it can't be applied to sorting by other metrics. 
*'''Solution:'''The idea is to create a more versatile approach by changing the name of <code>sort_reviewer_by_review_volume_desc</code> to <code>sort_reviewer_desc</code> and allowing a metric argument for sorting reviewers, as demonstrated below. This modification was also applied to the <code>review_report</code> section where it was originally used. 
*'''Changes'''
[[File:5_1.jpg]]
 
[[File:Issue_Metrics5.jpg]]
 
----

'''Issue #6:''' make a separate file for for chart related elements in <code>review_mapping_helper.rb</code> 
*'''Methods involved:''' <code>display_volume_metric_chart()</code>, <code>display_tagging_interval_chart()</code> 
*'''Previous Code:''' The methods mentioned used to be included in the review_mapping_helper.rb file, but they should have been in their own helper file. 
*'''Problem:''' These techniques carry out comparable functions in creating graphs and can be implemented as either distinct modules or mixins. Additionally, each method contains more than 30 lines, making them excessively lengthy. 
*'''Solution:''' Created new module named 'Charts' withinthe same file. This way the chart helper methods are separated from review_mapping_helper where they are out of place and difficult for any developer to find. 
*'''Changes'''
[[File:Issue6.jpg]] 
[[File:Issue6.2.jpg]]
 
----

'''Issue #7:''' Several methods for feedback_response_maps, check docs to manage this confusion 
*'''Methods involved:''' <code> feedback_response_for_author()</code> 
*'''Previous Code:''' previous code had an ambiguous variable name 
*'''Problem:''' The purpose of the variable was unclear. 
*'''Solution:''' Rename the variable to enhance its clarity and provide comments to explain its purpose clearly. 
*'''Changes'''
[[File:Issue7.jpg]]
 
----

==Test Plan==
This project involves refactoring, meaning that the original functionality should remain unchanged. Tasks included adding comments to existing code, altering variable names, and similar adjustments that didn't affect the test cases. When method names were modified or moved to different files, corresponding changes were made in the relevant RSpec files. All test cases passed successfully, indicating that the refactoring didn't break the existing functionality.
 

As part of this project, the following actions were taken to ensure '''test accuracy and full functionality''':
* RSpec files were updated to reflect the new service class structure.
* The test suite was run after each change to confirm that behavior remained consistent across helper and service files.
* New service classes were fully tested, achieving '''100% test coverage'''.
* The `ReviewMappingHelper` was thoroughly reviewed and maintained a high coverage rate of '''87.85%''', confirming strong existing test quality.
* All helper methods dependent on parameters like `@assignment` were validated to ensure they functioned correctly after being decoupled from instance variables.
* No failing or broken tests were introduced during or after the refactor.

 The image below shows the test cases are passing as well as most of the code climate issues were resolved. 
[[File:Wiki5.jpg]]

{| class="wikitable" style="text-align:center"
! File
! % Covered
! Lines
! Relevant Lines
! Lines Covered
! Lines Missed
! Avg. Hits/Line
|-
| app/helpers/review_mapping_helper.rb
| 87.85%
| 319
| 181
| 159
| 22
| 3.5
|-
| app/services/chart_data_service.rb
| 100.00%
| 112
| 30
| 30
| 0
| 3.9
|-
| app/services/chart_initialization_service.rb
| 100.00%
| 34
| 22
| 22
| 0
| 243.4
|-
| app/services/review_report_service.rb
| 100.00%
| 53
| 26
| 26
| 0
| 11.9
|}

CSC/ECE 517 Spring 2025 - E2501: Refactor review mapping helper.rb

2025-03-23T20:57:57Z

Spatel68:

== Team ==
=== Mentor ===
* Janice Uwujaren

=== Team Members ===
* Katerina Vilkomir (evilkom)
* Smit Patel (spatel68)
* Kenil Patel (kpatel47)

=== Relevant Links ===
* '''Link to Expertiza Repository:''' https://github.com/expertiza/expertiza 

* '''Link to Forked Repository: ''' https://github.com/Kii4ka/expertiza 

* ''' Link to Pull Request: ''' 

== Expertiza Background==
Expertiza is a Ruby on Rails framework-based open-source web application. It is kept up by NC State University employees and students. With the help of this program, instructors can fully manage the assignments and responsibilities assigned in their classes. Expertiza provides many strong features, such as subject addition, group creation, and peer review administration. It is a flexible platform that can manage many kinds of tasks. Users can consult the Expertiza wiki to get more in-depth details about the many capabilities that Expertiza offers.

== About Helper ==
The review_mapping_helper is integral to the peer review process, handling tasks such as calculating review scores, managing submission statuses, generating reports, and displaying review metrics. However, the module is complex because it depends on numerous instance variables, like @assignment and @review_scores, and mixes business logic with view logic.

== Problem Statement ==
The tight coupling and unclear separation of concerns make the code difficult to understand, test, and manage. To improve the overall quality of the codebase, the goal of this project is to refactor to ensure that the required info is passed as parameters and to separate business logic from view-related logic.

== What Needs To Be Done ==
* Separation of Business Logic and Helpers.
** The get_data_for_review_report method in the ReviewMappingHelper module contains business logic. To improve separation of concerns, create a dedicated Review Report Service to handle calculations, allowing the helper class to focus on view-related logic.
** Similarly, the display_volume_metric_chart method includes business logic for generating chart data. A Chart Data Service should be introduced to manage these calculations, leaving the helper responsible for rendering.
** The Charts Module is tightly coupled with the helper class due to its reliance on initialize_chart_elements. To enhance modularity, a separate service object should handle chart initialization.

*Improving Method Clarity and Consistency
**Some methods use inconsistent naming conventions (determine, get, obtain). Align method names with Ruby conventions—"get" is generally preferred.
**Methods like get_data_for_review_report rely on instance variables such as @assignment, assuming they are set beforehand. To improve clarity and reduce potential errors, pass these variables as parameters instead of assuming their existence.

*Reducing Unnecessary Comments and Inline Styles
**Replace redundant comments with meaningful explanations that provide insight rather than restating what the code does.
**Remove these comments from _review_rounds.html.erb. They add no value. Also, remove any hardcoded values.
*** 
*** 
**The image_tag in _grade_and_comments_form.html.erb uses inline styles; these should be moved to a CSS stylesheet for better maintainability.
**The _reviewer_info.html.erb view also contains inline styles that should be extracted into a stylesheet.
*Removing Unused and Inefficient Code
**The ReviewMappingHelper class contains commented-out code. If this code is no longer needed, delete it.
**Identify opportunities to use guard clauses to break out of methods early, reducing unnecessary nesting and improving readability.
Efficient Database Queries
**Several views, such as _response_maps.html.erb and _review_rounds.html.erb, execute database queries directly within the view, making multiple database calls. This logic should be moved to the model, with the Controller passing pre-fetched data to the view.
**The _reviewer_info.html.erb view accesses session data directly, which should instead be retrieved from the Controller to uphold the Separation of Concerns principle.
*Testing and Code Validation
**Ensure all new Service Classes created for report calculations, chart data, and database queries are thoroughly tested.
**Review existing tests and fix any failing tests to ensure correctness.
**Ensure full test coverage for all helper methods.
*Clarify Hardcoded Logic
**The _reviewer_info.html.erb view references reviewer.id == 1, but the significance of this ID is unclear. Either document its importance or store it in a constant that makes sense for better readability.

== Implementation ==

=== Phase 1 ===

==== Refactor the <code>`get_data_for_review_report`</code> method ====
[[File:t1.jpg|750px]]

==== Refactor the <code>`color-related`</code> methods ====
[[File:t2.jpg|750px]]

==== Rename methods starting with <code>`get`</code> to adhere to Ruby conventions methods ====
[[File:t3.jpg|750px]]

== Design Pattern ==
The Refactoring pattern is essential to enhance code readability, maintainability, and adherence to best practices. By systematically restructuring code components, eliminating redundancies, and applying standard conventions, the Refactoring pattern ensures improved code quality and easier future modifications.

==Plan of work==

We began our work by examining the <code>review_mapping_helper.rb</code> file to understand its connection to the rest of the codebase. We then had detailed discussions with our mentor about the project's flow and structure. We organized our project requirements by difficulty levels, assigned tasks among the team, and began refactoring existing methods and creating new files as needed. We made sure to preserve the integrity of existing test cases while adding new ones where needed.

=='''Project 4 - DESIGN DOC'''==
We have been assigned below-cited changes to do after project 3

===4. Refactoring get_awarded_review_score Method:===
* Standardize the score-calculation logic to ensure consistency and readability. 
* Break down complex calculations into smaller, more understandable components. 
* Improve method documentation and clarity to enhance understanding. 

===5. Generalizing sort_reviewer_by_review_volume_desc:===
* Modify the method to accept flexible sorting metrics, enabling customization based on various criteria. 
* Update method documentation to reflect the changes and provide usage guidelines. 

===6. Code Organization Enhancements:===
* Extract chart-generating methods into separate modules or classes for better organization and reusability. 
* Evaluate the necessity of list_review_submissions method based on insights from the Expertiza wiki and refactor accordingly. 

===7. Documentation and Clarity Improvements:===
* Review methods related to feedback_response_maps and add comments or refactor small classes to enhance clarity and documentation. 
* Ensure consistent naming conventions and adherence to coding standards throughout the codebase. 

'''This section contains changes and contributions to Project 4'''
----

'''Issue #4:''' Method <code>get_awarded_review_score</code> needs to be refactored
*'''Methods involved:''' <code>get_awarded_review_score</code> 
*'''Solution:'''After addressing Issue 2, the method was renamed to <code>determine_awarded_review_score</code> to align with Ruby conventions. Additionally, redundant instance variable settings were moved inside the loop to streamline the code. The explicit nil or -1.0 checks were eliminated and integrated into the <code>instance_variable_set</code> line. 
*'''Changes'''
[[File:Issue4.jpg]]
 

----

'''Issue #5:''' The method <code>sort_reviewer_by_review_volume_desc</code> should be generalized so that it can sort by any metric, not just review volume. Other metrics might include number of suggestions, or number of suggestions + number of problems detected. This method should not be counting the number of review rounds! Since other places in the code will need to know the number of review rounds, it should be calculated somewhere else in the system. 
*'''Methods involved:''' <code>sort_reviewer_by_review_volume_desc</code> 
*'''Problem:''' The current function, sort_reviewer_by_review_volume_desc, arranges reviewers in descending order based solely on review volume. This limited approach means it can't be applied to sorting by other metrics. 
*'''Solution:'''The idea is to create a more versatile approach by changing the name of <code>sort_reviewer_by_review_volume_desc</code> to <code>sort_reviewer_desc</code> and allowing a metric argument for sorting reviewers, as demonstrated below. This modification was also applied to the <code>review_report</code> section where it was originally used. 
*'''Changes'''
[[File:5_1.jpg]]
 
[[File:Issue_Metrics5.jpg]]
 
----

'''Issue #6:''' make a separate file for for chart related elements in <code>review_mapping_helper.rb</code> 
*'''Methods involved:''' <code>display_volume_metric_chart()</code>, <code>display_tagging_interval_chart()</code> 
*'''Previous Code:''' The methods mentioned used to be included in the review_mapping_helper.rb file, but they should have been in their own helper file. 
*'''Problem:''' These techniques carry out comparable functions in creating graphs and can be implemented as either distinct modules or mixins. Additionally, each method contains more than 30 lines, making them excessively lengthy. 
*'''Solution:''' Created new module named 'Charts' withinthe same file. This way the chart helper methods are separated from review_mapping_helper where they are out of place and difficult for any developer to find. 
*'''Changes'''
[[File:Issue6.jpg]] 
[[File:Issue6.2.jpg]]
 
----

'''Issue #7:''' Several methods for feedback_response_maps, check docs to manage this confusion 
*'''Methods involved:''' <code> feedback_response_for_author()</code> 
*'''Previous Code:''' previous code had an ambiguous variable name 
*'''Problem:''' The purpose of the variable was unclear. 
*'''Solution:''' Rename the variable to enhance its clarity and provide comments to explain its purpose clearly. 
*'''Changes'''
[[File:Issue7.jpg]]
 
----

==Test Plan==
This project involves refactoring, meaning that the original functionality should remain unchanged. Tasks included adding comments to existing code, altering variable names, and similar adjustments that didn't affect the test cases. When method names were modified or moved to different files, corresponding changes were made in the relevant RSpec files. All test cases passed successfully, indicating that the refactoring didn't break the existing functionality.
 

As part of this project, the following actions were taken to ensure '''test accuracy and full functionality''':
* RSpec files were updated to reflect the new service class structure.
* The test suite was run after each change to confirm that behavior remained consistent across helper and service files.
* New service classes were fully tested, achieving '''100% test coverage'''.
* The `ReviewMappingHelper` was thoroughly reviewed and maintained a high coverage rate of '''87.85%''', confirming strong existing test quality.
* All helper methods dependent on parameters like `@assignment` were validated to ensure they functioned correctly after being decoupled from instance variables.
* No failing or broken tests were introduced during or after the refactor.

 The image below shows the test cases are passing as well as most of the code climate issues were resolved. 
[[File:Wiki5.jpg]]

{| class="wikitable" style="text-align:center"
! File
! % Covered
! Lines
! Relevant Lines
! Lines Covered
! Lines Missed
! Avg. Hits/Line
|-
| app/helpers/review_mapping_helper.rb
| 87.85%
| 319
| 181
| 159
| 22
| 3.5
|-
| app/services/chart_data_service.rb
| 100.00%
| 112
| 30
| 30
| 0
| 3.9
|-
| app/services/chart_initialization_service.rb
| 100.00%
| 34
| 22
| 22
| 0
| 243.4
|-
| app/services/review_report_service.rb
| 100.00%
| 53
| 26
| 26
| 0
| 11.9
|}

CSC/ECE 517 Spring 2025 - E2501: Refactor review mapping helper.rb

2025-03-23T00:41:04Z

Spatel68: Created page with "== Team == === Mentor === * Janice Uwujaren === Team Members === * katerina Vilkomir () * Smit Patel (spatel68) * Kenil Patel () === Relevant Links === * '''Link to Expertiza Repository:''' https://github.com/expertiza/expertiza * '''Link to Forked Repository: ''' https://github.com/Kii4ka/expertiza * ''' Link to Pull Request: ''' == Expertiza Background== Expertiza is a Ruby on Rails framework-based open-source web application. It is kept up by NC S..."

== Team ==
=== Mentor ===
* Janice Uwujaren

=== Team Members ===
* katerina Vilkomir ()
* Smit Patel (spatel68)
* Kenil Patel ()

=== Relevant Links ===
* '''Link to Expertiza Repository:''' https://github.com/expertiza/expertiza 

* '''Link to Forked Repository: ''' https://github.com/Kii4ka/expertiza 

* ''' Link to Pull Request: ''' 

== Expertiza Background==
Expertiza is a Ruby on Rails framework-based open-source web application. It is kept up by NC State University employees and students. With the help of this program, instructors can fully manage the assignments and responsibilities assigned in their classes. Expertiza provides many strong features, such as subject addition, group creation, and peer review administration. It is a flexible platform that can manage many kinds of tasks. Users can consult the Expertiza wiki to get more in-depth details about the many capabilities that Expertiza offers.

== About Helper ==
The review_mapping_helper is integral to the peer review process, handling tasks such as calculating review scores, managing submission statuses, generating reports, and displaying review metrics. However, the module is complex because it depends on numerous instance variables, like @assignment and @review_scores, and mixes business logic with view logic.

== Problem Statement ==
The tight coupling and unclear separation of concerns make the code difficult to understand, test, and manage. To improve the overall quality of the codebase, the goal of this project is to refactor to ensure that the required info is passed as parameters and to separate business logic from view-related logic.

== What Needs To Be Done ==
* Separation of Business Logic and Helpers.
** The get_data_for_review_report method in the ReviewMappingHelper module contains business logic. To improve separation of concerns, create a dedicated Review Report Service to handle calculations, allowing the helper class to focus on view-related logic.
** Similarly, the display_volume_metric_chart method includes business logic for generating chart data. A Chart Data Service should be introduced to manage these calculations, leaving the helper responsible for rendering.
** The Charts Module is tightly coupled with the helper class due to its reliance on initialize_chart_elements. To enhance modularity, a separate service object should handle chart initialization.

*Improving Method Clarity and Consistency
**Some methods use inconsistent naming conventions (determine, get, obtain). Align method names with Ruby conventions—"get" is generally preferred.
**Methods like get_data_for_review_report rely on instance variables such as @assignment, assuming they are set beforehand. To improve clarity and reduce potential errors, pass these variables as parameters instead of assuming their existence.

*Reducing Unnecessary Comments and Inline Styles
**Replace redundant comments with meaningful explanations that provide insight rather than restating what the code does.
**Remove these comments from _review_rounds.html.erb. They add no value. Also, remove any hardcoded values.
***
***<!-- Later, we can create a hyperlink question type to deal with this situation.
***The image_tag in _grade_and_comments_form.html.erb uses inline styles; these should be moved to a CSS stylesheet for better maintainability.
***The _reviewer_info.html.erb view also contains inline styles that should be extracted into a stylesheet.
*Removing Unused and Inefficient Code
The ReviewMappingHelper class contains commented-out code. If this code is no longer needed, delete it.
Identify opportunities to use guard clauses to break out of methods early, reducing unnecessary nesting and improving readability.
Efficient Database Queries
Several views, such as _response_maps.html.erb and _review_rounds.html.erb, execute database queries directly within the view, making multiple database calls. This logic should be moved to the model, with the Controller passing pre-fetched data to the view.
The _reviewer_info.html.erb view accesses session data directly, which should instead be retrieved from the Controller to uphold the Separation of Concerns principle.
Testing and Code Validation
Ensure all new Service Classes created for report calculations, chart data, and database queries are thoroughly tested.
Review existing tests and fix any failing tests to ensure correctness.
Ensure full test coverage for all helper methods.
Clarify Hardcoded Logic
The _reviewer_info.html.erb view references reviewer.id == 1, but the significance of this ID is unclear. Either document its importance or store it in a constant that makes sense for better readability.

== Implementation ==

=== Phase 1 ===

==== Refactor the <code>`get_data_for_review_report`</code> method ====
[[File:t1.jpg|750px]]

==== Refactor the <code>`color-related`</code> methods ====
[[File:t2.jpg|750px]]

==== Rename methods starting with <code>`get`</code> to adhere to Ruby conventions methods ====
[[File:t3.jpg|750px]]

== Design Pattern ==
The Refactoring pattern is essential to enhance code readability, maintainability, and adherence to best practices. By systematically restructuring code components, eliminating redundancies, and applying standard conventions, the Refactoring pattern ensures improved code quality and easier future modifications.

==Plan of work==

We began our work by examining the <code>review_mapping_helper.rb</code> file to understand its connection to the rest of the codebase. We then had detailed discussions with our mentor about the project's flow and structure. We organized our project requirements by difficulty levels, assigned tasks among the team, and began refactoring existing methods and creating new files as needed. We made sure to preserve the integrity of existing test cases while adding new ones where needed.

=='''Project 4 - DESIGN DOC'''==
We have been assigned below-cited changes to do after project 3

===4. Refactoring get_awarded_review_score Method:===
* Standardize the score-calculation logic to ensure consistency and readability. 
* Break down complex calculations into smaller, more understandable components. 
* Improve method documentation and clarity to enhance understanding. 

===5. Generalizing sort_reviewer_by_review_volume_desc:===
* Modify the method to accept flexible sorting metrics, enabling customization based on various criteria. 
* Update method documentation to reflect the changes and provide usage guidelines. 

===6. Code Organization Enhancements:===
* Extract chart-generating methods into separate modules or classes for better organization and reusability. 
* Evaluate the necessity of list_review_submissions method based on insights from the Expertiza wiki and refactor accordingly. 

===7. Documentation and Clarity Improvements:===
* Review methods related to feedback_response_maps and add comments or refactor small classes to enhance clarity and documentation. 
* Ensure consistent naming conventions and adherence to coding standards throughout the codebase. 

'''This section contains changes and contributions to Project 4'''
----

'''Issue #4:''' Method <code>get_awarded_review_score</code> needs to be refactored
*'''Methods involved:''' <code>get_awarded_review_score</code> 
*'''Solution:'''After addressing Issue 2, the method was renamed to <code>determine_awarded_review_score</code> to align with Ruby conventions. Additionally, redundant instance variable settings were moved inside the loop to streamline the code. The explicit nil or -1.0 checks were eliminated and integrated into the <code>instance_variable_set</code> line. 
*'''Changes'''
[[File:Issue4.jpg]]
 

----

'''Issue #5:''' The method <code>sort_reviewer_by_review_volume_desc</code> should be generalized so that it can sort by any metric, not just review volume. Other metrics might include number of suggestions, or number of suggestions + number of problems detected. This method should not be counting the number of review rounds! Since other places in the code will need to know the number of review rounds, it should be calculated somewhere else in the system. 
*'''Methods involved:''' <code>sort_reviewer_by_review_volume_desc</code> 
*'''Problem:''' The current function, sort_reviewer_by_review_volume_desc, arranges reviewers in descending order based solely on review volume. This limited approach means it can't be applied to sorting by other metrics. 
*'''Solution:'''The idea is to create a more versatile approach by changing the name of <code>sort_reviewer_by_review_volume_desc</code> to <code>sort_reviewer_desc</code> and allowing a metric argument for sorting reviewers, as demonstrated below. This modification was also applied to the <code>review_report</code> section where it was originally used. 
*'''Changes'''
[[File:5_1.jpg]]
 
[[File:Issue_Metrics5.jpg]]
 
----

'''Issue #6:''' make a separate file for for chart related elements in <code>review_mapping_helper.rb</code> 
*'''Methods involved:''' <code>display_volume_metric_chart()</code>, <code>display_tagging_interval_chart()</code> 
*'''Previous Code:''' The methods mentioned used to be included in the review_mapping_helper.rb file, but they should have been in their own helper file. 
*'''Problem:''' These techniques carry out comparable functions in creating graphs and can be implemented as either distinct modules or mixins. Additionally, each method contains more than 30 lines, making them excessively lengthy. 
*'''Solution:''' Created new module named 'Charts' withinthe same file. This way the chart helper methods are separated from review_mapping_helper where they are out of place and difficult for any developer to find. 
*'''Changes'''
[[File:Issue6.jpg]] 
[[File:Issue6.2.jpg]]
 
----

'''Issue #7:''' Several methods for feedback_response_maps, check docs to manage this confusion 
*'''Methods involved:''' <code> feedback_response_for_author()</code> 
*'''Previous Code:''' previous code had an ambiguous variable name 
*'''Problem:''' The purpose of the variable was unclear. 
*'''Solution:''' Rename the variable to enhance its clarity and provide comments to explain its purpose clearly. 
*'''Changes'''
[[File:Issue7.jpg]]
 
----

==Test Plan==
This project involves refactoring, meaning that the original functionality should remain unchanged. Tasks included adding comments to existing code, altering variable names, and similar adjustments that didn't affect the test cases. When method names were modified or moved to different files, corresponding changes were made in the relevant RSpec files. All test cases passed successfully, indicating that the refactoring didn't break the existing functionality.
 

 The image below shows the test cases are passing as well as most of the code climate issues were resolved. 
[[File:Wiki5.jpg]]

CSC/ECE 517 Spring 2025

2025-03-23T00:20:38Z

Spatel68:

* [[CSC/ECE 517 Spring 2025 - E2504. Mentor-meeting management]]
* [[CSC/ECE 517 Spring 2025 - E2505. Testing Answer Tagging]]
* [[CSC/ECE 517 Fall 2024 - E2508. Reimplement bidding-algorithm web service]]
* [[CSC/ECE 517 Fall 2024 - E2519. Implement view for results of bidding]]
* [[CSC/ECE 517 Spring 2025 - E2522. Enhancing UI Consistency in Expertiza 1]]
* [[CSC/ECE 517 Spring 2025 - E2520. Reimplement heatgrid UI for reviews]]
* [[CSC/ECE 517 Spring 2025 - E2517. Reimplement internationalization (frontend + backend)]]
* [[CSC/ECE 517 Spring 2025 - E2523: Enhancing UI Consistency in Expertiza 2]]
* [[CSC/ECE 517 Spring 2025 - E2515: Reimplement student_teams_controller.rb]]
* [[CSC/ECE 517 Spring 2025 - E2501: Refactor review_mapping_helper.rb]]