Skip to content

Conversation

@keshvi00
Copy link

This pull request introduces six refactoring improvements to address implementation and design smells identified in the codebase. The goal is to enhance readability, modularity, and maintainability without altering core functionality. Below is a summary of the changes:

Set 1: Implementation Smell Fixes

  1. Decomposed Conditional (EmailValidator - isValidDomain)
    Smell: Complex conditional logic made it difficult to read and maintain.
    Solution: Extracted conditions into separate methods, making the logic clearer and improving readability.

  2. Extracted Method (BaseJsonValidator - registerAndMergeDiscriminator)
    Smell: Large method (registerAndMergeDiscriminator) contained multiple responsibilities.
    Solution: Extracted helper methods to improve readability and single responsibility.

  3. Renamed Long Identifier (AbsoluteIris - unicodeToASCII)
    Smell: Variable IDN_TOASCII_PRESERVES_TRAILING_DOTS was too long.
    Solution: Shortened it to IDN_KEEPS_TRAILING_DOTS while preserving its context.

Set 2: Design Smell Fixes

  1. Moved Method (BaseJsonValidator - checkDiscriminatorMatch, registerAndMergeDiscriminator)
    Smell: Core validation logic was mixed with discriminator handling, making BaseJsonValidator too large (400 lines).
    Solution: Moved discriminator-specific methods to DiscriminatorContext, reducing complexity.

  2. Replaced Conditional with Polymorphism (BaseJsonValidator - atRoot)
    Smell: The atRoot method used an if-else chain based on PathType.
    Solution: Introduced a PathStrategy interface with multiple implementations (LegacyPathStrategy, JsonPointerPathStrategy, JsonPathStrategy, DefaultPathStrategy).

  3. Pushed Down Method (ContainsValidator - validate)
    Smell: ContainsValidator mixed validation logic with annotation collection.
    Solution: Moved annotation-specific logic to a new subclass, AnnotatedContainsValidator.

@justin-tay
Copy link
Collaborator

justin-tay commented Mar 27, 2025

Not keen on this, plus you removed some test cases, and looks like you are refactoring for the sake of refactoring. If you want something to work on an automated benchmark that runs on each pull request would be nice.

It also looks like you included the 8.3 MB jar of the tool in along with your pull request.

@stevehu stevehu closed this Apr 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants