Skip to content

Feature option tfor null safe value handling#135

Open
Lhlmlund wants to merge 86 commits intomainfrom
feature_Option_Tfor_null-safe_value_handling
Open

Feature option tfor null safe value handling#135
Lhlmlund wants to merge 86 commits intomainfrom
feature_Option_Tfor_null-safe_value_handling

Conversation

@Lhlmlund
Copy link
Copy Markdown

@Lhlmlund Lhlmlund commented Feb 24, 2025

This merge implements a generic Option type in org.fungover.breeze.control, introducing a functional approach to handling optional values. The Option hierarchy consists of:

Some – Represents a present, non-null value.
None – Represents the absence of a value (singleton instance).

This implementation follows functional programming principles, ensuring immutability, null-safety, and seamless compatibility with Java’s Optional, Either, and Try types.

Some: Represents a present, non-null value.
None: Represents the absence of a value (singleton instance).
This implementation aligns with functional programming principles, ensuring immutability, null-safety, and compatibility with Java’s Optional, Either, and Try types.

It contains all the necessary methods and Tests for 100% Coverage as well as it has IntegrationTesting, it had a stub class for TRY but was afterward replaced by the Try implementation into Main.

✅ Complete implementation of Option with all required methods.
✅ 100% test coverage, including unit tests and integration tests.
✅ Integration with Try, ensuring compatibility in OptionTryIntegrationTest.java.
✅ Removed the temporary stub Try class after merging the proper Try implementation into main.

*** OBS ***

✅ Made Try.Success & Try.Failure public to allow access in integration tests.

*** OBS ***

previous PR HERE : ❌ -> #120

After running into some rebasing problems I had to remake this pull request

Summary by CodeRabbit

Summary by CodeRabbit

  • New Features

    • Introduced functional programming constructs to handle the presence or absence of values safely, offering a robust, null-free approach to manage data.
    • Added a new Option class along with Some and None implementations for better handling of optional values.
  • Documentation

    • Added comprehensive package documentation with clear usage examples to assist with adopting the new optional value handling paradigm.
  • Tests

    • Expanded unit and integration test coverage for Option, Some, and None classes to ensure reliable and predictable behavior across various value scenarios.
  • Style

    • Made minor formatting improvements for consistency and clearer error handling.

- Added a null check to prevent `null` values.
- Throws `NullPointerException` if the input is `null`.
- method provides a convenient way to construct `Some<T>` safely.
- Documented the Option<T> class to explain its purpose and behavior.
- and `of(T value)` to clarify its null-handling behavior.
- and `some(T value)` to enforce non-null constraints.
- Implemented `isDefined()` to determine if the Option contains a value.

- Added `get()` to retrieve the contained value, throwing an exception if None.
- Added `getOrElse(T other)` to return the contained value or a default.
- Added `getOrElseGet(Supplier<? extends T> supplier)` for lazy default computation.
- Added `getOrNull()` to return the contained value or null.

Added retrieval methods for Some and None for all above

Added SupressWarning on 'getInstance
 in 'None'
- Added flatMap to apply a function returning an Option.
- Implemented filter to conditionally return Some or None.
- Introduced forEach to execute an action if a value is present.
- Added peek for inspecting values without modifying the Option.
- Implemented fold to handle both Some and None cases.
- Ensured None<T> correctly returns defaults or does nothing where appropriate.
- Added Javadoc for clarity and maintainability.
- Updated 'Some<T>map' to return None if the mapper produces a null.

- and 'Some<T>flatMap' to return None if a function returns null
- Clarified that isEmpty() checks for None and is not the same as Optional.isEmpty().
-emphasized that the supplier is only called if the Option is None.
- Highlighted functional use cases
- Added a NullPointerException note for clarity
-clarified functional pattern-matching use cases, and defined behavior for Some and None cases.

- added a @throws NullPointerException for null suppliers and functions.
- Expanded documentation to clarify 'filter()' behaviour for Some<T> and None<T>

