RFR: 8250802: Refactor StringConverter and its subclasses [v2]

[Andy Goryachev]

On Mon, 25 Aug 2025 11:29:09 GMT, Nir Lisker <nlisker at openjdk.org> 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