You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by "basil (via GitHub)" <gi...@apache.org> on 2025/08/11 18:22:06 UTC
[PR] Simplify `BooleanConverter` and `ColorConverter` [commons-beanutils]
basil opened a new pull request, #364:
URL: https://github.com/apache/commons-beanutils/pull/364
See #359 for context. commit 53cd215292bd19ec6a467156eea031fe83c1defc 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 to `src/main`) confirms that this simplified code works for unusual locales.
Before you push a pull request, review this list:
- [x] Read the [contribution guidelines](CONTRIBUTING.md) for this project.
- [x] Read the [ASF Generative Tooling Guidance](https://www.apache.org/legal/generative-tooling.html) if you use Artificial Intelligence (AI).
- [ ] I used AI to create any part of, or all of, this pull request.
- [x] Run a successful build using the default [Maven](https://maven.apache.org/) goal with `mvn`; that's `mvn` on the command line by itself.
- [ ] Write unit tests that match behavioral changes, where the tests fail if the changes to the runtime are not applied. This may not always be possible, but it is a best-practice.
- [x] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
- [x] Each commit in the pull request should have a meaningful subject line and body. Note that a maintainer may squash commits during the merge process.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Simplify `BooleanConverter` and `ColorConverter` [commons-beanutils]
Posted by "basil (via GitHub)" <gi...@apache.org>.
basil commented on code in PR #364:
URL: https://github.com/apache/commons-beanutils/pull/364#discussion_r2267911120
##########
src/main/java/org/apache/commons/beanutils2/converters/ColorConverter.java:
##########
@@ -90,42 +90,38 @@ protected <T> T convertToType(final Class<T> type, final Object value) throws Th
if (Color.class.isAssignableFrom(type)) {
final String stringValue = toString(value);
- switch (toLowerCase(stringValue)) {
- case "black":
+ if (stringValue.equalsIgnoreCase("black")) {
return type.cast(Color.BLACK);
- case "blue":
+ } else if (stringValue.equalsIgnoreCase("blue")) {
Review Comment:
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Simplify `BooleanConverter` and `ColorConverter` [commons-beanutils]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #364:
URL: https://github.com/apache/commons-beanutils/pull/364#discussion_r2267668244
##########
src/main/java/org/apache/commons/beanutils2/converters/ColorConverter.java:
##########
@@ -90,42 +90,38 @@ protected <T> T convertToType(final Class<T> type, final Object value) throws Th
if (Color.class.isAssignableFrom(type)) {
final String stringValue = toString(value);
- switch (toLowerCase(stringValue)) {
- case "black":
+ if (stringValue.equalsIgnoreCase("black")) {
return type.cast(Color.BLACK);
- case "blue":
+ } else if (stringValue.equalsIgnoreCase("blue")) {
Review Comment:
-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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Simplify `BooleanConverter` and `ColorConverter` [commons-beanutils]
Posted by "basil (via GitHub)" <gi...@apache.org>.
basil commented on code in PR #364:
URL: https://github.com/apache/commons-beanutils/pull/364#discussion_r2267776790
##########
src/main/java/org/apache/commons/beanutils2/converters/ColorConverter.java:
##########
@@ -90,42 +90,38 @@ protected <T> T convertToType(final Class<T> type, final Object value) throws Th
if (Color.class.isAssignableFrom(type)) {
final String stringValue = toString(value);
- switch (toLowerCase(stringValue)) {
- case "black":
+ if (stringValue.equalsIgnoreCase("black")) {
return type.cast(Color.BLACK);
- case "blue":
+ } else if (stringValue.equalsIgnoreCase("blue")) {
Review Comment:
> 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, unlike `toLowerCase()` (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 multiple `.equals()` calls, but with only slightly higher per-comparison cost, making the new approach not much worse than the original.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Simplify `BooleanConverter` and `ColorConverter` [commons-beanutils]
Posted by "basil (via GitHub)" <gi...@apache.org>.
basil closed pull request #364: Simplify `BooleanConverter` and `ColorConverter`
URL: https://github.com/apache/commons-beanutils/pull/364
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Simplify `BooleanConverter` and `ColorConverter` [commons-beanutils]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #364:
URL: https://github.com/apache/commons-beanutils/pull/364#discussion_r2267887859
##########
src/main/java/org/apache/commons/beanutils2/converters/ColorConverter.java:
##########
@@ -90,42 +90,38 @@ protected <T> T convertToType(final Class<T> type, final Object value) throws Th
if (Color.class.isAssignableFrom(type)) {
final String stringValue = toString(value);
- switch (toLowerCase(stringValue)) {
- case "black":
+ if (stringValue.equalsIgnoreCase("black")) {
return type.cast(Color.BLACK);
- case "blue":
+ } else if (stringValue.equalsIgnoreCase("blue")) {
Review Comment:
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:
```java
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` with `PRIMARY` 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Simplify `BooleanConverter` and `ColorConverter` [commons-beanutils]
Posted by "basil (via GitHub)" <gi...@apache.org>.
basil commented on code in PR #364:
URL: https://github.com/apache/commons-beanutils/pull/364#discussion_r2267776790
##########
src/main/java/org/apache/commons/beanutils2/converters/ColorConverter.java:
##########
@@ -90,42 +90,38 @@ protected <T> T convertToType(final Class<T> type, final Object value) throws Th
if (Color.class.isAssignableFrom(type)) {
final String stringValue = toString(value);
- switch (toLowerCase(stringValue)) {
- case "black":
+ if (stringValue.equalsIgnoreCase("black")) {
return type.cast(Color.BLACK);
- case "blue":
+ } else if (stringValue.equalsIgnoreCase("blue")) {
Review Comment:
> 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, unlike `toLowerCase()` (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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org