- Included an example showcasing the filter logic with Some and None

_ Also specified that filter does nothing for None<T> and returns None if the predicate fails

_ Also a note on 'NullpointerException' if a null predicate is provided
…T>, and None<T>.

Ensured Option<T> and its subclasses are Serializable.
- Documented `equals()`, `hashCode()`, and `toString()` methods.

- Clarified equality rules for `Some<T>` and `None<T>`.
- Explained hash code behavior and singleton nature of `None<T>`.
- Provided detailed format descriptions for `toString()`.
- Mentioned `Serializable` compliance in class-level Javadoc.
…lPointerException if the action is null, added example case
…ion if the action is null, improved the javadoc with examples for clarity
- Added `Objects.requireNonNull()`

- added functional correctness in handling optional values.
- Some<T>: Removed unnecessary null check on 'getOrElse'(T other) as it should have a value

- Added `Objects.requireNonNull` to `getOrElseGet to enforce null safety.
…Some.map to better clarify the intent to disallow null
"Cannot create Some with null value. Use None instead.");

-altered and added Javadoc for None.java
…hashCode, due to it having the same hashcode on the same instance
- Updated `None.toTry(Supplier<Exception>)`:

- Fixed `orElseThrow` implementation in `Some`:
  - Now correctly returns `T` instead of `void`, ensuring expected behavior.

  - Adjusted integration tests:
    - `noneToTryWithNullExceptionFromSupplierShouldThrow` now correctly expects
      `NullPointerException` when the supplier returns `null`.
  - Existence Checks
  - Retrieval Methods
  - Transformation Methods (map, flatMap)
  - Filtering Behavior
  - Collection & Stream Conversion
  - Either Conversion
  - Equality & Hashcode
  - String Representation
  - Exception Handling
  - Extreme Value Handling
  - Edge Cases
  - None Creation & Basic Properties
  - Retrieval Methods & Default Values
  - Transformation Methods
  - Equality and Identity
  - Collection & Conversion
  - Either Conversion & Exception Handling
  - Serialization & Singleton Property
  - Fold Behavior
- Creation & Presence Checks
- Retrieval & Transformation
- Serialization & Conversion
- Verified `Option.some` converts to `Either.Right` with correct value.

- Verified `Option.none` converts to `Either.Left` with expected fallback.

- Ensured `toEither` throws `NullPointerException` when supplier is null.

- Ensured `toEither` throws `NullPointerException` when supplier returns null.
…n<R>

Returns Some<R> if the Either is right; otherwise, returns None.

Added Javadoc
…correctly converts a right Either into Some <R>

Added leftToOptionShouldReturnNone to ensure a left Either is converted into None.
- Added `toOptionShouldReturnSameInstance` to verify `toOption()` returns the same instance.
- Added `someShouldThrowIfValueIsNull` to ensure `Option.some(null)` throws `NullPointerException`.
- Added `foldShouldReturnMappedValueForSome` to test `fold()` behavior on `Some`.
- Added `foldShouldThrowIfNoneSupplierIsNull` and `foldShouldThrowIfPresentFunctionIsNull` to check `NullPointerException` cases for `fold()`.
- Added `peekShouldPerformActionAndReturnSameInstance` to verify `peek()` executes action and returns the same instance.
- Added `peekShouldThrowIfActionIsNull` to check `NullPointerException` for `peek()`.
- Added `forEachShouldExecuteConsumer` to ensure `forEach()` executes the provided consumer.
- Added `orElseThrowShouldReturnValueForSome` to confirm `orElseThrow()` returns value.
- Added `orElseThrowShouldThrowIfSupplierIsNull` to verify `NullPointerException` when supplier is `null`.
- Added `orElseThrowShouldDoNothingForSome` to ensure `orElseThrow()` does not throw for `Some`.
- Verified `toString()` formatting in `toStringShouldReturnFormattedStringForSome`.
- Ensured `None.toString()` correctly returns `"None()"`.
- Added `getOrNullShouldReturnNullForNone` to validate `None` returns `null`.
- More extensive javadoc on Some.getOrElse & None.toEither
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Feb 24, 2025

Walkthrough

This pull request introduces a new functional programming abstraction for handling optional values. It adds an abstract Option<T> class along with its two concrete implementations: Some<T> for present values and None<T> (implemented as a singleton) for absent values. Additionally, package documentation is provided via package-info.java. Minor formatting improvements are applied to the Try class in the funclib package. A comprehensive suite of new unit and integration tests validates the behavior, conversions, and error handling of the new Option type system.

Changes

File(s) Change Summary
src/main/java/org/fungover/breeze/control/(Option.java, None.java, Some.java, package-info.java) Introduces the core functional constructs for optional values, including an abstract Option class, concrete implementations (Some and None), and package documentation.
src/main/java/org/fungover/breeze/funclib/control/Try.java Applies minor formatting adjustments for consistent indentation and style in the Try class.
src/test/java/org/fungover/breeze/control/(NoneTest.java, OptionEitherIntegrationTest.java, OptionTest.java, OptionTryIntegrationTest.java, SomeTest.java) Adds comprehensive unit and integration tests covering creation, transformation, conversion, and error handling for the Option type system.

Possibly related PRs

Suggested labels

feature

Suggested reviewers

  • voffie
  • kappsegla
  • Stelle83

Poem

I'm a little rabbit in a field of code,
Hopping over bugs on every road.
Option, Some, and None now stand tall,
Ensuring values are managed for all.
With tests in place and logic so neat,
I celebrate these changes with a joyful beat!


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d9ac34d and fd38de9.

📒 Files selected for processing (1)
  • src/main/java/org/fungover/breeze/funclib/control/Try.java (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/main/java/org/fungover/breeze/funclib/control/Try.java

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (9)
src/main/java/org/fungover/breeze/control/Some.java (1)

172-197: Be cautious with unchecked casts in map.

Reusing the same instance when the mapper returns the original object is an interesting optimization, but it relies on the assumption that the returned type is safely castable to U. This might introduce subtle class cast risks if future changes weaken the type constraints or reuse the same instance incorrectly.

src/test/java/org/fungover/breeze/control/SomeTest.java (3)

14-311: Consider using nested test classes for better organization.

The test suite is comprehensive but could be more organized using JUnit 5's @Nested feature to group related tests. This would improve readability and maintainability.

Example refactor for one section:

 class SomeTest {
+    @Nested
+    class BasicProperties {
+        @Test
+        void someIsDefined() {
+            Some<Integer> some = new Some<>(5);
+            assertTrue(some.isDefined());
+        }
+
+        @Test
+        void someIsNotEmpty() {
+            Some<Integer> some = new Some<>(5);
+            assertFalse(some.isEmpty());
+        }
+    }

69-76: Consider using a more focused test approach.

The test uses a mutable array to capture values. Consider using AssertJ's event assertions or a more functional approach.

-    void forEachShouldExecuteConsumer() {
-        Option<Integer> some = Option.some(10);
-        final int[] capturedValue = {0};
-
-        some.forEach(val -> capturedValue[0] = val);
-
-        assertThat(capturedValue[0]).isEqualTo(10);
+    void forEachShouldExecuteConsumer() {
+        Option<Integer> some = Option.some(10);
+        List<Integer> captured = new ArrayList<>();
+        
+        some.forEach(captured::add);
+        
+        assertThat(captured).containsExactly(10);
     }

258-274: Add more edge cases for numeric types.

While the test suite covers some extreme values, it could benefit from additional edge cases.

Consider adding tests for:

@Test
void someHandlesInfinity() {
    Some<Double> some = new Some<>(Double.POSITIVE_INFINITY);
    assertThat(some.get()).isEqualTo(Double.POSITIVE_INFINITY);
}

@Test
void someHandlesNegativeInfinity() {
    Some<Double> some = new Some<>(Double.NEGATIVE_INFINITY);
    assertThat(some.get()).isEqualTo(Double.NEGATIVE_INFINITY);
}
src/test/java/org/fungover/breeze/control/NoneTest.java (1)

82-91: Consider using parameterized tests for transformation methods.

The map and flatMap tests could be combined into a parameterized test to reduce duplication.

+    @ParameterizedTest
+    @MethodSource("transformationMethods")
+    void transformationMethodsShouldReturnNoneForNone(
+            String methodName,
+            Function<Option<Integer>, Option<Integer>> transformation
+    ) {
+        Option<Integer> none = Option.none();
+        assertThat(transformation.apply(none)).isSameAs(Option.none());
+    }
+
+    private static Stream<Arguments> transformationMethods() {
+        return Stream.of(
+            Arguments.of("map", (Option<Integer> opt) -> opt.map(x -> x * 2)),
+            Arguments.of("flatMap", (Option<Integer> opt) -> opt.flatMap(x -> Option.some(x * 2)))
+        );
+    }
-    @Test
-    void mapShouldReturnNoneForNone() {
-        Option<Integer> none = Option.none();
-        assertEquals(Option.none(), none.map(x -> x * 2));
-    }
-
-    @Test
-    void flatMapShouldReturnNoneForNone() {
-        Option<Integer> none = Option.none();
-        assertEquals(Option.none(), none.flatMap(x -> Option.some(x * 2)));
-    }
src/main/java/org/fungover/breeze/control/Option.java (1)

30-30: Consider relaxing the <T extends Serializable> restriction.

Requiring all potential types to implement Serializable may limit the usability of this Option class in scenarios where serialization isn't mandatory. Unless there's a strong architectural requirement for serialization, consider allowing a broader type parameter (e.g., Option<T> without extends Serializable).

src/main/java/org/fungover/breeze/control/None.java (3)

70-72: Keep or unify equality checks for better clarity.

equals now relies on obj instanceof None. While this is correct since you treat all None instances as equal, consider javadoc clarifications or references to the singleton pattern so maintainers don’t mistake it for an oversight.


107-117: Consider throwing NoSuchElementException instead of UnsupportedOperationException.

When calling get() on None, throwing a NoSuchElementException may better align with typical optional semantics in Java (like Optional.get()). This is purely stylistic but could improve clarity for developers who expect that exception.

Suggested change:

-throw new UnsupportedOperationException("Cannot call get() on None");
+throw new NoSuchElementException("Cannot call get() on None");

317-320: Consider referencing NoSuchElementException as well.

orElseThrow() in None uses UnsupportedOperationException. For consistency with typical option/optional semantics, you might unify the exception type with get(). If you decide to keep UnsupportedOperationException, ensure it’s documented why it differs from typical Java optional behavior.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cdfad16 and 91258ae.

📒 Files selected for processing (10)
  • src/main/java/org/fungover/breeze/control/None.java (1 hunks)
  • src/main/java/org/fungover/breeze/control/Option.java (1 hunks)
  • src/main/java/org/fungover/breeze/control/Some.java (1 hunks)
  • src/main/java/org/fungover/breeze/control/package-info.java (1 hunks)
  • src/main/java/org/fungover/breeze/funclib/control/Try.java (3 hunks)
  • src/test/java/org/fungover/breeze/control/NoneTest.java (1 hunks)
  • src/test/java/org/fungover/breeze/control/OptionEitherIntegrationTest.java (1 hunks)
  • src/test/java/org/fungover/breeze/control/OptionTest.java (1 hunks)
  • src/test/java/org/fungover/breeze/control/OptionTryIntegrationTest.java (1 hunks)
  • src/test/java/org/fungover/breeze/control/SomeTest.java (1 hunks)
✅ Files skipped from review due to trivial changes (2)
  • src/main/java/org/fungover/breeze/funclib/control/Try.java
  • src/main/java/org/fungover/breeze/control/package-info.java
🧰 Additional context used
🪛 GitHub Actions: Java CI with Maven
src/test/java/org/fungover/breeze/control/OptionTryIntegrationTest.java

[error] 22-22: org.fungover.breeze.funclib.control.Try.Success is not public in org.fungover.breeze.funclib.control.Try; cannot be accessed from outside package


[error] 31-31: org.fungover.breeze.funclib.control.Try.Failure is not public in org.fungover.breeze.funclib.control.Try; cannot be accessed from outside package


[error] 42-42: org.fungover.breeze.funclib.control.Try.Failure is not public in org.fungover.breeze.funclib.control.Try; cannot be accessed from outside package


[error] 52-52: org.fungover.breeze.funclib.control.Try.Success is not public in org.fungover.breeze.funclib.control.Try; cannot be accessed from outside package


[error] 61-61: org.fungover.breeze.funclib.control.Try.Failure is not public in org.fungover.breeze.funclib.control.Try; cannot be accessed from outside package


[error] 82-82: org.fungover.breeze.funclib.control.Try.Success is not public in org.fungover.breeze.funclib.control.Try; cannot be accessed from outside package

🔇 Additional comments (10)
src/main/java/org/fungover/breeze/control/Some.java (2)

28-28: Good use of final for immutability.

Marking the class as final ensures it cannot be subclassed, which aligns with functional programming principles of immutability.


52-54: Nice null safety in the constructor.

Using Objects.requireNonNull provides clear and immediate handling of invalid input, reinforcing the intent that Some must always hold a valid non-null value.

src/test/java/org/fungover/breeze/control/OptionEitherIntegrationTest.java (2)

12-19: Test successfully checks Some -> Either.right conversion.

Verifying that Option.some(...) correctly converts to Either.Right with the expected value ensures coverage for the "happy path."


38-44: Good validation for null left value in supplier.

Checking that a NullPointerException is thrown when the supplier returns null confirms robust error handling in the toEither method.

src/test/java/org/fungover/breeze/control/OptionTest.java (2)

82-101: Comprehensive serialization test coverage.

Verifying that both Some and None can be serialized and deserialized confirms correct handling of state integrity across I/O boundaries.


115-119: Appropriate exception handling test for null value creation.

Confirming that Option.some(null) throws a NullPointerException enforces the contract that a non-empty option must never hold a null reference.

src/main/java/org/fungover/breeze/control/Option.java (3)

93-96: Ensure consistent usage between some(...) and of(...).

Both some(...) and of(...) serve similar purposes, but differ in their treatment of null. Ensure that all call sites in your code clearly reflect the intent:

  • Use some(...) strictly when you expect a non-null value.
  • Use of(...) when the value may be null and should produce None.

203-204: Double-check exception supplier usage.

orElseThrow(Supplier<? extends X> exceptionSupplier) throws an exception created by the supplier on None. If there's a possibility of returning null from the supplier, it may cause unexpected behavior. Confirm that all exception suppliers in the codebase produce a valid exception instance.


290-290: Minor note on leftValue null check.

You already check for null in leftValue after calling leftSupplier.get(), correctly throwing a NullPointerException. This ensures no invalid Either.left(null) is returned, which is good defensive coding. No changes needed here.

src/main/java/org/fungover/breeze/control/None.java (1)

333-344: Good fallback exception logic.

Your fallback usage of NoSuchElementException when exceptionSupplier is null or returns null is a thoughtful approach to ensure predictable failure modes. It helps preserve consistent semantics in the absence of a supplied exception.

Comment on lines +312 to +317
/**
* Returns the wrapped value of this {@link Some<T>}.
*/
@Override
public void orElseThrow() {
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Mismatch between documentation and return type in orElseThrow().

The Javadoc states it should “return the wrapped value,” but the method signature is void and contains no return statement. Consider one of the following fixes:
• Update the documentation to clarify that this method is a no-op for Some.
• Change the method signature to return the contained value, matching the documentation.

Example fix:

-/**
- * Returns the wrapped value of this {@link Some<T>}.
- */
+/**
+ * No-op implementation for {@code Some}, since the value is always present.
+ */
 public void orElseThrow() {
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
/**
* Returns the wrapped value of this {@link Some<T>}.
*/
@Override
public void orElseThrow() {
}
/**
* No-op implementation for {@code Some}, since the value is always present.
*/
@Override
public void orElseThrow() {
}

Comment on lines +22 to +23
assertThat(result).isInstanceOf(Try.Success.class);
assertThat(result.get()).isEqualTo(42);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix access to Try's inner classes.

The pipeline is failing because Try.Success and Try.Failure are not public. Instead of checking the specific implementation class, test the behavior.

-        assertThat(result).isInstanceOf(Try.Success.class);
+        assertThat(result.isSuccess()).isTrue();
         assertThat(result.get()).isEqualTo(42);

-        assertThat(result).isInstanceOf(Try.Failure.class);
+        assertThat(result.isFailure()).isTrue();

Also applies to: 31-32, 42-43, 52-53, 61-62, 82-83

🧰 Tools
🪛 GitHub Actions: Java CI with Maven

[error] 22-22: org.fungover.breeze.funclib.control.Try.Success is not public in org.fungover.breeze.funclib.control.Try; cannot be accessed from outside package

Comment on lines +186 to +201
@Test
@SuppressWarnings("unchecked")
void serializationShouldPreserveSingletonProperty() throws IOException, ClassNotFoundException {
Option<Integer> none = Option.none();

ByteArrayOutputStream baos = new ByteArrayOutputStream();
ObjectOutputStream oos = new ObjectOutputStream(baos);
oos.writeObject(none);
oos.close();

ByteArrayInputStream bais = new ByteArrayInputStream(baos.toByteArray());
ObjectInputStream ois = new ObjectInputStream(bais);
Option<Integer> deserializedNone = (Option<Integer>) ois.readObject();

assertThat(deserializedNone).isSameAs(none);
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve resource handling in serialization test.

The test should use try-with-resources to ensure proper cleanup of I/O resources.

     @Test
     @SuppressWarnings("unchecked")
     void serializationShouldPreserveSingletonProperty() throws IOException, ClassNotFoundException {
         Option<Integer> none = Option.none();
 
-        ByteArrayOutputStream baos = new ByteArrayOutputStream();
-        ObjectOutputStream oos = new ObjectOutputStream(baos);
-        oos.writeObject(none);
-        oos.close();
+        byte[] serialized;
+        try (ByteArrayOutputStream baos = new ByteArrayOutputStream();
+             ObjectOutputStream oos = new ObjectOutputStream(baos)) {
+            oos.writeObject(none);
+            serialized = baos.toByteArray();
+        }
 
-        ByteArrayInputStream bais = new ByteArrayInputStream(baos.toByteArray());
-        ObjectInputStream ois = new ObjectInputStream(bais);
-        Option<Integer> deserializedNone = (Option<Integer>) ois.readObject();
+        Option<Integer> deserializedNone;
+        try (ByteArrayInputStream bais = new ByteArrayInputStream(serialized);
+             ObjectInputStream ois = new ObjectInputStream(bais)) {
+            deserializedNone = (Option<Integer>) ois.readObject();
+        }
 
         assertThat(deserializedNone).isSameAs(none);
     }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
@Test
@SuppressWarnings("unchecked")
void serializationShouldPreserveSingletonProperty() throws IOException, ClassNotFoundException {
Option<Integer> none = Option.none();
ByteArrayOutputStream baos = new ByteArrayOutputStream();
ObjectOutputStream oos = new ObjectOutputStream(baos);
oos.writeObject(none);
oos.close();
ByteArrayInputStream bais = new ByteArrayInputStream(baos.toByteArray());
ObjectInputStream ois = new ObjectInputStream(bais);
Option<Integer> deserializedNone = (Option<Integer>) ois.readObject();
assertThat(deserializedNone).isSameAs(none);
}
@Test
@SuppressWarnings("unchecked")
void serializationShouldPreserveSingletonProperty() throws IOException, ClassNotFoundException {
Option<Integer> none = Option.none();
byte[] serialized;
try (ByteArrayOutputStream baos = new ByteArrayOutputStream();
ObjectOutputStream oos = new ObjectOutputStream(baos)) {
oos.writeObject(none);
serialized = baos.toByteArray();
}
Option<Integer> deserializedNone;
try (ByteArrayInputStream bais = new ByteArrayInputStream(serialized);
ObjectInputStream ois = new ObjectInputStream(bais)) {
deserializedNone = (Option<Integer>) ois.readObject();
}
assertThat(deserializedNone).isSameAs(none);
}

efpcode
efpcode previously approved these changes Feb 24, 2025
Copy link
Copy Markdown
Contributor

@efpcode efpcode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think everything looks great, as stated before the code is clean and easy to follow.

- updated OptionTryIntegrationTest, Replaced direct instanceof checks for Try.Success and Try.Failure
with behavior-based assertions
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a 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

🧹 Nitpick comments (4)
src/test/java/org/fungover/breeze/control/OptionTryIntegrationTest.java (4)

7-10: Remove unused imports.

The following imports from java.io package are not used in the test class:

  • ByteArrayInputStream
  • ByteArrayOutputStream
  • ObjectInputStream
  • ObjectOutputStream
-import java.io.ByteArrayInputStream;
-import java.io.ByteArrayOutputStream;
-import java.io.ObjectInputStream;
-import java.io.ObjectOutputStream;
 import java.util.NoSuchElementException;

56-56: Remove extra blank line.

There's an unnecessary extra blank line here.

    }

-
    @Test

73-73: Remove unnecessary comment.

The comment // Expect NoSuchElementException instead appears to be a leftover from development and doesn't add value to the test.

        assertThatThrownBy(() -> none.toTry(() -> null).get())
-                .isInstanceOf(NoSuchElementException.class) // Expect NoSuchElementException instead
+                .isInstanceOf(NoSuchElementException.class)
                .hasMessage("No value present");

87-91: Remove multiple consecutive blank lines.

There are several consecutive blank lines at the end of the file which should be cleaned up.

    }

-
-
-
-
}
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between bc331e8 and d9ac34d.

📒 Files selected for processing (3)
  • src/main/java/org/fungover/breeze/control/None.java (1 hunks)
  • src/main/java/org/fungover/breeze/funclib/control/Try.java (3 hunks)
  • src/test/java/org/fungover/breeze/control/OptionTryIntegrationTest.java (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/main/java/org/fungover/breeze/funclib/control/Try.java
  • src/main/java/org/fungover/breeze/control/None.java
🔇 Additional comments (4)
src/test/java/org/fungover/breeze/control/OptionTryIntegrationTest.java (4)

17-24: Good test implementation for successful Option to Try conversion.

The test correctly validates that a Some option converts to a successful Try using behavioral assertions instead of implementation-specific type checks. This follows good testing practices by focusing on behavior rather than implementation details.


26-34: Good edge case testing for None to Try conversion.

Well-structured test that verifies both the failure state and the expected exception message when converting a None option to Try.


68-75: Good null handling test case.

This test effectively verifies the behavior when a null exception is returned from the supplier, ensuring proper error handling in the Option-Try integration.


77-85: Good reference equality check.

This test correctly verifies that the reference to the original value is maintained when converting from Option to Try, which is important for ensuring object identity is preserved during conversions.

@sonarqubecloud
Copy link
Copy Markdown

Copy link
Copy Markdown
Contributor

@Stelle83 Stelle83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nicely encapsulated logic for handling optional values.
Ensure all methods have comprehensive Javadoc comments, which is well done here.

Excellent test coverage with detailed unit tests for Some, None, and Option classes.

Clear and concise documentation explaining the purpose and usage of the Option type.

The pull request is well-implemented with comprehensive test coverage and clear documentation.

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.

3 participants