Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removed code smells #369

Closed
wants to merge 11 commits into from
Closed

Removed code smells #369

wants to merge 11 commits into from

Conversation

Freya02
Copy link

@Freya02 Freya02 commented Nov 24, 2023

Implementation Smells

  1. Decompose Conditional
    Package: com.redis.om.spring.convert
    Class: MappingRedisOMConverter,
    Method: doReadInternal (code line 151)
    Cause of the Smell: The conditional expression entity!= null && instance != null && entity.hasIdProperty() is complex.

  2. Extract Method
    Package: com.redis.om.spring.audit
    Class: EntityAuditor
    Method: processEntity (code line 25)
    Cause of the Smell: Long method processEntity

  3. Introducing Explaining Variable
    Package: com.redis.om.spring
    Class: RedisModulesConfiguration
    Method: faceDetectionTranslator
    Cause of the Smell: confThresh is not a clear variable name.

  4. Introducing Explaining Variable
    Package: com.redis.om.spring.cuckoo
    Class: CuckooAspect
    Method: addToCuckoo
    Cause of the smell : Complex conditional statement

  5. Extract Method
    Package: com.redis.om.spring
    Class: RediSearchIndexer
    Method: findIndexFields
    Cause of the smell : Complex method smell.

Design Smells

  1. Move method/field.
    Package: com.redis.om.spring.id
    Class: ULIDIdentifierGenerator

Description:
This refactoring technique involves moving a method or a field from one class to another to improve the organization and design of the code. In this case, the getSecureRandom method and the OsTools class were moved from the ULIDIdentifierGenerator enum to a new class named SecureRandomUtils. This makes the code more modular and separates concerns related to secure random generation from the main identifier generation logic.

  1. Replace conditional with polymorphism
    Package: com.redis.om.spring.util
    Class: ObjectUtils

Description:
This technique involves replacing conditional statements (like if-else or switch) with polymorphic behavior, where each condition is represented by a separate class or method. The original code had a series of if-else statements to determine the appropriate GeoUnit based on the DistanceUnit.I introduced the DistanceUnitConverter interface, defining a common method (getGeoUnit) that represents the behavior of converting a DistanceUnit to a GeoUnit.Four concrete classes (MilesConverter, FeetConverter, KilometersConverter, MetersConverter) were created, each implementing the DistanceUnitConverter interface and encapsulating conversion logic for a specific DistanceUnit.A Map named converters is used to associate each DistanceUnit with its corresponding DistanceUnitConverter. This replaced the original if-else statements.Polymorphic calls to the getGeoUnit() method calls allows to dynamic dispatch to the appropriate converter based on the DistanceUnit.and other units.

  1. Extract Class
    Package: com.redis.om.spring
    Class: RedisJSONKeyValueAdapter

Description:
The logic for processing references and versions has been moved to dedicated classes (ReferenceProcessor and VersionProcessor). This adheres to the Single Responsibility Principle by ensuring that each class has a single reason to change and is responsible for a specific aspect of the overall functionality.These refactoring changes aim to enhance the code quality, maintainability, and adherence to good design principles.

@bsbodden
Copy link
Contributor

This PR seems pretty broken. Is this AI generated?

@bsbodden bsbodden closed this Nov 27, 2023
@Freya02
Copy link
Author

Freya02 commented Nov 27, 2023

Hi, thank you for taking time out to review my PR.
This is not AI generated code, however some code smells were partially solved and thus some parts of code may have broken. I will solve them and raise a new PR. Thank you once again!

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.

2 participants