Merge walker class into writer #447
Conversation
* No need for a separate class anymore
📝 WalkthroughWalkthroughThe pull request involves significant changes to the JSON serialization mechanism for Joda-Beans. The Changes
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
This comment was marked as outdated.
This comment was marked as outdated.
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/main/java/org/joda/beans/ser/json/JodaBeanSimpleJsonWriter.java (4)
19-25: Encourage the use of var and handle potential reflection concerns.Good to see that these imports are arranged logically. In Java 21, favour using 'var' where it can simplify code without losing clarity. Additionally, reflectively handling arrays always has a potential risk of difficulty in maintenance. Here, everything seems correct, but ensure that accidental type confusion or class-loading constraints do not arise, especially if reflection logic becomes more complex in the future.
66-82: Leverage pattern matching for class checks (if desired).Java 21 supports pattern matching for instanceof, which safely handles null. For instance:
if (value instanceof Bean beanValue) { … }
However, since your approach relies on ClassValue and an explicit hierarchy of condition checks, this is acceptable and works efficiently for your scenario. No changes are strictly required.
162-165: Final new line approach for JSON output.Appending the newline after serialisation is often nice for readability. Ensure that, in nested usage or streaming scenarios, this behaviour does not introduce extraneous blank lines. Currently, it appears safe, but consider making it configurable in the future.
426-442: Multiset serialisation approach.Writing a multiset as nested arrays for [element, count] is clear. Just be mindful of the length of output for massive sets. This should be documented for consumers who rely on performance.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/org/joda/beans/ser/json/JodaBeanSimpleJsonWalker.java(0 hunks)src/main/java/org/joda/beans/ser/json/JodaBeanSimpleJsonWriter.java(4 hunks)
💤 Files with no reviewable changes (1)
- src/main/java/org/joda/beans/ser/json/JodaBeanSimpleJsonWalker.java
🧰 Additional context used
📓 Path-based instructions (1)
src/main/java/org/joda/beans/ser/json/JodaBeanSimpleJsonWriter.java (2)
Pattern **/*.java: - Review code using Java 21 standards, taking into account the rules defined by src/main/checkstyle/checkstyle.xml.
- Validate that code indentation uses spaces, not tabs, with an indent of multiple of 4.
- Validate that immutable local variables are not annotated with
finalunless the variable is required for use in an inner class. - Favour use of
varkeyword for type declarations.varmay also be used when the value is a castnull. - Use a coding standard where multi-line expressions have operators and tenary separators at the end of line.
- Propose changes that only use the Java 21 API, not the API of Guava.
- The pattern matching
instanceofexpression safely handlesnull, returningfalse.
Pattern **/main/java/**/*.java: - This project is mature and must provide a stable backwards-compatible public Java API.
- In the 'Walkthrough' section, you must always provide a list of up to 25 changes to the public Java API that will affect end users.
If there are no changes, you must explicitly state that there are no changes to the public Java API in this PR.
The public Java API is defined as public and protected methods on public classes, plus the filemodule-info.java.
Provide the list by deeply analysing code flow, which incudes analysing code flow through private methods and calls to Guava and Java 21.
Changes to be reported on include:- New or removed methods in the public Java API
- Changes to method return types or parameter types in the public Java API
- Changes to method behaviour in the public Java API that might affect consumers
- This project uses
System.out.printlninstead of logging - This project tends to prefer
forloops to streams for performance reasons, however either form is acceptable.
Do not make suggestions to change between streams and for loops or vice versa.
🔇 Additional comments (10)
src/main/java/org/joda/beans/ser/json/JodaBeanSimpleJsonWriter.java (10)
63-65: Observe O(1) approach correctness.
Using ClassValue is a neat approach for O(1) lookups. This is indeed a recommended Java approach to caching class-based data rather than relying on comparatively slower reflection-based solutions or maps. Ensure correct synchronisation with the rest of your pipeline if concurrency is introduced.
117-118: Validate immutability usage and var.
Fields 'settings' is properly declared final. Given that it remains unchanged and is not shared across threads, this ensures safe publication. Also check if the 'output' field might ever require re-entrancy. Currently, it is replaced each time you call 'write', so concurrency is out of scope. This is consistent.
170-176: Potential short-circuiting for null checks.
Your approach to handle null with output.writeNull() is correct. The subsequent block obtains a handler from LOOKUP. Since the code is straightforward, there is no immediate improvement required for clarity or performance.
179-188: Joda-Convert usage can throw runtime exceptions.
Ensure that the approach to handle convertible beans is safe. The fallback to writing properties individually is sensible. The try-catch block for handling conversion errors is present elsewhere. Maintain consistent error handling for all potential conversions.
262-279: Interface naming and scope.
'JsonHandler' is declared private, so it's intentionally not part of your public API. Fine for encapsulation. The default method for handleProperty() also helps keep property handling consistent. This design effectively centralises logic.
300-311: Map and Collection detection approach.
Using a chain of if-statements here is pragmatic, especially with ClassValue caching. Suggestions to adopt pattern matching wouldn't necessarily improve clarity. This code block is well-structured.
354-380: Map writing logic and null key guard.
This logic for writing map keys is correct by forcing keys to be non-null. The error message clarifies the failure. The usage of a dedicated converter for keys is also well thought out, ensuring type safety.
482-491: Hierarchy-based solution for Grid support.
Introducing a separate 'CollectJsonHandlers' build on top of 'GuavaJsonHandlers' is consistent with the sealed class approach. No complexities are evident for future expansions.
522-552: Implementation for OptionalJsonHandler is correct.
Handling optional values with an orElse(null) approach is fine, ignoring them if empty. This aligns with typical JSON omission patterns. Check that consumers do not require explicit "null" writes for empty Optionals.
555-585: Guava’s Optional handling is analogous.
Same logic for 'com.google.common.base.Optional' as standard Optional. It’s helpful for bridging older code still using Guava utilities.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor