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

Refactor YAML Parsing and Management for Enhanced Readability and Maintainability #177

Closed
wants to merge 9 commits into from

Conversation

ramchaik
Copy link
Contributor

This pull request introduces significant refactoring to the YAML parsing and management components of the project, aiming to improve code organization, readability, and maintainability. The changes are structured around three main commit messages, each focusing on a different aspect of the refactoring process.

  1. Extract YamlEntryManager Class: I've extracted the YamlEntryManager class from YamlMapping to encapsulate the logic related to managing YAML entries. This encapsulation adheres to the principle of encapsulation, ensuring that entry management is handled internally within the scope of YamlMapping. This change enhances code organization and readability by isolating entry management functionality within its own class, improving modularity and maintainability by separating concerns within the YamlMapping class.

  2. Replace Conditional Logic with Polymorphism: I've implemented polymorphism to handle different YAML element types, enhancing code maintainability and extensibility. This refactoring removes switch statements from readValue and readDocument methods, delegating to specific reader classes. I introduced a YamlElementReader interface and concrete implementations for each YAML element type, which simplifies the handling of different YAML elements and makes the code more extensible.

  3. Move readSequenceElements to SequenceReader: I've encapsulated sequence reading logic within SequenceReader for better organization. This change improves maintainability and readability of YamlDocumentReader by delegating sequence-specific operations to a dedicated class.

Please review the changes and provide feedback on the improvements made to the code structure and functionality.

Checklist

Ran tests after refactors; builds successfully.

Screenshot 2024-03-28 at 1 55 24 AM

Makes the name more descriptive and readable.
By reading the name you can tell it's a hashmap
which was not clear previously.
…nternal

Extract out the deserialization of scalar values to a new method.
Make readValueInternal more modular
The method valueConvertedNumber has been renamed to parseNumber for improved clarity and consistency with naming conventions.

Enhances the readability of the codebase and better reflects the purpose of the method, which is to parse a string value into a Number object.
- Extracted the YamlEntryManager class as a private inner class within YamlMapping to encapsulate logic related to managing YAML entries.
- Enhances code organization and readability by isolating entry management functionality within its own class.
- Improves modularity and maintainability by separating concerns within the YamlMapping class.
- This refactoring adheres to the principle of encapsulation, ensuring that entry management is handled internally within the scope of YamlMapping.
…t reader

- Implemented polymorphism to handle different YAML element types, enhancing code maintainability and extensibility.
- Removed switch statements from readValue and readDocument methods, delegating to specific reader classes.
- Introduced YamlElementReader interface and concrete implementations for each YAML element type.
…ader

- Encapsulate sequence reading logic within SequenceReader for better organization.
- Improve maintainability and readability of YamlDocumentReader by delegating sequence-specific operations.
@NathanSweet
Copy link
Member

I don't think I like moving read functions to read classes. Creating an instance of a class just to call a method to do the reading doesn't seem better. It also allocates and needs to be GCed (not a big deal since reading is expected to allocate, but it's a difference). YamlDocumentReader is 200 LOC, so isn't in dire need of readability improvements.

In general this library is in maintenance mode and readability is less of a priority than it would be in active development. I'd rather not accept PRs unless they fix bugs or add features that people want.

Was this PR created by AI? Please be clear if making contributions using AI.

@ramchaik
Copy link
Contributor Author

ramchaik commented Mar 29, 2024

Firstly, thank you for taking the time to review my Pull Request.

Regarding your feedback, I completely understand your perspective on the refactor proposed for YamlDocumentReader. I acknowledge that in the context of a library in maintenance mode, readability enhancements might not be the highest priority, especially considering the overhead of creating instances and method calls.

Allow me to provide some context for this PR. It's part of an assignment for a course I'm currently undertaking, where we're tasked with refactoring open source projects. The changes proposed are aligned with specific requirements for the assignment. While the ultimate goal would be to contribute meaningfully to the project, I understand the limitations given the maintenance mode status.

Given your insights, I'm more than willing to redirect my efforts towards areas of the codebase that might benefit more from refactoring. Could you please suggest any specific classes or functionalities that you believe could use attention? I'd be more than happy to explore those areas and submit a revised PR accordingly.

@NathanSweet
Copy link
Member

I have decided YAML is basically a poor choice for any needs and I don't use it in my projects. This library exists for others who haven't figured out that YAML sucks yet. New features are driven by user needs. If you are not a user of the library and don't know what it needs for real world use cases, then you should not open PRs.

In the future please don't use AI to post issues on my projects, and especially not to perform code changes. If English is not your first language, I would rather you post less than perfect English than post text from AI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants