-
Notifications
You must be signed in to change notification settings - Fork 223
Simplify BooleanConverter
and ColorConverter
#364
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?
Conversation
return type.cast(Color.BLACK); | ||
case "blue": | ||
} else if (stringValue.equalsIgnoreCase("blue")) { |
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.
-1 This is terrible IMO for two reasons: 1) the one-step switch logic has devolved into a cascading if-else and 2) equals() is replaced by the ignore case version. This is worse all for catering to your needs. Previously the constructor performed a conversion once, now you've got equalsIgnoreCade all over the place.
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.
This is terrible IMO for two reasons: 1) the one-step switch logic has devolved into a cascading if-else and 2)
equals()
is replaced by the ignore case version.
Hi @garydgregory, thanks for your feedback. While your comment accurately describes the changes in this PR, it doesn't explain why they're problematic. To clarify, here are some advantages of the proposed approach:
- Reduces code volume compared to the original, enhancing readability and maintainability.
- Avoids extra variables and method calls for string normalization, eliminating minor indirection that can hinder readability.
- Eliminates upfront string creation:
equalsIgnoreCase()
compares characters on the fly without allocating a new string, unliketoLowerCase()
(which can incur memory and copy overhead). - Steers clear of premature optimization, which is generally discouraged.
That said, the original switch
approach does indeed benefit from Java's hash-based lookup and performs normalization only once (both improving performance). However, this optimization matters mainly for large inputs or hot code paths, and such optimizations were likely more critical in 2005 than today. Moreover, multiple .equalsIgnoreCase()
calls still involve repeated comparisons, just like the multiple .equals()
calls in convertToType
, but with only slightly higher per-comparison cost, making the new approach not much worse than the original.
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.
I think we first need to clarify what we’re trying to achieve here, because Unicode case-insensitive comparison is not a trivial problem.
If our goal is to support localized color names, neither toLowerCase(...)
nor equalsIgnoreCase(...)
is sufficient for true case-insensitive matching.
For example, in German:
final String left = "WEISS";
final String right = "weiß";
LOGGER.info("Comparing German words: left='{}', right='{}'", left, right);
LOGGER.info("Upper case (Locale.ROOT): left='{}', right='{}'",
left.toUpperCase(Locale.ROOT),
right.toUpperCase(Locale.ROOT));
LOGGER.info("Lower case (Locale.ROOT): left='{}', right='{}'",
left.toLowerCase(Locale.ROOT),
right.toLowerCase(Locale.ROOT));
LOGGER.info("String.equalsIgnoreCase: {}", left.equalsIgnoreCase(right));
final Collator collator = Collator.getInstance(Locale.ROOT);
collator.setStrength(Collator.PRIMARY);
LOGGER.info("Collator (PRIMARY strength): {}", collator.equals(left, right));
Output:
Comparing German words: left='WEISS', right='weiß'
Upper case (Locale.ROOT): left='WEISS', right='WEISS'
Lower case (Locale.ROOT): left='weiss', right='weiß'
String.equalsIgnoreCase: false
Collator (PRIMARY strength): true
Here, "weiß"
and "WEISS"
differ only in case, yet:
toLowerCase
fails because it preservesß
.equalsIgnoreCase
fails because it compares per character without case folding.- Only a full case folding comparison (
Collator
withPRIMARY
strength) treats them as equal.
If we really want proper locale support, we need to use the right tool for the job, Collator
, not just string case transformations.
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.
I think we first need to clarify what we’re trying to achieve here, because Unicode case-insensitive comparison is not a trivial problem.
@ppkarwasz Thanks for asking! As stated in the PR description, this is a code simplification PR. The goal I am trying to achieve is a net simplification without any regressions in correctness and without (significant) regressions in performance.
If our goal is to support localized color names
That is not the goal in this PR. The java.awt.Color
has no knowledge of locales, and I see no compelling reason to support color translation as part of this library.
See #359 for context. commit 53cd215 from 2005 added a performance optimization that makes this code quite a bit more complicated than it needs to be. This strikes me as an example of premature optimization. The code can be made half as long (removing 33 lines) by simplifying this to use the standard
.equalsIgnoreCase
method. This also makes the code simpler to read and easier to reason about. An updated test (which passes both before and after the changes tosrc/main
) confirms that this simplified code works for unusual locales.Before you push a pull request, review this list:
mvn
; that'smvn
on the command line by itself.