On Mon, 25 Aug 2025 11:29:09 GMT, Nir Lisker <[email protected]> wrote:
>> modules/javafx.base/src/main/java/javafx/util/converter/BaseTemporalStringConverter.java
>> line 50:
>>
>>> 48: protected BaseTemporalStringConverter(FormatStyle dateStyle,
>>> FormatStyle timeStyle, Locale locale, Chronology chrono) {
>>> 49: locale = Objects.requireNonNullElse(locale, DEFAULT_LOCALE);
>>> 50: chrono = Objects.requireNonNullElse(chrono, DEFAULT_CHRONO);
>>
>> Parameter reassignment here. It all looks like fields, but only the last
>> two are. Fields could be prefixed with `this.` to make this clearer, but
>> IMHO parameter assignment is always bad.
>
> IDEs can show fields and variables differently so it's very easy to discern.
> Perhaps it depends on your text editor settings. Adding "this" tends to be
> more noise than it's worth in my opinion. Can change if there's an agreement.
I second John's suggestion with `this.`. It matters when refactoring, even
with an IDE.
>> modules/javafx.base/src/main/java/javafx/util/converter/CurrencyStringConverter.java
>> line 39:
>>
>>> 37: public CurrencyStringConverter() {
>>> 38: super();
>>> 39: }
>>
>> `super()` here is just noise
>
> I opted to explicitly use `super` in these places for uniformity with the
> other constructors as it can help understand which constructor is called from
> where. The "constructor jungle" in these classes made me do things I don't
> normally do. In `LocalDateStringConverter`, for example, the constructor goes
> through a `this` constructor instead of `super`, so it can be confusing. Can
> remove anyway.
Second John's suggestion about `super()` - there is no need.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2535715860
PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2535733547
