-
-
Notifications
You must be signed in to change notification settings - Fork 37
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 standard binary serialization #433
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request involve significant updates to the serialization logic within the Joda Beans library. Key modifications include enhancements to error handling in the Changes
Warning Rate limit exceeded@jodastephen has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 25 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
src/main/java/org/joda/beans/ser/bin/MsgPackOutput.java (1)
Line range hint
204-217
: Consider documenting the removal of the performance optimisationConsider adding a comment explaining why the custom UTF-8 encoding was removed, helping future maintainers understand the historical context.
void writeString(String value) throws IOException { + // Previously used custom UTF-8 encoding for performance + // Simplified to use standard API as JVM string handling has improved var bytes = value.getBytes(UTF_8);src/test/java/org/joda/beans/ser/bin/TestSerializeStandardBin.java (1)
457-457
: Consider using a test utility for assertion chaining.The repeated pattern of
assertThatRuntimeException().isThrownBy(...)
could be extracted into a test utility method to improve readability and reduce duplication.+private void assertThrowsRuntimeException(ThrowableAssert.ThrowingCallable callable) { + assertThatRuntimeException().isThrownBy(callable); +} + @Test public void test_read_invalidFormat_sizeOneArrayAtRoot() throws IOException { // ... - assertThatRuntimeException() - .isThrownBy(() -> JodaBeanSer.COMPACT.binReader().read(bytes, FlexiBean.class)); + assertThrowsRuntimeException(() -> + JodaBeanSer.COMPACT.binReader().read(bytes, FlexiBean.class));Also applies to: 470-470, 495-495, 530-530, 548-548, 566-566, 571-572, 581-581
src/main/java/org/joda/beans/ser/bin/JodaBeanStandardBinWriter.java (3)
322-332
: RefactorgetInstance()
to avoid exception-driven control flowThe
getInstance()
method inBaseBinHandlers
uses exceptions to determine which handler to instantiate. This practice can obscure the code's intent and may catch unintended exceptions.Consider using explicit checks for class availability instead of relying on exceptions. This can be achieved using reflection or checking class existence.
-private static final BaseBinHandlers getInstance() { - try { - return new CollectBinHandlers(); - } catch (RuntimeException | LinkageError ex) { - try { - return new GuavaBinHandlers(); - } catch (RuntimeException | LinkageError ex2) { - return new BaseBinHandlers(); - } - } +private static BaseBinHandlers getInstance() { + if (/* Check if CollectBinHandlers is available */) { + return new CollectBinHandlers(); + } else if (/* Check if GuavaBinHandlers is available */) { + return new GuavaBinHandlers(); + } else { + return new BaseBinHandlers(); + } }
260-263
: Remove redundantfinal
modifier from private methodThe method
writeMetaPropertyReference
is declared asprivate static final void
. Since private methods cannot be overridden, thefinal
modifier is unnecessary.Apply this minor correction:
-private static final void writeMetaPropertyReference(JodaBeanStandardBinWriter writer, String metaTypeName) throws IOException { +private static void writeMetaPropertyReference(JodaBeanStandardBinWriter writer, String metaTypeName) throws IOException {
53-55
: Minimise use of raw types and suppressed warningsThe code uses raw types in the
ClassValue<BinHandler<Object>>
and suppresses warnings with@SuppressWarnings("rawtypes")
. While sometimes necessary, raw types can lead to unsafe code.Consider parameterising the types to avoid raw types and suppression of warnings, enhancing type safety and code clarity.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
src/main/java/org/joda/beans/ser/bin/AbstractBinReader.java
(3 hunks)src/main/java/org/joda/beans/ser/bin/JodaBeanStandardBinWriter.java
(1 hunks)src/main/java/org/joda/beans/ser/bin/MsgPackOutput.java
(1 hunks)src/test/java/org/joda/beans/ser/bin/TestSerializeStandardBin.java
(15 hunks)src/test/resources/org/joda/beans/ser/Collections2.binstr
(1 hunks)src/test/resources/org/joda/beans/ser/ImmAddress2.binstr
(1 hunks)src/test/resources/org/joda/beans/ser/ImmArrays2.binstr
(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/test/resources/org/joda/beans/ser/ImmArrays2.binstr
🧰 Additional context used
📓 Path-based instructions (4)
src/main/java/org/joda/beans/ser/bin/AbstractBinReader.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
final
unless the variable is required for use in an inner class. - Favour use of
var
keyword for type declarations.var
may 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
instanceof
expression 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.println
instead of logging - This project tends to prefer
for
loops to streams for performance reasons, however either form is acceptable.
Do not make suggestions to change between streams and for loops or vice versa.
src/main/java/org/joda/beans/ser/bin/JodaBeanStandardBinWriter.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
final
unless the variable is required for use in an inner class. - Favour use of
var
keyword for type declarations.var
may 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
instanceof
expression 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.println
instead of logging - This project tends to prefer
for
loops to streams for performance reasons, however either form is acceptable.
Do not make suggestions to change between streams and for loops or vice versa.
src/main/java/org/joda/beans/ser/bin/MsgPackOutput.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
final
unless the variable is required for use in an inner class. - Favour use of
var
keyword for type declarations.var
may 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
instanceof
expression 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.println
instead of logging - This project tends to prefer
for
loops to streams for performance reasons, however either form is acceptable.
Do not make suggestions to change between streams and for loops or vice versa.
src/test/java/org/joda/beans/ser/bin/TestSerializeStandardBin.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
final
unless the variable is required for use in an inner class. - Favour use of
var
keyword for type declarations.var
may 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
instanceof
expression safely handlesnull
, returningfalse
.
Pattern **/test/java/**/*.java
: For test code, focus on:
- Correctness of test assertions
- Test coverage of edge cases
- Clear test naming and documentation
- Encourage test methods to be package-scoped where possible
- Be more lenient with code style and minor optimisations
🔇 Additional comments (16)
src/test/resources/org/joda/beans/ser/Collections2.binstr (4)
1-5
: LGTM: Well-structured binary serialisation format
The file header and basic structure are well-organised with proper size declarations and type definitions.
58-103
: LGTM: Comprehensive interface test coverage
The interface representations are well-structured and maintain consistency with their implementation counterparts. The empty collections provide good edge case coverage.
104-123
: Verify wildcard type information preservation
Whilst empty collections are suitable for testing wildcard generics, we should verify that type information is properly preserved during deserialisation.
✅ Verification successful
Let me gather more information about the wildcard type handling in these files.
Let me check the actual test class implementation to understand how these wildcards are being tested.
Let me try a different approach to check the wildcard type implementations.
Wildcard type handling is properly implemented and tested
The codebase demonstrates robust handling of wildcard types:
- Proper type preservation through meta-properties and builder patterns
- Comprehensive coverage of bounded wildcards (
? extends T
,? extends Number
,? extends Comparable<?>
) - Type-safe serialisation ensured via immutable collections and proper null validation
- Complete test coverage with both direct property access and builder patterns
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Look for corresponding deserialisation test cases
rg -l "WildExtendsT|WildExtendsNumber|WildExtendsComparable" src/test/java/
Length of output: 226
Script:
#!/bin/bash
# Check the test cases and implementations
rg -A 10 "WildExtendsT|WildExtendsNumber|WildExtendsComparable" src/test/java/org/joda/beans/TestImmutable.java
Length of output: 515
Script:
#!/bin/bash
# Look for the test class and its implementation
ast-grep --pattern 'class ImmGuava {
$$$
}' src/test/java/org/joda/beans/sample/ImmGuava.java
Length of output: 97
Script:
#!/bin/bash
# Check the content of ImmGuava class for wildcard implementations
rg -A 5 "List<\? extends" src/test/java/org/joda/beans/sample/ImmGuava.java
rg -A 5 "Set<\? extends" src/test/java/org/joda/beans/sample/ImmGuava.java
Length of output: 12572
6-57
: Consider expanding test coverage
Whilst the current test data covers basic scenarios well, consider adding test cases for:
- Collections containing null elements
- Collections with mixed element types (where applicable)
- Empty string elements
- Maximum/minimum integer values for counts
This would help ensure robust serialisation handling across edge cases.
src/main/java/org/joda/beans/ser/bin/MsgPackOutput.java (1)
Line range hint 204-217
: Changes to string encoding look good!
The simplified approach using String.getBytes(UTF_8)
follows Java 21 best practices and maintains clean, readable code. The removal of the custom UTF-8 encoding is a sensible modernisation.
Let's verify the performance impact of this change:
✅ Verification successful
Changes to string encoding are safe and well-tested
The simplified string encoding approach using String.getBytes(UTF_8)
is well-supported by the test suite. The test file includes comprehensive serialisation tests that cover:
- Various string lengths and encodings through MessagePack format
- String arrays and nested string structures
- Boundary cases and error conditions
- Backwards compatibility with older formats
The change maintains compatibility whilst modernising the implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for performance-related tests that might need updating
# Look for performance tests related to string serialization
rg -l "(?i)(perf|performance|benchmark).*string" --type java
# Check for any existing benchmarks
fd -e java ".*Benchmark.*"
Length of output: 91
Script:
#!/bin/bash
# Let's check for any test files related to MsgPackOutput
fd -e java ".*MsgPack.*Test.*"
# Look for any usage of MsgPackOutput in tests
rg -l "MsgPackOutput" --type java
# Check if there are any existing performance or benchmark configurations
fd ".*\.(jmh|perf|benchmark).*"
# Look for any performance-related comments in MsgPackOutput
rg -i "(?:performance|optimization)" src/main/java/org/joda/beans/ser/bin/MsgPackOutput.java
Length of output: 446
Script:
#!/bin/bash
# Let's examine the test file to understand the testing coverage
cat src/test/java/org/joda/beans/ser/bin/TestSerializeStandardBin.java
# Look for string serialization tests specifically
rg -A 5 -B 5 "writeString|String.*serializ" src/test/java/org/joda/beans/ser/bin/TestSerializeStandardBin.java
Length of output: 26147
src/main/java/org/joda/beans/ser/bin/AbstractBinReader.java (3)
27-27
: LGTM!
The addition of SerIteratorFactory
import is necessary for the array handling enhancement.
213-218
: Excellent enhancement to array type handling!
The code now gracefully handles array types sent without a metatype by using the component type to create an appropriate iterator. This improves the robustness of the deserialization process.
310-310
: Good improvement to type checking!
Using isAssignableFrom
instead of direct equality makes the code more flexible by supporting String subtypes, adhering to the Liskov Substitution Principle.
src/test/java/org/joda/beans/ser/bin/TestSerializeStandardBin.java (3)
43-43
: LGTM!
The addition of the SimpleJson
import is necessary for the new test cases.
308-395
: Well-structured tests for backward compatibility.
The new test methods effectively verify that the current implementation can read old serialization formats:
test_readOldPrimitiveArrayFormat
: Tests reading array format from v2.12test_readOldStringArrayWithMetaFormat
: Tests reading string arrays with metadatatest_readOldListWithMetaFormat
: Tests reading lists with type metadata
Good practice to maintain backward compatibility and prevent breaking changes.
63-63
: Verify the updated serialization file paths.
The test methods have been updated to use new serialization file paths (e.g., Address2.binstr
, ImmAddress2.binstr
, etc.). Ensure these files exist in the test resources directory.
Also applies to: 74-74, 87-87, 104-104, 117-117
✅ Verification successful
All serialization files are present in the test resources directory
The verification confirms that all the referenced serialization files exist at the expected location src/test/resources/org/joda/beans/ser/
:
- Address2.binstr
- ImmAddress2.binstr
- ImmOptional2.binstr
- ImmArrays2.binstr
- Collections2.binstr
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of the new serialization files
echo "Checking for new serialization files..."
for file in Address2.binstr ImmAddress2.binstr ImmOptional2.binstr ImmArrays2.binstr Collections2.binstr; do
fd -t f "$file" src/test/resources/org/joda/beans/ser/
done
Length of output: 1301
src/test/resources/org/joda/beans/ser/ImmAddress2.binstr (1)
1-381
: Overall binary serialisation structure is correctly defined
The binary serialisation format appears well-structured and accurately represents the complex data model. The nested arrays and maps are appropriately organised, and the data types are correctly specified.
src/main/java/org/joda/beans/ser/bin/JodaBeanStandardBinWriter.java (4)
266-275
: Assess the necessity of the toWeakenedType
method
The comment suggests that toWeakenedType
can be removed once the parser handles ResolvedType
. Verify if this is still applicable.
Please check if the parser now supports ResolvedType
, and if so, consider refactoring or removing toWeakenedType
to simplify the codebase.
47-693
: No changes to the public Java API
After a thorough analysis, there are no changes to the public Java API in this PR. All public and protected methods on public classes remain unchanged, ensuring backward compatibility for end users.
35-39
: 🛠️ Refactor suggestion
Replace Guava collection types with Java 21 alternatives
The code imports Guava collection types such as Multimap
, Multiset
, and Table
. According to the coding guidelines, the project should only use the Java 21 API and avoid Guava. While Java 21 does not have direct equivalents for all these types, consider refactoring to eliminate the dependency on Guava where possible.
Please verify if the Guava types can be replaced with Java 21 alternatives or custom implementations.
496-499
: 🛠️ Refactor suggestion
Migrate from Guava's Optional
to java.util.Optional
The code uses Guava's Optional
, but Java provides java.util.Optional
since Java 8. To adhere to the guideline of using only the Java 21 API, replace com.google.common.base.Optional
with java.util.Optional
.
Apply the following changes:
- Remove imports of
com.google.common.base.Optional
. - Refactor
GuavaOptionalBinHandler
to accommodatejava.util.Optional
or useOptionalBinHandler
. - Update usages accordingly.
-import com.google.common.base.Optional;
...
-if (com.google.common.base.Optional.class.isAssignableFrom(type)) {
- return GuavaOptionalBinHandler.INSTANCE;
+if (Optional.class.isAssignableFrom(type)) {
+ return OptionalBinHandler.INSTANCE;
Likely invalid or redundant comment.
* Use a ClassValue map to speed up serialization * Tweak parsing to accept String where it matches expected type * Tweak parsing to use expected type to handle nested arrays * Tweak `MsgPackOutput` removing performance tweak no longer applicable
0a2dec7
to
3b7db66
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
src/main/java/org/joda/beans/ser/bin/JodaBeanStandardBinWriter.java (4)
51-52
: Consider enhancing the comment about ClassValue usage.The comment explains why ClassValue is used but could be more detailed about the performance characteristics and trade-offs compared to pattern matching.
- // why is there an ugly ClassValue setup here? - // because this is O(1) whereas switch with pattern match which is O(n) + // ClassValue provides O(1) type lookup performance through caching, + // whereas switch with pattern matching would require O(n) comparisons. + // This optimisation is crucial for serialization performance.
267-277
: Consider extracting type weakening logic to a separate class.The type weakening logic is a complex operation that could benefit from being in its own class, especially since it's marked as potentially removable in the future. This would make it easier to remove or modify when no longer needed.
Consider creating a
TypeWeakener
class:final class TypeWeakener { private static boolean isCollection(Class<?> rawType) { return LOOKUP.get(rawType).isCollection(rawType); } public static ResolvedType weaken(ResolvedType base) { for (var arg : base.getArguments()) { if (isCollection(arg.getRawType())) { return base.toRawType(); } } return base; } }
537-543
: Consider pre-sizing collections for better performance.When writing multisets, consider pre-sizing the internal collections based on the known size to avoid resizing operations.
- var entrySet = mset.entrySet(); + var entrySet = new ArrayList<>(mset.entrySet());
625-697
: Consider extracting common Optional handling logic.The Java Optional and Guava Optional handlers share similar patterns. Consider extracting the common logic to reduce code duplication.
Consider creating an abstract base class:
private abstract static class AbstractOptionalHandler<T> implements BinHandler<T> { protected abstract Object getValueOrNull(T optional); protected abstract ResolvedType getValueType(ResolvedType declaredType); protected abstract String getTypeName(); @Override public void handle( JodaBeanStandardBinWriter writer, ResolvedType declaredType, String propertyName, T opt) throws IOException { if (!isCorrectType(declaredType.getRawType())) { writeMetaPropertyReference(writer, getTypeName()); } var valueType = getValueType(declaredType); writer.writeObject(valueType, "", getValueOrNull(opt)); } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
src/main/java/org/joda/beans/ser/bin/AbstractBinReader.java
(3 hunks)src/main/java/org/joda/beans/ser/bin/JodaBeanStandardBinWriter.java
(1 hunks)src/main/java/org/joda/beans/ser/bin/MsgPackOutput.java
(1 hunks)src/test/java/org/joda/beans/ser/bin/TestSerializeStandardBin.java
(15 hunks)src/test/resources/org/joda/beans/ser/Collections2.binstr
(1 hunks)src/test/resources/org/joda/beans/ser/ImmAddress2.binstr
(1 hunks)src/test/resources/org/joda/beans/ser/ImmArrays2.binstr
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/main/java/org/joda/beans/ser/bin/AbstractBinReader.java
- src/main/java/org/joda/beans/ser/bin/MsgPackOutput.java
- src/test/java/org/joda/beans/ser/bin/TestSerializeStandardBin.java
- src/test/resources/org/joda/beans/ser/Collections2.binstr
- src/test/resources/org/joda/beans/ser/ImmAddress2.binstr
- src/test/resources/org/joda/beans/ser/ImmArrays2.binstr
🧰 Additional context used
📓 Path-based instructions (1)
src/main/java/org/joda/beans/ser/bin/JodaBeanStandardBinWriter.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
final
unless the variable is required for use in an inner class. - Favour use of
var
keyword for type declarations.var
may 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
instanceof
expression 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.println
instead of logging - This project tends to prefer
for
loops 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 (3)
src/main/java/org/joda/beans/ser/bin/JodaBeanStandardBinWriter.java (3)
279-315
: Well-designed handler interfaces.
The interface hierarchy with BinHandler and CollectionBinHandler is well-structured, making good use of default methods and providing clear extension points.
599-620
: Excellent optimization for grid serialization.
The implementation intelligently chooses between sparse and dense serialization based on grid density, which is an excellent optimization.
470-477
: Consider adding null value validation for map entries.
While null keys are validated, null values might also need validation depending on the requirements. Consider whether null values should be allowed in maps.
MsgPackOutput
removing performance tweak no longer applicableSummary by CodeRabbit
New Features
Bug Fixes
Tests
Chores