-
Notifications
You must be signed in to change notification settings - Fork 388
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
CLDR-15830 Refactor/simplify NameType #4287
Conversation
-Remove getNameName, getNameTypeName, String[][] NameTable, INDEX_LANGUAGE, ..., nameTableIndex, fromNameTableIndex -Associate the enum values with the corresponding path fragments by means of constructors -Move FIX_KEY_NAME, fixKeyName from CLDRFile to NameType -New method StandardCodes.CodeType.fromNameType converts NameType to CodeType and obviates conversion to/from String as intermediate step -Comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great refactor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one item, otherwise looks great.
@@ -84,6 +84,26 @@ public NameType toNameType() { | |||
return NameType.NONE; | |||
} | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to make the related enum values be elements of the other one.
That is, CodeType would have a field set from its private constructor:
final NameType nameType;
Then toNameType becomes:
public NameType toNameType() {
return nameType;
}
Similarly, NameType would have a field set from its private constructor:
final CodeType codeType;
Then fromNameType moves to be a method NameType.toCodeType.
public CodeType toNameType() {
if (codeType == null) {
throw new IllegalArgumentException("Unsupported name type");
}
return codeType;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good idea, I'll make that change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@macchiati I thought it was a great idea, but it failed due to the sequence of initialization. If NameType constructor references CodeType, and CodeType constructor references NameType, one or the other will get null values.
See https://stackoverflow.com/questions/1506410/java-enums-two-enum-types-each-containing-references-to-each-other which suggests various workarounds, but they're all kind of ugly.
Instead I just made it a little more symmetrical by implementing NameType.toCodeType instead of CodeType.fromNameType.
Maybe there's still a more elegant or efficient way; I suggest leaving that for a follow-up PR if and when it seems worthwhile to pursue.
-Note: setting the mappings in constructors would be problematic due to initialization order
I've done it with something like the following, to avoid switches. However, I think your solution is good enough.
|
-Remove getNameName, getNameTypeName, String[][] NameTable, INDEX_LANGUAGE, ..., nameTableIndex, fromNameTableIndex
-Associate the enum values with the corresponding path fragments by means of constructors
-Move FIX_KEY_NAME, fixKeyName from CLDRFile to NameType
-New method NameType.toCodeType converts NameType to CodeType and obviates conversion to/from String as intermediate step
-Comments
CLDR-15830
ALLOW_MANY_COMMITS=true