Skip to content

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Simplify BooleanConverter and ColorConverter #364

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 0 additions & 11 deletions src/main/java/org/apache/commons/beanutils2/converters/AbstractConverter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
import org.apache.commons.beanutils2.ConversionException;
import org.apache.commons.beanutils2.ConvertUtils;
import org.apache.commons.beanutils2.Converter;
import org.apache.commons.lang3.StringUtils;
import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory;

Expand Down Expand Up @@ -59,16 +58,6 @@ public abstract class AbstractConverter<D> implements Converter<D> {
// private static final String PACKAGE = AbstractConverter.class.getPackage().getName() + ".";
private static final String PACKAGE = "org.apache.commons.beanutils2.converters.";

/**
* Converts the given object to a lower-case string.
*
* @param value the input string.
* @return the given string trimmed and converter to lower-case.
*/
protected static String toLowerCase(final Object value) {
return StringUtils.toRootLowerCase(toString(value));
}

/**
* Converts the given object to a lower-case string.
*
Expand Down
32 changes: 7 additions & 25 deletions src/main/java/org/apache/commons/beanutils2/converters/BooleanConverter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,20 +43,6 @@
*/
public final class BooleanConverter extends AbstractConverter<Boolean> {

/**
* Copies the provided array, and ensures that all the strings in the newly created array contain only lower-case letters.
* <p>
* Using this method to copy string arrays means that changes to the src array do not modify the dst array.
* </p>
*/
private static String[] copyStrings(final String[] src) {
final String[] dst = new String[src.length];
for (int i = 0; i < src.length; ++i) {
dst[i] = toLowerCase(src[i]);
}
return dst;
}

/**
* The set of strings that are known to map to Boolean.TRUE.
*/
Expand Down Expand Up @@ -97,8 +83,8 @@ public BooleanConverter(final Boolean defaultValue) {
* @since 1.8.0
*/
public BooleanConverter(final String[] trueStrings, final String[] falseStrings) {
this.trueStrings = copyStrings(trueStrings);
this.falseStrings = copyStrings(falseStrings);
this.trueStrings = trueStrings.clone();
this.falseStrings = falseStrings.clone();
}

/**
Expand All @@ -115,8 +101,8 @@ public BooleanConverter(final String[] trueStrings, final String[] falseStrings)
*/
public BooleanConverter(final String[] trueStrings, final String[] falseStrings, final Boolean defaultValue) {
super(defaultValue);
this.trueStrings = copyStrings(trueStrings);
this.falseStrings = copyStrings(falseStrings);
this.trueStrings = trueStrings.clone();
this.falseStrings = falseStrings.clone();
}

/**
Expand All @@ -136,20 +122,16 @@ public BooleanConverter(final String[] trueStrings, final String[] falseStrings,
@Override
protected <T> T convertToType(final Class<T> type, final Object value) throws Throwable {
if (Boolean.class.equals(type) || Boolean.TYPE.equals(type)) {
// All the values in the trueStrings and falseStrings arrays are
// guaranteed to be lower-case. By converting the input value
// to lowercase too, we can use the efficient String.equals method
// instead of the less-efficient String.equalsIgnoreCase method.
final String stringValue = toLowerCase(value);
final String stringValue = toString(value);

for (final String trueString : trueStrings) {
if (trueString.equals(stringValue)) {
if (trueString.equalsIgnoreCase(stringValue)) {
return type.cast(Boolean.TRUE);
}
}

for (final String falseString : falseStrings) {
if (falseString.equals(stringValue)) {
if (falseString.equalsIgnoreCase(stringValue)) {
return type.cast(Boolean.FALSE);
}
}
Expand Down
42 changes: 19 additions & 23 deletions src/main/java/org/apache/commons/beanutils2/converters/ColorConverter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")) {
Copy link
Member

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.

Copy link
Contributor Author

@basil basil Aug 11, 2025 •
edited
Loading

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, 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.

Copy link

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 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.

Copy link
Contributor Author

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.

return type.cast(Color.BLUE);
case "cyan":
} else if (stringValue.equalsIgnoreCase("cyan")) {
return type.cast(Color.CYAN);
case "darkgray":
case "darkgrey":
case "dark_gray":
case "dark_grey":
} else if (stringValue.equalsIgnoreCase("darkgray")
|| stringValue.equalsIgnoreCase("darkgrey")
|| stringValue.equalsIgnoreCase("dark_gray")
|| stringValue.equalsIgnoreCase("dark_grey")) {
return type.cast(Color.DARK_GRAY);
case "gray":
case "grey":
} else if (stringValue.equalsIgnoreCase("gray") || stringValue.equalsIgnoreCase("grey")) {
return type.cast(Color.GRAY);
case "green":
} else if (stringValue.equalsIgnoreCase("green")) {
return type.cast(Color.GREEN);
case "lightgray":
case "lightgrey":
case "light_gray":
case "light_grey":
} else if (stringValue.equalsIgnoreCase("lightgray")
|| stringValue.equalsIgnoreCase("lightgrey")
|| stringValue.equalsIgnoreCase("light_gray")
|| stringValue.equalsIgnoreCase("light_grey")) {
return type.cast(Color.LIGHT_GRAY);
case "magenta":
} else if (stringValue.equalsIgnoreCase("magenta")) {
return type.cast(Color.MAGENTA);
case "orange":
} else if (stringValue.equalsIgnoreCase("orange")) {
return type.cast(Color.ORANGE);
case "pink":
} else if (stringValue.equalsIgnoreCase("pink")) {
return type.cast(Color.PINK);
case "red":
} else if (stringValue.equalsIgnoreCase("red")) {
return type.cast(Color.RED);
case "white":
} else if (stringValue.equalsIgnoreCase("white")) {
return type.cast(Color.WHITE);
case "yellow":
} else if (stringValue.equalsIgnoreCase("yellow")) {
return type.cast(Color.YELLOW);
default:
// Do nothing.
}

if (stringValue.startsWith(HEX_COLOR_PREFIX)) {
Expand Down
6 changes: 3 additions & 3 deletions src/test/java/org/apache/commons/beanutils2/converters/BooleanConverterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,10 @@ class BooleanConverterTest {

@Test
void testAdditionalStrings() {
final String[] trueStrings = { "sure" };
final String[] falseStrings = { "nope" };
final String[] trueStrings = { "sure", "ναι" };
final String[] falseStrings = { "nope", "hayır" };
final AbstractConverter<Boolean> converter = new BooleanConverter(trueStrings, falseStrings);
testConversionValues(converter, new String[] { "sure", "Sure" }, new String[] { "nope", "nOpE" });
testConversionValues(converter, new String[] { "sure", "Sure", "ναι", "Ναι" }, new String[] { "nope", "nOpE", "hayır", "Hayır" });

assertThrows(ConversionException.class, () -> converter.convert(Boolean.class, "true"));
assertThrows(ConversionException.class, () -> converter.convert(Boolean.class, "bogus"));
Expand Down
Loading