-
Notifications
You must be signed in to change notification settings - Fork 542
8250802: Refactor StringConverter and its subclasses #1880
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
d6c3ce3
b88e385
96d6021
9ded0d7
90aed8d
2b610c6
4e8bb92
3448438
010a39c
ffad73e
a030ce5
1b4248c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,113 @@ | ||
| /* | ||
| * Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved. | ||
| * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. | ||
| * | ||
| * This code is free software; you can redistribute it and/or modify it | ||
| * under the terms of the GNU General Public License version 2 only, as | ||
| * published by the Free Software Foundation. Oracle designates this | ||
| * particular file as subject to the "Classpath" exception as provided | ||
| * by Oracle in the LICENSE file that accompanied this code. | ||
| * | ||
| * This code is distributed in the hope that it will be useful, but WITHOUT | ||
| * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or | ||
| * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License | ||
| * version 2 for more details (a copy is included in the LICENSE file that | ||
| * accompanied this code). | ||
| * | ||
| * You should have received a copy of the GNU General Public License version | ||
| * 2 along with this work; if not, write to the Free Software Foundation, | ||
| * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. | ||
| * | ||
| * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA | ||
| * or visit www.oracle.com if you need additional information or have any | ||
| * questions. | ||
| */ | ||
|
|
||
| package javafx.util.converter; | ||
|
|
||
| import java.time.chrono.Chronology; | ||
| import java.time.chrono.IsoChronology; | ||
| import java.time.format.DateTimeFormatter; | ||
| import java.time.format.DateTimeFormatterBuilder; | ||
| import java.time.format.DecimalStyle; | ||
| import java.time.format.FormatStyle; | ||
| import java.time.temporal.Temporal; | ||
| import java.time.temporal.TemporalQuery; | ||
| import java.util.Locale; | ||
| import java.util.Objects; | ||
|
|
||
| /// Base class for the java.time types converters. | ||
| abstract class BaseTemporalStringConverter<T extends Temporal> extends BaseStringConverter<T> { | ||
|
|
||
| private static final Locale DEFAULT_LOCALE = Locale.getDefault(Locale.Category.FORMAT); | ||
| private static final Chronology DEFAULT_CHRONO = IsoChronology.INSTANCE; | ||
|
|
||
| private final DateTimeFormatter formatter; | ||
| private final DateTimeFormatter parser; | ||
|
|
||
| protected BaseTemporalStringConverter(FormatStyle dateStyle, FormatStyle timeStyle, Locale locale, Chronology chrono) { | ||
| locale = Objects.requireNonNullElse(locale, DEFAULT_LOCALE); | ||
| chrono = Objects.requireNonNullElse(chrono, DEFAULT_CHRONO); | ||
|
Comment on lines
+49
to
+50
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Parameter reassignment here. It all looks like fields, but only the last two are. Fields could be prefixed with
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I second John's suggestion with
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'm not sure what you want me to change here exactly. Is it about renaming parameters (see this discussion), or not using
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I meant to suggest using L50
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 I don't mind adding
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have to be careful when reviewing in github - I thought it was a field. chrono is not a field, does not need Parameter reassignment is ok (in my opinion).
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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. |
||
| formatter = createFormatter(dateStyle, timeStyle, locale, chrono); | ||
| parser = createParser(dateStyle, timeStyle, locale, chrono); | ||
| } | ||
|
|
||
| protected BaseTemporalStringConverter(DateTimeFormatter formatter, DateTimeFormatter parser, | ||
| FormatStyle dateStyle, FormatStyle timeStyle) { | ||
| this.formatter = formatter != null ? formatter : | ||
| createFormatter(dateStyle, timeStyle, DEFAULT_LOCALE, DEFAULT_CHRONO); | ||
| this.parser = parser != null ? parser : | ||
| formatter != null ? formatter : | ||
| createParser(dateStyle, timeStyle, DEFAULT_LOCALE, DEFAULT_CHRONO); | ||
| } | ||
|
|
||
| private final DateTimeFormatter createParser(FormatStyle dateStyle, FormatStyle timeStyle, Locale locale, Chronology chrono) { | ||
| String pattern = DateTimeFormatterBuilder.getLocalizedDateTimePattern(dateStyle, timeStyle, chrono, locale); | ||
| return new DateTimeFormatterBuilder().parseLenient() | ||
| .appendPattern(pattern) | ||
| .toFormatter() | ||
| .withChronology(chrono) | ||
| .withDecimalStyle(DecimalStyle.of(locale)); | ||
| } | ||
|
|
||
| /// To satisfy the requirements of date formatters as described in, e.g., | ||
| /// [LocalDateStringConverter#LocalDateStringConverter()], the method checks if there's a need to modify the pattern. | ||
| private final DateTimeFormatter createFormatter(FormatStyle dateStyle, FormatStyle timeStyle, Locale locale, Chronology chrono) { | ||
| if (dateStyle != null) { | ||
| String pattern = DateTimeFormatterBuilder.getLocalizedDateTimePattern(dateStyle, timeStyle, chrono, locale); | ||
| if (pattern.contains("yy") && !pattern.contains("yyy")) { | ||
| // Modify pattern to show four-digit year, including leading zeros. | ||
| String newPattern = pattern.replace("yy", "yyyy"); | ||
| return DateTimeFormatter.ofPattern(newPattern).withDecimalStyle(DecimalStyle.of(locale)); | ||
| } | ||
| } | ||
| return getLocalizedFormatter(dateStyle, timeStyle) | ||
| .withLocale(locale) | ||
| .withChronology(chrono) | ||
| .withDecimalStyle(DecimalStyle.of(locale)); | ||
| } | ||
|
|
||
| abstract DateTimeFormatter getLocalizedFormatter(FormatStyle dateStyle, FormatStyle timeStyle); | ||
|
|
||
| @Override | ||
| String toStringFromNonNull(T value) { | ||
| return formatter.format(value); | ||
| } | ||
|
|
||
| @Override | ||
| T fromNonEmptyString(String string) { | ||
| return parser.parse(string, getTemporalQuery()); | ||
| } | ||
|
|
||
| abstract TemporalQuery<T> getTemporalQuery(); | ||
|
|
||
| /// For tests only | ||
| DateTimeFormatter getFormatter() { | ||
| return formatter; | ||
| } | ||
|
|
||
| /// For tests only | ||
| DateTimeFormatter getParser() { | ||
| return parser; | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.