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

From Expertiza_Wiki
Jump to navigation Jump to search

Team

Mentor

  • Janice Uwujaren

Team Members

  • Kenil Patel (kpatel47)
  • Smit Patel (spatel68)
  • Katerina Vilkomir (evilkom)

Relevant Links

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.
      • <!-- Hard-coded Dr.Kidd's question to display link. -->
      • <!-- 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.


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 review_mapping_helper.rb 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 #7: Refactor Review Report Logic

  • Methods involved: get_data_for_review_report()
  • Previous Code: Business logic was embedded within ReviewMappingHelper.
  • Problem: The method contained excessive business logic, making the helper class less maintainable and harder to read.
  • Solution: Extract business logic into a new Review Report Service to keep ReviewMappingHelper clean and focused.
  • Changes



Issue #8: Refactor Chart Data Processing

  • Methods involved: display_volume_metric_chart()
  • Previous Code: Business logic was tightly coupled within the chart display function.
  • Problem: The method handled both data processing and presentation, reducing modularity and reusability.
  • Solution: Extract business logic into a separate Chart Data Service to enhance maintainability and separation of concerns.
  • Changes



Issue #9: Decouple Charts Module from Helpers

  • Methods involved: initialize_chart_elements()
  • Previous Code: Chart initialization logic was embedded within helpers.
  • Problem: The tight coupling made the module harder to maintain and extend.
  • Solution: Move chart initialization logic to a dedicated Chart Initialization Service for better modularity.
  • Changes



Issue #10: Standardize Method Names

  • Methods involved: All methods using determine, obtain, and get
  • Previous Code: Inconsistent method naming conventions using determine, obtain, and get
  • Problem: Lack of standardization in method names reduced readability and maintainability.
  • Solution: Rename all methods to use get for consistency across the codebase.
  • Changes



Issue #11: Improve Parameter Handling in get_data_for_review_report

  • Methods involved: get_data_for_review_report()
  • Previous Code: The method relied on instance variables instead of explicitly passing parameters.
  • Problem: Implicit dependency on instance variables reduced modularity and made testing more difficult.
  • Solution: Refactor the method to explicitly accept required parameters for better modularity and testability.
  • Changes



Issue #13: Improve Readability Using Guard Clauses

  • Methods involved: Various methods with deeply nested conditions
  • Previous Code: Methods contained excessive nesting, making the logic harder to follow.
  • Problem: Deeply nested conditions reduced readability and maintainability.
  • Solution: Implement guard clauses to simplify logic and improve code clarity.
  • Changes
   => Identified methods with deeply nested conditions. 


   => Refactored code to use guard clauses for better readability. 


   => Wrote tests to confirm that behavior remains unchanged. 


   => Updated wiki documentation to reflect the changes and provide justification.



Issue #29: Removed redundant comments from _review_rounds.html.erb

  • 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




Issue #24:Inline styles fix for _grades_and_comments._form.html.erb and _reviewer_info.html.erb

  • 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







Issue #25: Move database queries to models in _response_maps.html.erb and _review_rounds.html.erb.

  • Solution: Moved database queries to models by replacing Team.where(id: reviewer_map.reviewee_id).length > 0 with Team.from_response_map(reviewer_map), encapsulating team retrieval logic in the model. Removed redundant Team.find calls and replaced Response.exists?(map_id: reviewer_map.id) with reviewer_response_map.has_response?.
  • Changes






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 user.super_admin?, super_admin? is a model method defining the condition
  • Changes



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.


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
Retrieved from "https://wiki.expertiza.ncsu.edu/index.php?title=CSC/ECE_517_Spring_2025_-_E2501:_Refactor_review_mapping_helper.rb&oldid=162827"