Modernize codebase with Java improvements

[gnodet]

This PR includes several improvements to modernize the codebase:

1. Replace IllegalArgumentException with ClassCastException for type cast failures
2. Replace IllegalArgumentException with NullPointerException for null checks
3. Use modern Java collections API (toList() instead of collect(Collectors.toList()))
4. Replace custom null checks with Objects.requireNonNull
5. Add @nonnull and @nullable annotations throughout the codebase

These changes make the code more maintainable, safer, and aligned with modern Java practices.

[kwin]

Regarding point 2. (and the related point 4.): I consider the IAE more concise than an NPE in case a method argument is passed with null (although not allowed). Compare also with https://discuss.kotlinlang.org/t/why-does-requirenotnull-throw-an-illegalargumentexception/7617/4

[desruisseaux]

I think that the main argument in favour of NullPointerException is consistency. Not all methods perform explicit null checks. Especially since JEP 358: Helpful NullPointerExceptions provides even more helpful messages than many explicit null checks. Therefore, if Objects.requireNonNull was throwing IllegalArgumentException for a null argument, we would be at risk of having sometime an IllegalArgumentException, and sometime a NullPointerException, depending on whether the method chooses to perform explicit null checks or not. It would also be a risk of compatibility break if the method implementation changes its strategy over time.

[Pankraz76]

• Use modern Java collections API (toList() instead of collect(Collectors.toList()))

Hi folks, please check out attached openrewrite link as its related and some can be done by computer.

This could be supplemented in dedicated PR as there is a migration for this:

• https://docs.openrewrite.org/recipes/staticanalysis/replacestreamtolistwithcollect

Maybe we can team up with as its kind of related. The migration is "free" as done by one click so we can reproduce anytime.

• https://docs.openrewrite.org/recipes/java/migrate/upgradetojava21
• [POC] Migrate to Java 21 - code #2281

[Pankraz76]

please consider this supplemental PR to apply SRP:

• Modernize codebase with Java improvements - Use modern Java collections API (toList() instead of Collectors.toList()) #2287

[Pankraz76]

its nice to fix this once, but without a computer checking and enforcing this by some tool like checkstyle or rewrite, its not completed.

[Pankraz76]

• https://docs.openrewrite.org/recipes/staticanalysis/replacestreamtolistwithcollect

this is wrong. Only the upgradetojava21 recipe does the desired migration. Anyways the PR is now fixed. #2287

[Pankraz76]

• Use modern Java collections API (toList() instead of collect(Collectors.toList()))
  AvoidOutdatedUsage: Use modern Java collections API (toList() instead of collect(Collectors.toList())) checkstyle/checkstyle#17000

[gnodet]

its nice to fix this once, but without a computer checking and enforcing this by some tool like checkstyle or rewrite, its not completed.

I don't see where it's done in your PR as it only provides the result of executing the transformation.

[Pankraz76]

Current State of NullPointerException Handling

The current implementation around NullPointerException is not ideal, but in context, it's clear what's happening.

Options:

1. Keep it as-is
   • Works but lacks elegance.
2. Remove all such checks
   • Simplifies but loses explicit null-check messaging.
3. Refactor into a dedicated layer
   • Separate the "cannot be null" concern to avoid:
     • DRY violations (repetition)
     • Feature envy (fluff and copy-paste)

---

Proposal: Dedicated Logging Layer

If improved logging is desired, this concern should be moved to a dedicated utility layer, similar to Apache Maven's approach:

maven/impl/maven-impl/src/main/java/org/apache/maven/impl/ImplUtils.java

Line 37 in d65c375

throw new IllegalArgumentException(name + " cannot be null");

Benefits:

• Contextual clarity: Right now, the check is just a short "non-null" hint.
• Better debugging: Explicit messaging improves NullPointerException readability.
• Consistency: Centralized handling avoids duplication.

This change is already reflected in PR #2290.

---

[Pankraz76]

Current State of NullPointerException Handling

The current implementation around NullPointerException is not ideal, but in context, it's clear what's happening.

Options:

1. Keep it as-is
   • Works but lacks elegance.
2. Remove all such checks
   • Simplifies but loses explicit null-check messaging.
