apache
Modernize codebase with Java improvements
This PR includes several improvements to modernize the codebase:
- Replace IllegalArgumentException with ClassCastException for type cast failures
- Replace IllegalArgumentException with NullPointerException for null checks
- Use modern Java collections API (toList() instead of collect(Collectors.toList()))
- Replace custom null checks with Objects.requireNonNull
- Add @Nonnull and @Nullable annotations throughout the codebase
These changes make the code more maintainable, safer, and aligned with modern Java practices.
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
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.
- 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
- https://github.com/apache/maven/pull/2281
its nice to fix this once, but without a computer checking and enforcing this by some tool like checkstyle or rewrite, its not completed.
- 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
- Use modern Java collections API (toList() instead of collect(Collectors.toList())) https://github.com/checkstyle/checkstyle/issues/17000
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 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 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."
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: @.***>
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:
there's an app for that.
- https://github.com/apache/maven/pull/2333
- https://github.com/apache/maven/pull/2333#discussion_r2090512771
- https://github.com/apache/maven/pull/2322
Will never succeed, until rewrite comes in place.