On Sat, 30 Aug 2025 19:57:30 GMT, Nir Lisker <[email protected]> wrote:
>> modules/javafx.base/src/main/java/javafx/util/converter/LocalDateTimeStringConverter.java
>> line 82:
>>
>>> 80: /// [IsoChronology#INSTANCE] will be used.
>>> 81: public LocalDateTimeStringConverter(FormatStyle dateStyle,
>>> FormatStyle timeStyle, Locale locale, Chronology chronology) {
>>> 82: // JEP-513 could make this look better by moving the null
>>> checks before super
>>
>> This seems like an irrelevant comment. I doubt it would even look better if
>> you did this (as you'd require variables). How about just making it nice to
>> read like:
>>
>>
>> super(
>> Objects.requireNonNullElse(dateStyle, FormatStyle.SHORT),
>> Objects.requireNonNullElse(timeStyle, FormatStyle.SHORT),
>> locale,
>> chronology
>> );
>>
>>
>> Also, you're using here an indent that is not a multiple of 4.
>
> I think that the checks before calling the constructors make code clearer.
> I've been using flexible constructor bodies for this for a couple of versions
> now and I prefer it. As for the indent, it aligns with the parameter above it.
Can't you already use that feature? The baseline of JavaFX is JDK-25, no?
So I agree with John, either we remove this comment or implement it as such.
Otherwise this comment will exist for the next 10 years without any purpose.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r3801988203
