On Wed, 19 Nov 2025 20:48:24 GMT, Andy Goryachev <[email protected]> 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
