Refactoring a method having multiple switch case statements & generics

Question

I have an existing project based on Java in which an existing utility method is present which gets some values as an input and provides result as a boolean value.

I need to refactor this method since there is an upcoming requirement which will cause a greater number of switch cases to be introduced in this method.

I tried other possible solutions shown on other Stack Overflow questions, but those solutions were not applicable for the use case I have here.

The code snippet is mentioned below.

protected static < T extends Comparable < T >> boolean compareValues(T lookupValue, T actualValue, String comparisonCondition, List < T > lookupValues) {
    comparisonCondition = comparisonCondition.toUpperCase();
    boolean result;
    switch (comparisonCondition) {
        case EQUALS:
            result = lookupValue instanceof String && actualValue instanceof String ? (String.valueOf(lookupValue).trim()).equalsIgnoreCase(String.valueOf(actualValue).trim()) : lookupValue.compareTo(actualValue) == 0;
            break;
        case NOT_EQUALS:
            result = lookupValue.compareTo(actualValue) != 0;
            break;
        case LIKE:
            result = StringUtils.containsIgnoreCase(String.valueOf(actualValue), String.valueOf(lookupValue));
            break;
        case NOT_LIKE:
            result = !StringUtils.containsIgnoreCase(String.valueOf(actualValue), String.valueOf(lookupValue));
            break;
        case IN:
            result = lookupValues.stream().anyMatch(lkpValue - > lkpValue instanceof String ? ((String) lkpValue).trim().compareToIgnoreCase(String.valueOf(actualValue).trim()) == 0 : lkpValue.compareTo(actualValue) == 0);
            break;
        case NOT_IN:
            result = lookupValues.stream().noneMatch(lkpValue - > lkpValue instanceof String ? ((String) lkpValue).trim().compareToIgnoreCase(String.valueOf(actualValue).trim()) == 0 : lkpValue.compareTo(actualValue) == 0);
            break;
        default:
            if (LOGGER.isDebugEnabled()) {
                LOGGER.debug(MSG_FORMAT_INVALID_COMPARISON_CONDITION, comparisonCondition);
            }
            result = false;
    }
    if (LOGGER.isDebugEnabled()) {
        LOGGER.debug("Comparing value '{}' with '{}' using comparison condition '{}'.{}Result: {}", actualValue, Objects.nonNull(lookupValue) ? lookupValue : lookupValues.stream().map(Object::toString).collect(Collectors.joining(WhlProcessingConstants.SPLIT_COMMA)), comparisonCondition, LINE_SEPARATOR, result);
    }
    return result;
}

Can you please suggest some solution by which this code of the method can be refactored, so that it is scalable for any future requirements and also, the cognitive complexity will not increase as we include more comparisonConditions / cases in it?

For your information: I am using SonarLint plugin to analyze the cognitive complexity of the code.

Why are you using `T` if you're always expecting String? And why use `compare` instead of `equals`? — shmosel, May 08 '23 at 17:18
Adding more cases doesn't add **cognitive** complexity, really; it's just busy work--"I have this string, let's find it". The code essentially maps a string to a function taking the lookup and actual value, and whatever "lookup values" are. Quickest approach is to extract that interface and build the map. — Dave Newton, May 08 '23 at 17:26
You could use an `enum` instead (values: EQUALS, NOT_EQUALS, etc), in the enum create an abstract compare(...) method with an implementation for each value, then in your snippet replace switch with `enumInstance = YOURENUM.valueOf(comparisonCondition); return enumInstance.compare(...)` — Ale Zalazar, May 08 '23 at 17:26
GIves us an example of the future requirements so we can understand them. — aled, May 08 '23 at 17:28

score 0 · Answer 1 · answered May 09 '23 at 07:34

what do you think about defining a map with all these predicates and the keys? Additionally, you can try extracting the common comparisons that keep repeating. Here is a possible solution, but you might need to twitch a few things:

private static final Map<String, TriPredicate<T>> comparators = Map.of(
        "EQUALS", (lookup, actual, __) -> lookup instanceof String && actual instanceof String ? compareStrings(lookup, actual) : lookup.compareTo(actual) == 0,
        "NOT_EQUALS", (lookup, actual, __) -> lookup.compareTo(actual) != 0,
        "LIKE", (lookup, actual, __) -> containsIgnoreCase(lookup, actual),
        "NOT_LIKE", (lookup, actual, __) -> !containsIgnoreCase(lookup, actual),
        "IN", (__, actual, lookup) -> listContains(actual, lookup),
        "NOT_IN", (__, actual, lookup) -> !listContains(actual, lookup)
);

protected static <T extends Comparable<T>> boolean compareValues(T lookupValue, T actualValue, String comparisonCondition, List<T> lookupValues) {
    if (LOGGER.isDebugEnabled()) {
        LOGGER.debug("Comparing value '{}' with '{}' using comparison condition '{}'.{}Result: {}", actualValue, Objects.nonNull(lookupValue) ? lookupValue : lookupValues.stream().map(Object::toString).collect(Collectors.joining(WhlProcessingConstants.SPLIT_COMMA)), comparisonCondition, LINE_SEPARATOR, result);
    }
    return comparators.getOrDefault(comparisonCondition.toUpperCase(), (a, b, c) -> false)
            .test(lookupValue, actualValue, lookupValues);
}

private <T extends Comparable> boolean listContains(T actual, List<T> lookup) {
    return lookup.stream().anyMatch(val -> val instanceof String ? compareStrings(val, actual) : val.compareTo(actual) == 0);
}

private boolean containsIgnoreCase(Object actual, Object lookup) {
    return StringUtils.containsIgnoreCase(String.valueOf(actual), String.valueOf(lookup));
}

private boolean compareStrings(Object lookup, Object actual) {
    return (String.valueOf(lookup).trim()).equalsIgnoreCase(String.valueOf(actual).trim());
}


@FunctionalInterface
interface TriPredicate<X> {
    boolean test(X lookup, X actual, List<X> lookupValues);
}

Sagar · Answer 2 · 2023-07-27T12:03:57.803

A very similar example from book Effective Java and after adding generics would work

interface Operation {
    public <lookup,actual> boolean validate(lookup lookupValue, actual actualValue);
}
 enum BasicOperationn implements Operation{
    EQUALS{
        @Override
        public <lookup, actual> boolean validate(lookup lookupValue, actual actualValue) {
          return lookupValue instanceof String && actualValue instanceof String ? (String.valueOf(lookupValue).trim()).equalsIgnoreCase(String.valueOf(actualValue).trim()) :((String) lookupValue).compareTo(((String) actualValue)) == 0;        }
    },
     LIKE {
         @Override
         public <lookup, actual> boolean validate(lookup lookupValue, actual actualValue) {
           return   StringUtils.containsIgnoreCase(String.valueOf(actualValue), String.valueOf(lookupValue));
         }
     };
     ...

}

Edited: Adding client code

class ClientClass{
    protected static < T extends Comparable < T >> boolean compareValues(T lookupValue, T actualValue, String comparisonCondition, List< T > lookupValues) {
        comparisonCondition = comparisonCondition.toUpperCase();
        BasicOperationn operation = BasicOperationn.valueOf(comparisonCondition);
        boolean result = operation.validate(lookupValue, actualValue);
        return result;
    }
}

Refactoring a method having multiple switch case statements & generics

2 Answers2