3. Refactor into a dedicated layer
   • Separate the "cannot be null" concern to avoid:
     • DRY violations (repetition)
     • Feature envy (fluff and copy-paste)

---

Proposal: Dedicated Logging Layer

If improved logging is desired, this concern should be moved to a dedicated utility layer, similar to Apache Maven's approach:

maven/impl/maven-impl/src/main/java/org/apache/maven/impl/ImplUtils.java

Line 37 in d65c375

throw new IllegalArgumentException(name + " cannot be null");

Benefits:

• Contextual clarity: Right now, the check is just a short "non-null" hint.
• Better debugging: Explicit messaging improves NullPointerException readability.
• Consistency: Centralized handling avoids duplication.

This change is already reflected in PR #2290.

---

[gnodet]

@Pankraz76 I'm not sure what you aim for with your PRs. How do they differ from this one ? It's fine very fine that you used openrewrite rules, but isn't the result the same at the end ?

[Pankraz76]

@Pankraz76 I'm not sure what you aim for with your PRs. How do they differ from this one ? It's fine very fine that you used openrewrite rules, but isn't the result the same at the end ?

Hello friend, yes thats the goal see reasoning below:

Refactoring Large PRs: The Case for Atomic Changes/Why Smaller Changes Win

Key Insight:
When executed correctly, changes maintain a strict one-to-one (1:1) correspondence—preserving merge integrity. A targeted rebase could reduce the diff from ~200 files to ~100, dramatically enhancing review efficiency.

Why Monolithic PRs Fail:
We systematically reject PRs with hundreds of modified files because they:

• 🚫 Defy human review (cognitive overload)
• 🤖 Require blind automation (whereas small PRs enable thoughtful review)
• 💥 Create binary outcomes (either perfect or disastrous - no middle ground)

This PR's Structural Flaw:
While combining refactoring + modernization, it suffers from:

• 🌀 Concern entanglement (multiple domains modified simultaneously)
• 🎲 Gambler's fallacy ("all changes must be perfect")
• 🏚️ Feature envy (components overstepping their boundaries)

The Atomic PR Advantage:
Strategic splitting delivers:
✅ Reviewer ergonomics (focused attention beats fatigue)
✅ Precision rollbacks (targeted fixes without collateral damage)
✅ Architectural purity (SoC + SRP enforced naturally)
✅ Feature envy prevention (clear ownership boundaries)

The Granularity Paradox:
While defining "one logical change" is subjective, we resolve this through:

• 🧱 Strict module boundaries
• ⚖️ Cost/benefit weighing (if unsure, split smaller)

"These patterns persist because they survive Darwinian selection in production systems."

[elharo]

"Use modern Java collections API (toList() instead of collect(Collectors.toList()))" is riskier than the other changes because of the move of mutable to immutable lists. Nine times out of ten this is fine. The tenth time it's a bear to debug. These changes should be pulled out into a separate PR.

The rest could be split, but maybe don't have to be. Nuulable and Nonnull might also call for a separate PR.

[Pankraz76]

Yes we working on a dedicated commit for each concern. Thanks Sent from my iPhoneOn 5 May 2025, at 12:41, Elliotte Rusty Harold ***@***.***> wrote: @elharo requested changes on this pull request. "Use modern Java collections API (toList() instead of collect(Collectors.toList()))" is riskier than the other changes because of the move of mutable to immutable lists. Nine times out of ten this is fine. The tenth time it's a bear to debug. These changes should be pulled out into a separate PR. The rest could be split, but maybe don't have to be. Nuulable and Nonnull might also call for a separate PR. —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>

[Pankraz76]

Regarding point 2. (and the related point 4.): I consider the IAE more concise than an NPE in case a method argument is passed with null (although not allowed). Compare also with https://discuss.kotlinlang.org/t/why-does-requirenotnull-throw-an-illegalargumentexception/7617/4

Java and Lombok prefer NPE as well.

https://docs.oracle.com/javase/8/docs/api/java/util/Objects.html#requireNonNull-T-

https://projectlombok.org/features/NonNull

both valid points. Ideally its just one config-switch to adjust:

[gnodet]

There 's no plan to use Lombok at this point.