Search code that performs in-memory search

Question

This is the code I came across in our repository. This performs search in-memory and is invoked on service layer code.

In our front end operators like EQUAL_TO and so on are passed with field name and field values are passed as list of parameters in the query.

This is autowired and passed in the service calls like so

return paginate(SearchEngine.search(pgRatingResponses, operator, fieldName, fieldValues), pageable);

public class SearchEngine {
    /**
     * Valid pattern.
     */
    private static final String REGEX = "^[A-Z_a-z0-9 ]*$";

    /**
     * Searches of records.
     *
     * @param records
     * @param operator
     * @param fieldName
     * @param fieldValues
     * @param <T>
     * @return
     */
    public <T> List<T> search(final List<T> records, final SearchOperator operator, final String fieldName,
            final List<String> fieldValues) {
        List<T> results = new ArrayList<>();
        if (Objects.nonNull(records) && Objects.nonNull(operator) && Objects.nonNull(fieldName)
                && StringUtils.hasLength(fieldName) && Objects.nonNull(fieldValues) && !fieldValues.isEmpty()
                && !records.isEmpty()) {
            switch (operator) {
            case EQUAL_TO:
                results = records.stream()
                        .filter(record -> fieldValues.stream().filter(Objects::nonNull).map(String::trim)
                                .filter(StringUtils::hasLength).distinct()
                                .filter(fieldValue -> Pattern.matches(REGEX, fieldValue))
                                .anyMatch(fieldValue -> isEqual(fieldName.trim(), fieldValue, record)))
                        .collect(Collectors.toList());
                break;
            case NOT_EQUAL_TO:
                results = records.stream()
                        .filter(record -> fieldValues.stream().filter(Objects::nonNull).map(String::trim)
                                .filter(StringUtils::hasLength).distinct()
                                .filter(fieldValue -> Pattern.matches(REGEX, fieldValue))
                                .allMatch(fieldValue -> !isEqual(fieldName.trim(), fieldValue, record)))
                        .collect(Collectors.toList());
                break;
            case CONTAINS:
                results = records.stream()
                        .filter(record -> fieldValues.stream().filter(Objects::nonNull).map(String::trim)
                                .filter(StringUtils::hasLength).distinct()
                                .filter(fieldValue -> Pattern.matches(REGEX, fieldValue))
                                .anyMatch(fieldValue -> contains(fieldName.trim(), fieldValue, record)))
                        .collect(Collectors.toList());
                break;
            case DOES_NOT_CONTAIN:
                results = records.stream()
                        .filter(record -> fieldValues.stream().filter(Objects::nonNull).map(String::trim)
                                .filter(StringUtils::hasLength).distinct()
                                .filter(fieldValue -> Pattern.matches(REGEX, fieldValue))
                                .allMatch(fieldValue -> !contains(fieldName.trim(), fieldValue, record)))
                        .collect(Collectors.toList());
                break;
            default:
                throw new IllegalArgumentException(ErrorConstants.PARAMETER_VALUE_MISMATCHING);
            }
        } else if (Objects.nonNull(records)) {
            results = records;
        }
        return results;
    }

    /**
     * Checks for equality.
     *
     * @param <T>
     * @param fieldName
     * @param fieldValue
     * @param record
     * @return
     */
    private <T> boolean isEqual(final String fieldName, final String fieldValue, final T record) {
        try {
            for (Field field : record.getClass().getDeclaredFields()) {
                field.setAccessible(true);
                if (field.getName().equals(fieldName) && Objects.nonNull(field.get(record))) {
                    if (field.getType().equals(String.class)) {
                        return field.get(record).toString().trim().equalsIgnoreCase(fieldValue);
                    } else {
                        return field.get(record).equals(fieldValue);
                    }
                }
            }
        } catch (IllegalArgumentException | IllegalAccessException e) {
            log.error(ErrorConstants.PARAMETER_VALUE_MISMATCHING + e.getLocalizedMessage());
        }
        return false;
    }

    /**
     * Checks for pattern matchings.
     *
     * @param <T>
     * @param fieldName
     * @param fieldValue
     * @param record
     * @return
     */
    private <T> boolean contains(final String fieldName, final String fieldValue, final T record) {
        try {
            for (Field field : record.getClass().getDeclaredFields()) {
                field.setAccessible(true);
                if (field.getName().equals(fieldName) && Objects.nonNull(field.get(record))) {
                    return Pattern.matches(".*" + fieldValue.toLowerCase() + ".*",
                            field.get(record).toString().trim().toLowerCase());
                }
            }
        } catch (IllegalArgumentException | IllegalAccessException e) {
            log.error(ErrorConstants.PARAMETER_VALUE_MISMATCHING + e.getLocalizedMessage());
        }
        return false;
    }
}

