RFR: 8250802: Refactor StringConverter and its subclasses
John Hendrikx
jhendrikx at openjdk.org
Sun Aug 24 20:52:06 UTC 2025
On Sun, 24 Aug 2025 10:13:25 GMT, Nir Lisker <nlisker at openjdk.org> wrote:
> Refactoring of all `StringConverter`s and their tests. General notes:
> * The documentation language has been unified and `null` parameter rules have been documented.
> * Tests have been cleaned up in the vein of https://github.com/openjdk/jfx/pull/1759 and unneeded `@BeforeAll`s were removed.
> * Internal fields were made `private final` to guarantee immutability.
>
> Incremental commits are provided for easier reviewing:
>
> ### Parent classes
> * `StringConverter`: updated documentation
> * `BaseStringConverter`: a new internal class that implements repeated code from converter implementations and serves as an intermediate superclass. It does empty and `null` string checks that are handled uniformly, except for `DefaultStringConverter`, which has a different formatting mechanism.
>
> ### Primitive-related converters
> * All primitive (wrapper) converters also document their formatting and parsing mechanism since these are "well-established".
>
> ### Format converter
> * Checked for `null` during constriction time to avoid runtime NPEs.
> * There is no test class for this converter. A followup might be desirable.
> * A followup should deprecate for removal `protected Format getFormat()` (as in [JDK-8314597](https://bugs.openjdk.org/browse/JDK-8314597) and [JDK-8260475](https://bugs.openjdk.org/browse/JDK-8260475).
>
> ### Number and subclasses converters
> * The intermediate `locale` and `pattern` fields were removed (along with their tests). The class generated a new formatter from these on each call. This only makes sense for mutable fields where the resulting formatter can change, but here the formatter can be computed once on construction and stored.
> * The only difference between these classes is a single method for creating a format from a `null` pattern, which was encapsulated in the `getSpecializedNumberFormat` method.
> * The terminally deprecated `protected NumberFormat getNumberFormat()` was removed. Can be split to its own issue if preferred. In my opinion, it shouldn't exist even internally since testing the internal formatter doesn't help. The only tests here should be for to/from strings, and these are lacking. A followup can be filed for adding more conversion tests.
>
> ### Date/Time converters
> * Added a documentation note advising users to use the `java.time` classes instead of the old `Date` class.
> * As with Number converters, only the `dateFormat` field was kept, which is created once on construction instead of on each call.
> * As with Number converters, the `getSpecialzied...
Looks fine, left some readability/style comments
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.
modules/javafx.base/src/main/java/javafx/util/converter/BaseTemporalStringConverter.java line 68:
> 66: .toFormatter()
> 67: .withChronology(chrono)
> 68: .withDecimalStyle(DecimalStyle.of(locale));
This is IMHO a bad way to indent things; not only is the indent no longer a multiple of 4, it also makes diffs larger than they need to be when say `DateTimeFormatterBuilder` is renamed or put in a variable.
modules/javafx.base/src/main/java/javafx/util/converter/BaseTemporalStringConverter.java line 86:
> 84: .withChronology(chrono)
> 85: .withDecimalStyle(DecimalStyle.of(locale));
> 86: }
Weird level of indent, perhaps use 4 or 8?
modules/javafx.base/src/main/java/javafx/util/converter/CurrencyStringConverter.java line 39:
> 37: public CurrencyStringConverter() {
> 38: super();
> 39: }
`super()` here is just noise
modules/javafx.base/src/main/java/javafx/util/converter/DateStringConverter.java line 43:
> 41: /// and the user's [Locale].
> 42: public DateStringConverter() {
> 43: super();
`super()` here is just noise
modules/javafx.base/src/main/java/javafx/util/converter/DateTimeStringConverter.java line 99:
> 97: /// both date and time styles.
> 98: public DateTimeStringConverter(Locale locale, String pattern) {
> 99: dateFormat = create(locale, DateFormat.DEFAULT, DateFormat.DEFAULT, pattern);
Just noting inconsistent style here (see below `this.dateFormat = ` or `dateFormat = `) -- I personally favor prefixing with `this.` when it is a write action.
modules/javafx.base/src/main/java/javafx/util/converter/DateTimeStringConverter.java line 112:
> 110:
> 111: private DateFormat create(Locale locale, int dateStyle, int timeStyle, String pattern) {
> 112: locale = Objects.requireNonNullElse(locale, DEFAULT_LOCALE);
See previous comment about using `Objects.requireNonNullElse` and doing parameter reassignment.
modules/javafx.base/src/main/java/javafx/util/converter/DateTimeStringConverter.java line 119:
> 117: }
> 118:
> 119: // treat as protected
Seeing this a lot, what do you want to convey here? I'm just wondering what this would mean to a future maintainer... "don't accidentally make it protected because then it would be API"? Can add that to a lot of methods in FX then...
If you really want to treat it as protected, then I expect a full javadoc. If you just want an inbetween class with helpful methods that you don't want to make public, then making the class and all its methods package private is a standard pattern (see StringBuilder / StringBuffer)... So I'm just wondering what a future maintainer will use this comment for?
modules/javafx.base/src/main/java/javafx/util/converter/DateTimeStringConverter.java line 129:
> 127: return dateFormat.parse(string);
> 128: } catch (ParseException e) {
> 129: throw new RuntimeException(e);
Should be `IllegalArgumentException`
modules/javafx.base/src/main/java/javafx/util/converter/FormatStringConverter.java line 42:
> 40: private final Format format;
> 41:
> 42: /// Creates a `StringConverter` for arbitrary types that uses the given `Format`.
Shouldn't that be `FormatStringConverter` that it creates?
To be honest, I never bother repeating this on constructors, so I just write "Creates a new instance ... "
modules/javafx.base/src/main/java/javafx/util/converter/FormatStringConverter.java line 57:
> 55: T result = (T) format.parseObject(string, pos);
> 56: if (pos.getIndex() != string.length()) {
> 57: throw new RuntimeException("Parsed string not according to the format");
Throw `IllegalArgumentException`?
modules/javafx.base/src/main/java/javafx/util/converter/FormatStringConverter.java line 67:
> 65: }
> 66:
> 67: /// {@return the `Format` instance for formatting and parsing in this `StringConverter`}
`FormatStringConverter` ? Or just leave that part of...
modules/javafx.base/src/main/java/javafx/util/converter/LocalDateTimeStringConverter.java line 82:
> 80: /// [IsoChronology#INSTANCE] will be used.
> 81: public LocalDateTimeStringConverter(FormatStyle dateStyle, FormatStyle timeStyle, Locale locale, Chronology chronology) {
> 82: // JEP-513 could make this look better by moving the null checks before super
This seems like an irrelevant comment. I doubt it would even look better if you did this (as you'd require variables). How about just making it nice to read like:
super(
Objects.requireNonNullElse(dateStyle, FormatStyle.SHORT),
Objects.requireNonNullElse(timeStyle, FormatStyle.SHORT),
locale,
chronology
);
Also, you're using here an indent that is not a multiple of 4.
modules/javafx.base/src/main/java/javafx/util/converter/NumberStringConverter.java line 82:
> 80:
> 81: private NumberFormat createFormat(Locale locale, String pattern) {
> 82: locale = Objects.requireNonNullElse(locale, Locale.getDefault());
Parameter reassignment is really bad form (Eclipse has a warning for it if you'd care to enable it).
Also I'm not sure I like `Objects.requireNonNullElse`; it's actually longer than:
this.locale = locale == null ? Locale.getDefault() : locale;
modules/javafx.base/src/main/java/javafx/util/converter/NumberStringConverter.java line 97:
> 95: return numberFormat.parse(string);
> 96: } catch (ParseException e) {
> 97: throw new RuntimeException(e);
This really should be `IllegalArgumentException`.
modules/javafx.base/src/main/java/javafx/util/converter/NumberStringConverter.java line 106:
> 104: }
> 105:
> 106: /// {@return the `NumberFormat` used for formatting and parsing in this `NumberStringConverter`}
Is this even a valid comment with the return tag embedded in curly braces? It's also internal, so can remove.
modules/javafx.base/src/main/java/javafx/util/converter/PercentageStringConverter.java line 38:
> 36: /// Creates a `PercentageStringConverter` that uses a formatter/parser based on the user's [Locale].
> 37: public PercentageStringConverter() {
> 38: super();
`super()` here is just noise
modules/javafx.base/src/main/java/javafx/util/converter/TimeStringConverter.java line 2:
> 1: /*
> 2: // * Copyright (c) 2010, 2024, Oracle and/or its affiliates. All rights reserved.
This looks like it shouldn't have been committed.
modules/javafx.base/src/main/java/javafx/util/converter/TimeStringConverter.java line 43:
> 41: /// time style, and the user's {@link Locale}.
> 42: public TimeStringConverter() {
> 43: super();
`super()` is just noise here
modules/javafx.base/src/test/java/test/javafx/util/converter/DateStringConverterTest.java line 48:
> 46: public class DateStringConverterTest {
> 47:
> 48: private static final Locale DEFALUT_LOCALE = Locale.getDefault(Locale.Category.FORMAT);
Typo: `DEFAULT_LOCALE`
modules/javafx.base/src/test/java/test/javafx/util/converter/DateStringConverterTest.java line 75:
> 73: new TestCase(new DateStringConverter(DateFormat.getDateInstance(DateFormat.LONG)),
> 74: DEFALUT_LOCALE, DateFormat.DEFAULT, null, DateFormat.getDateInstance(DateFormat.LONG))
> 75: );
Wouldn't this look better:
return List.of(
new TestCase(new DateStringConverter(), DEFALUT_LOCALE, DateFormat.DEFAULT, null, null),
new TestCase(new DateStringConverter(DateFormat.SHORT), DEFALUT_LOCALE, DateFormat.SHORT, null, null),
new TestCase(new DateStringConverter(Locale.UK), Locale.UK, DateFormat.DEFAULT, null, null),
new TestCase(new DateStringConverter(Locale.UK, DateFormat.SHORT), Locale.UK, DateFormat.SHORT, null, null),
new TestCase(new DateStringConverter("dd MM yyyy"), DEFALUT_LOCALE, DateFormat.DEFAULT, "dd MM yyyy", null),
new TestCase(
new DateStringConverter(DateFormat.getDateInstance(DateFormat.LONG)), DEFALUT_LOCALE,
DateFormat.DEFAULT, null, DateFormat.getDateInstance(DateFormat.LONG)
)
);
modules/javafx.base/src/test/java/test/javafx/util/converter/DateTimeStringConverterTest.java line 73:
> 71: new TestCase(new DateTimeStringConverter(),
> 72: Locale.getDefault(Locale.Category.FORMAT), DateFormat.DEFAULT, DateFormat.DEFAULT,
> 73: null, null, VALID_DATE_WITH_SECONDS),
Why not put the last closing bracket on a new line aligned with `new` just like everyone always does with the curly braces? Much more readable, and doesn't require the empty line between blocks to "make" it readable. So:
return List.of(
new TestCase(new DateTimeStringConverter(),
Locale.getDefault(Locale.Category.FORMAT), DateFormat.DEFAULT, DateFormat.DEFAULT,
null, null, VALID_DATE_WITH_SECONDS
),
// add more here...
);
modules/javafx.base/src/test/java/test/javafx/util/converter/DateTimeStringConverterTest.java line 94:
> 92: Locale.getDefault(Locale.Category.FORMAT), DateFormat.DEFAULT, DateFormat.DEFAULT,
> 93: null, DateFormat.getDateTimeInstance(DateFormat.LONG, DateFormat.FULL), VALID_DATE_WITH_SECONDS)
> 94: );
This makes it looks like it is the closing bracket related to `new TestCase` for example, while it is from the `List.of`
modules/javafx.base/src/test/java/test/javafx/util/converter/LocalDateStringConverterTest.java line 72:
> 70: NO_PARAM,
> 71: WITH_FORMATTER_PARSER,
> 72: WITH_FORMAT_STYLES,
I know enums allow it, but trailing comma is still ugly
modules/javafx.base/src/test/java/test/javafx/util/converter/LocalDateTimeStringConverterTest.java line 78:
> 76: NO_PARAM,
> 77: WITH_FORMATTER_PARSER,
> 78: WITH_FORMAT_STYLES,
Trailing comma
modules/javafx.base/src/test/java/test/javafx/util/converter/LocalDateTimeStringConverterTest.java line 134:
> 132: case WITH_FORMATTER_PARSER -> new LocalDateTimeStringConverter(aFormatter, aParser);
> 133: case WITH_FORMAT_STYLES -> new LocalDateTimeStringConverter(FormatStyle.SHORT, FormatStyle.SHORT,
> 134: Locale.UK, IsoChronology.INSTANCE);
Odd indent level, 4 or 8?
modules/javafx.base/src/test/java/test/javafx/util/converter/LocalTimeStringConverterTest.java line 72:
> 70: NO_PARAM,
> 71: WITH_FORMATTER_PARSER,
> 72: WITH_FORMAT_STYLES,
Trailing comma
modules/javafx.base/src/test/java/test/javafx/util/converter/NumberStringConverterTest.java line 44:
> 42: private static final String PATTERN = "#,##,###,####";
> 43:
> 44: private final NumberStringConverter usLocaleConverter = new NumberStringConverter(Locale.US);
can be `static` ?
modules/javafx.base/src/test/java/test/javafx/util/converter/NumberStringConverterTest.java line 50:
> 48: NumberFormat numberFormat = NumberFormat.getNumberInstance(Locale.getDefault());
> 49:
> 50: var nsc = new NumberStringConverter();
Why not `var converter`, `var c` or even just inlining this? I think these abbreviations are not helpful. This will make the tests all much more similar to each other.
modules/javafx.base/src/test/java/test/javafx/util/converter/TimeStringConverterTest.java line 86:
> 84: Locale.getDefault(Locale.Category.FORMAT), DateFormat.DEFAULT, null,
> 85: DateFormat.getTimeInstance(DateFormat.FULL), VALID_TIME_WITH_SECONDS)
> 86: );
See previous comment, this bracket seems related to `new TestCase`.
-------------
Marked as reviewed by jhendrikx (Reviewer).
PR Review: https://git.openjdk.org/jfx/pull/1880#pullrequestreview-3149487568
PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2296798578
PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2296799109
PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2296799512
PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2296786737
PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2296791683
PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2296792639
PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2296792949
PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2296793928
PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2296794534
PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2296785487
PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2296786007
PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2296786266
PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2296800138
PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2296787950
PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2296789162
PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2296789669
PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2296789894
PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2296794828
PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2296795007
PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2296796717
PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2296796208
PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2296797782
PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2296798081
PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2296801685
PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2296801878
PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2296802012
PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2296802251
PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2296791382
PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2296791086
PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2296798294
More information about the openjfx-dev
mailing list