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

[John Hendrikx]

On Wed, 19 Nov 2025 20:48:24 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

>> I don't understand this either :)
>> 
>> What is the field name in L50?
>> 
>> This is the discussed code:
>> 
>>     // fields
>>     private final DateTimeFormatter formatter;
>>     private final DateTimeFormatter parser;
>> 
>>     // constructor
>>     protected BaseTemporalStringConverter(FormatStyle dateStyle, FormatStyle timeStyle, Locale locale, Chronology chrono) {
>>         locale = Objects.requireNonNullElse(locale, DEFAULT_LOCALE);        // local variable reassignment
>>         chrono = Objects.requireNonNullElse(chrono, DEFAULT_CHRONO);        // local variable reassignment
>>         formatter = createFormatter(dateStyle, timeStyle, locale, chrono);  // field assignment
>>         parser = createParser(dateStyle, timeStyle, locale, chrono);        // field assignment
>>     }
>> 
>> John is against local variable reassignment, which I understand, but I find it useful in the specific cases of "curating" like clamping and non-null-ing. It avoids mistakes like this:
>> 
>> void area(long length, long width, Shape shape) {
>>     posLength = length <= 0 ? 1 : length;
>>     posWidth = width <= 0 ? 1 : width;
>> 
>>     if (shape == TRIANGLE) {
>>         return (double) (length * width) / 2;
>>     } else if (shape == RECT) {
>>         return posLength * posWidth;
>>     }
>> }
>> 
>> 
>> John also wants `this` when doing field assignments (from what I understood), which one has to use if the parameter name is the same as the field (otherwise I don't think the IDE can know which is which). If it's unambiguous, I tend to avoid `this` when it's not required except for some cases such as uniformity.
>> 
>> I don't mind adding `this` to field assignments or renaming local variables, I just need to understand to consensus.
>
> I have to be careful when reviewing in github - I thought it was a field.  chrono is not a field, does not need `this`.
> 
> Parameter reassignment is ok (in my opinion).

I'm just making suggestions, feel free to ignore.

Reassignment of variables is IMHO always bad as it changes the meaning of variables halfway through code, so it should be minimized.  Parameter reassignment being 100% avoidable in all cases makes it possible to turn on an IDE warning/error to simply never allow that.

The reason I suggested `this` in this code is because it is 4 lines, that *look* to be doing the same thing, but in actuality are two parameter assignments, two field assignments.  I was basically suggesting to do either of these, so its clear these two pairs of lines are doing two different things (by avoiding parameter reassignment they become local variable declarations, by using `this` the two bottom lines are clearly different).

Anyway, this doesn't need an infinite discussion.  It's a "nit"; it's not like people can't figure out the code after doing a double take.

-------------

PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2544002828