Answer 1

Surprising behavior

The search function does some validations up front. If any of the validation fails, then it will return records if it's not null, or else a new empty list. Returning records practically means that everything is matched. So everything will be returned when for example:

• operator is null
• the field name is null
• the field name is empty
• the field values list is null
• the field values list is empty

I find this surprising behavior. I would have expected either an empty list (no matches), or IllegalArgumentException in these cases.

Contradiction in the logic

As mentioned earlier, when the field values list is empty, it's considered that everything matched. However, when this list is not empty but contains only null, empty, or blank strings, then when the operator is applied using anyMatch filters, then nothing will match.

This seems to contradict the intention of the initial validations. I think the initial validation should be improved to make the logic more consistent, but beware, this will change the behavior.

Avoid duplicated logic

As another answer already pointed out, the cases of the switch statement have a lot of duplicated logic, which should be eliminated. Going a bit further as the other answer, I would write like this:

String trimmedFieldName = fieldName.trim();

Stream<String> stream = fieldValues.stream()
        .filter(Objects::nonNull)
        .map(String::trim)
        .filter(StringUtils::hasLength)
        .distinct()
        .filter(fieldValue -> VALID_VALUE_PATTERN.matcher(fieldValue).matches());

Predicate<T> predicate = switch (operator) {
    case EQUAL_TO -> record -> stream.anyMatch(fieldValue -> isEqual(trimmedFieldName, fieldValue, record));
    case NOT_EQUAL_TO -> record -> stream.noneMatch(fieldValue -> isEqual(trimmedFieldName, fieldValue, record));
    case CONTAINS -> record -> stream.anyMatch(fieldValue -> contains(trimmedFieldName, fieldValue, record));
    case DOES_NOT_CONTAIN -> record -> stream.noneMatch(fieldValue -> contains(trimmedFieldName, fieldValue, record));
    default -> throw new IllegalArgumentException(ErrorConstants.PARAMETER_VALUE_MISMATCHING);
};

return records.stream()
        .filter(predicate)
        .collect(Collectors.toList());

Do not create ArrayList for nothing

The search method creates an ArrayList up front. This ArrayList is returned in the case when a validation fails and records is null, otherwise it's not used in any of the execution paths. Use Collections.emptyList() instead of creating ArrayList.

Use Pattern.compile instead of Pattern.matches

When matching a pattern repeatedly, it's good to compile it, so that the matching can be performed faster. So instead of:

private static final String REGEX = "^[A-Z_a-z0-9 ]*$";

// ...

fieldValue -> Pattern.matches(REGEX, fieldValue)

Do like this:

private static final Pattern VALID_VALUE_PATTERN = Pattern.compile("^[A-Z_a-z0-9 ]*$");

// ...

fieldValue -> VALID_VALUE_PATTERN.matcher(fieldValue).matches()

Notice that I also took this opportunity to use a name to describe the purpose of the pattern.

Consider noneMatch

Instead of:

stream().allMatch(x -> !foo(x, y))

It's more natural to write:

stream().noneMatch(x -> foo(x, y))

Answer 2

Welcome to Stack Review, the first thing I noticed in your search function code is the if else chain (note: I reduced all present conditions to the first two) :

List<T> results = new ArrayList<>();
if (Objects.nonNull(records) && Objects.nonNull(operator)) { 
 /* here the switch code that calculates results*/
} else if (Objects.nonNull(records)) {
            results = records;
       }
return results;

This can be expressed in a more comfortable way like below :

if (Objects.nonNull(records) && Objects.nonNull(operator)) { /* here the switch code */
}

if (Objects.nonNull(records)) { 
   return records;
}
return Collections.emptyList();

Your switch code can be refactored isolating the common stream part like below:

Stream<String> stream = fieldValues.stream()
        .filter(Objects::nonNull).map(String::trim)
        .filter(StringUtils::hasLength).distinct()
        .filter(fieldValue -> Pattern.matches(REGEX, fieldValue));

Then you can switch over the .anyMatchand allMatch, applying them and return directly your list result.