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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
