casid / jte

Secure and speedy templates for Java and Kotlin.
https://jte.gg
Apache License 2.0
737 stars 54 forks source link

Use Stream#toList() instead of collect() #311

Closed MariusVolkhart closed 6 months ago

MariusVolkhart commented 6 months ago

toList was added in Java 16, and some streams can perform a more optimized operation compared to Collectors.toList().

MariusVolkhart commented 6 months ago

Comment from Brian Goetz on some details. IIRC, Mark Stuart also had a presentation about it when talking about the Collections API, but I can't find it. :) https://stackoverflow.com/a/65981428/2502247

codecov[bot] commented 6 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (0c83398) 91.20% compared to head (2bee4d7) 91.14%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #311 +/- ## ============================================ - Coverage 91.20% 91.14% -0.07% + Complexity 1204 1203 -1 ============================================ Files 76 76 Lines 3139 3139 Branches 489 489 ============================================ - Hits 2863 2861 -2 - Misses 167 168 +1 - Partials 109 110 +1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

casid commented 6 months ago

I think the main difference is that collect(Collectors.toList()) currently returns a mutable ArrayList (but this could change anytime in future Java versions) and toList() returns an immutable list.

This change could potentially break user code for these public methods:

public List<String> generateAll() public List<String> precompileAll() public List<String> precompile(List<String> names)

If users attempt to modify the returned lists. Most regular users of the lib don't call these, so it shouldn't be a problem. We should still add a comment to the patch notes though.