CLDR-15830 Refactor getName: new class NameGetter #4226

btangmu · 2024-12-04T22:30:45Z

-Each CLDRFile has a NameGetter; all getName methods moved to NameGetter

-Give GetName methods distinct names for clarify and to avoid excessive overloading

-Make methods private where possible

-Move some calls to nameGetter() out of loops, re-use

-Remove some dead code

-Comments

CLDR-15830

This PR completes the ticket.

ALLOW_MANY_COMMITS=true

-Each CLDRFile has a NameGetter; all getName methods moved to NameGetter

-All three CLDRFile constructors need to initialize NameGetter

…rloading

-Further renaming for clarity -Make methods private where possible -Move some calls to nameGetter() out of loops, re-use -Remove some dead code -Comments

macchiati

Looks good, just had a few comments.

macchiati · 2024-12-04T23:33:03Z

tools/cldr-code/src/main/java/org/unicode/cldr/util/NameGetter.java

+     */
+    public NameGetter(CLDRFile cldrFile) {
+        if (cldrFile == null) {
+            throw new RuntimeException("NameGetter must have non-null CLDRFile");


IllegalArgumentException is more specific.

Fixed in latest commit

macchiati · 2024-12-04T23:33:36Z

tools/cldr-code/src/main/java/org/unicode/cldr/util/NameGetter.java

+        this.cldrFile = cldrFile;
+    }
+
+    private static final String GETNAME_LOCALE_SEPARATOR =


Might put PATH in the name, also for similar ones.

Fixed in latest commit

macchiati · 2024-12-04T23:35:42Z

tools/cldr-code/src/main/java/org/unicode/cldr/util/NameGetter.java

+     * @return the name
+     */
+    public String getNameFromTypestrCode(String type, String code) {
+        return getNameFromTypenumCode(CLDRFile.typeNameToCode(type), code);


Once you get this merged, a second refactoring is to inline this and similar ones. 'Typenum' is hard to read; TypeEnum is better.

Yes, I'll address that in a subsequent PR. By "inline", do you mean, for example, to change getNameFromTypestrCode("territory", territory) to getNameFromTypenumCode(CLDRFile.TERRITORY_NAME, territory)? And then subsequently (or in the same PR) change CLDRFile.TERRITORY_NAME, etc., to an enum?

macchiati · 2024-12-04T23:37:33Z

tools/cldr-code/src/main/java/org/unicode/cldr/util/NameGetter.java

+     * @param code a code such as "JP"
+     * @return the name
+     */
+    public String getNameFromTypenumCode(int type, String code) {


We should also get rid of the CLDR fake enums like CLDRFile.TERRITORY_NAME in favor of enums. This is probably easier in a second PR, later.

Right; see above comment

macchiati · 2024-12-04T23:40:03Z

tools/cldr-code/src/main/java/org/unicode/cldr/util/NameGetter.java

+     * @return the name of the given bcp47 identifier
+     */
+    public synchronized String getNameFromBCP47(String localeOrTZID) {
+        return getNameFromBCP47Bool(localeOrTZID, false);


As long as we are writing new code, best to avoid having a boolean in any API, and rather use a meaningful enum. Again, could be done in later PR.

Good idea. I'll include that in a subsequent PR

-Make CLDRFile.nameGetter final -Remove 2 unused constants in CLDRFile -Change RuntimeException to IllegalArgumentException in NameGetter constructor -Rename 3 constants to end in _PATH -In TestUtilities.printCountries call nameGetter() once instead of repeatedly

btangmu · 2024-12-06T15:38:07Z

I've made this ready for review. So far this is fairly trivial refactoring but affects many files, so better not to keep changing on the same PR

srl295

I think it's OK but the naming is awkward. Many of the functions could have the same name, just with different overloaded parameters. A builder (fluent) API model might work better, to mutate a set of options rather than have a confusing and similar set of functions to choose from. BCP47 does not have @ signs in it, so a different name should be used.

I'm approving this because it's an important refactor.

srl295 · 2024-12-06T16:55:25Z

tools/cldr-code/src/main/java/org/unicode/cldr/util/NameGetter.java

+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+public class NameGetter {


The naming is a little awkward, isn't it a display name formatter?

Perhaps the naming should parallel ICU4J at least somewhat. https://unicode-org.github.io/icu-docs/apidoc/released/icu4j/com/ibm/icu/text/LocaleDisplayNames.html

isn't it a display name formatter?

Good question! Is that what it is?

BCP47 does not have @ signs in it, so a different name should be used.

That sounds important. Some of the methods already had comments saying parameters were bcp47 identifiers, and I carried that over to the related methods (callers/callees). If that's inaccurate, what would be a more accurate description? Part of the problem with this whole module is that there are strings and integers coming in without a clear explanation of what strings and integers are actually expected/allowed, and that's why Mark and I have been discussing an Enum instead of strings and integers, wherever that's possible...

srl295 · 2024-12-06T16:56:12Z

tools/cldr-code/src/main/java/org/unicode/cldr/util/NameGetter.java

+     * @param code a code such as "JP"
+     * @return the name
+     */
+    public String getNameFromTypestrCode(String type, String code) {


maybe forTypeAndCode() ?

yes, eventually we might not need/want to keep "getName" in the method names

srl295 · 2024-12-06T16:57:12Z

tools/cldr-code/src/main/java/org/unicode/cldr/util/NameGetter.java

+
+    /**
+     * Returns the name of the given bcp47 identifier. Note that extensions must be specified using
+     * the old "\@key=type" syntax.


then it's not BCP47!

The comment is just moved here from CLDRFile; I haven't really written any new code other than the glue between CLDRFile and NameGetter. Is the given identifier really "a bcp47 identifier or an extension thereof"? Pardon my ignorance!

srl295 · 2024-12-06T16:59:06Z

tools/cldr-code/src/main/java/org/unicode/cldr/util/NameGetter.java

+        return getNameFromBCP47BoolAlt(localeOrTZID, onlyConstructCompound, null);
+    }
+
+    public String getNameFromParserBool(LanguageTagParser lparser, boolean onlyConstructCompound) {


this could be an overload, forLocale() taking a LanguageTagParser as a et

srl295 · 2024-12-06T16:59:39Z

tools/cldr-code/src/main/java/org/unicode/cldr/util/NameGetter.java

+                null);
+    }
+
+    public synchronized String getNameFromBCP47Etc(


Etc is a little awkward, a Builder model might be better.

srl295 · 2024-12-06T17:01:24Z

tools/cldr-code/src/main/java/org/unicode/cldr/util/NameGetter.java

+     * @param paths if non-null, fillin with contributory paths
+     * @return the name of the given bcp47 identifier
+     */
+    private synchronized String getNameFromManyThings(


again, a builder model that sets up options might be better than a kitchen sink set of parameters.

new NameFormatter(locale).onlyConstructCompound(true).withLocalePatter(pattern)…

btangmu · 2024-12-06T17:44:28Z

I think it's OK but the naming is awkward. Many of the functions could have the same name, just with different overloaded parameters...

Steven, one reason for this refactor is that it was difficult to see how any particular version of getName() was actually used. With current main, IntelliJ reports getName "has 347 callers". Probably there's a way to find callers without including overloaded versions, but anyway 14 different methods in the same class with the same name is just too much for my little brain to work with. Refactoring is often easier if you can use global search-and-replace; methods with identical names get in the way of that.

Another reason for the refactor is that these methods were tangled up in the middle of CLDRFile which is just too large for maintainable code.

I agree that the names are somewhat awkward or arbitrary, but for me at least they give me a fighting chance to make sense what's calling what. After the code is reasonably clean, a nice finishing touch might be to restore some overloading, within reason.

I complete agree with you that some of these methods have a "kitchen sink of parameters" -- best practice is that no method should have more than about 4 parameters. A possible solution is to make them members of a class, and I suspect that's what you mean by a Builder model.

I can still hardly understand half of what's going on in this new class, so I can only revise so much at a time :-)

srl295 · 2024-12-06T17:51:15Z

I think it's OK but the naming is awkward. Many of the functions could have the same name, just with different overloaded parameters...

Steven, one reason for this refactor is that it was difficult to see how any particular version of getName() was actually used. With current main, IntelliJ reports getName "has 347 callers". Probably there's a way to find callers without including overloaded versions, but anyway 14 different methods in the same class with the same name is just too much for my little brain to work with. Refactoring is often easier if you can use global search-and-replace; methods with identical names get in the way of that.

That's a good point for that analysis. Since it's a library, I think it's harder to get this kind of analysis otherwise - SurveyTool might call it deep inside something for example (cross library), so "function is unused" warnings don't help.

Another reason for the refactor is that these methods were tangled up in the middle of CLDRFile which is just too large for maintainable code.

💯

I agree that the names are somewhat awkward or arbitrary, but for me at least they give me a fighting chance to make sense what's calling what. After the code is reasonably clean, a nice finishing touch might be to restore some overloading, within reason.

sounds good…

I complete agree with you that some of these methods have a "kitchen sink of parameters" -- best practice is that no method should have more than about 4 parameters. A possible solution is to make them members of a class, and I suspect that's what you mean by a Builder model.

At leisure take a look at https://www.baeldung.com/java-fluent-interface-vs-builder-pattern I think it's the Builder model (not fluent, though they have similarities). Yes, you'd have basically a Settings-type object that gets modified by calling functions on it (which you could chain) and then it ends up with a toString() call when you're ready to output it as text.

I can still hardly understand half of what's going on in this new class, so I can only revise so much at a time :-)

I'm with you! But it's a good refactor—— and Bottom line is that's why I hit Approve.

btangmu added 5 commits December 4, 2024 13:26

CLDR-15830 Refactor getName: new class NameGetter

03b38e1

-Each CLDRFile has a NameGetter; all getName methods moved to NameGetter

CLDR-15830 Refactor getName: fix 2 ommissions in first commit

00f60c6

-All three CLDRFile constructors need to initialize NameGetter

CLDR-15830 Refactor getName: reformat Java

c2ca341

CLDR-15830 Give GetName methods distinct names to avoid excessive ove…

fc0fd0c

…rloading

CLDR-15830 Revise GetName methods further

0da137d

-Further renaming for clarity -Make methods private where possible -Move some calls to nameGetter() out of loops, re-use -Remove some dead code -Comments

github-actions bot assigned btangmu Dec 4, 2024

macchiati reviewed Dec 4, 2024

View reviewed changes

btangmu marked this pull request as ready for review December 6, 2024 15:34

btangmu requested review from macchiati and srl295 December 6, 2024 15:34

btangmu changed the title ~~Refactor getName: new class NameGetter~~ CLDR-15830 Refactor getName: new class NameGetter Dec 6, 2024

srl295 approved these changes Dec 6, 2024

View reviewed changes

btangmu merged commit d4fffd7 into unicode-org:main Dec 6, 2024
9 checks passed

btangmu deleted the t15830_a branch December 6, 2024 17:56

CLDR-15830 Refactor getName: new class NameGetter #4226

CLDR-15830 Refactor getName: new class NameGetter #4226

Conversation

btangmu commented Dec 4, 2024

Uh oh!

macchiati left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

btangmu commented Dec 6, 2024

Uh oh!

srl295 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

btangmu commented Dec 6, 2024

Uh oh!

srl295 commented Dec 6, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants