Skip to content
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

Enhance naming of enums #1532

Merged
merged 2 commits into from Jul 24, 2017
Merged

Enhance naming of enums #1532

merged 2 commits into from Jul 24, 2017

Conversation

jodastephen
Copy link
Member

Add NamedEnum interface, to mark standard enums
Add EnumNames to ensure more lenient and consistent parsing
Enhanced performance of parse and format

Add `NamedEnum` interface, to mark standard enums
Add `EnumNames` to ensure more lenient and consistent parsing
Enhanced performance of parse and format
Copy link
Contributor

@brianweller89 brianweller89 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine - assuming the loss of custom parsing logic in the two highlighted enums is expected and known


// restricted constructor
private EnumNames(Class<T> enumType, boolean specialToString) {
ArgChecker.notNull(enumType, "enumType");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should use consistent naming when referring to the overridden toString() functionality; currently using "manualToString" in the method name and "specialToString" as the variable name (overriddenToString another option)

map.put(formatted.toUpperCase(Locale.ENGLISH), value);
map.put(formatted.toLowerCase(Locale.ENGLISH), value);
formattedSet.add(formatted);
formatMap.put(value, formatted);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move these 5 duplicate lines outside of the if/else? Declaring the String outside the if un-itilialised may look abit nasty but IMO in simple cases like this it is better than carrying the risk of having somebody updating one block of code whilst forgetting to update the duplicate.

* @return the enum value
*/
public T parse(String name) {
ArgChecker.notNull(name, "name");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NotEmpty would be a tighter check here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, but this way users get a better error message

String str = CaseFormat.UPPER_CAMEL.to(CaseFormat.UPPER_UNDERSCORE, uniqueName);
if (str.endsWith("I_S_D_A")) {
str = "ORIGINAL_ISDA";
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assume we are happy for this endsWith logic to be completely removed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was a hack that is now supported properly.

ArgChecker.notNull(uniqueName, "uniqueName");
return valueOf(uniqueName.replace('-', '_').replace("/", "").toUpperCase(Locale.ENGLISH));
public static FixedCouponBondYieldConvention of(String name) {
return NAMES.parse(name);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again we're losing some custom parsing logic here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was a hack that is now supported properly.

@jodastephen jodastephen merged commit 85c814f into master Jul 24, 2017
@jodastephen jodastephen deleted the topic/enum-names branch July 24, 2017 11:59
@jodastephen jodastephen modified the milestone: v1.4 Jul 31, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants