Getting the right conditional ConfigId

Question

Many conditions need to be scanned for the first one that matches a set of tests. If there is a matching condition, that conditions ConfigId should be returned.

I have a way to do the matching of conditions, but there are too many continue statements. Is there a better way to perform these checks?

private Integer match(List<ConfitionEntity> conditions, ActualDto actualDto) {

    if (CollectionUtils.isEmpty(conditions)) {
        return null;
    }
    Integer configId = null;
    for (ConfitionEntity condition : conditions) {
        boolean match = matchState(condition, actualDto);
        if (!match) {
            continue;
        }
        match = matchStatus(condition, actualDto);
        if (!match) {
            continue;
        }
        match = matchType(condition, actualDto);
        if (!match) {
            continue;
        }
        match = matchPre(condition, actualDto);
        if (!match) {
            continue;
        }
        match = matchMobile(condition, actualDto);
        if (!match) {
            continue;
        }
        match = matchAge(condition, actualDto);
        if (!match) {
            continue;
        }
        match = matchNumber(condition, actualDto);
        if (!match) {
            continue;
        }
        match = matchPassed(condition, actualDto);
        if (!match) {
            continue;
        }
        match = matchName(condition, actualDto);
        if (match) {
            configId = condition.getConfigId();
            break;
        }

    }
    return configId;
}

Answer 1

With the Java functional extensions you can "easily" encode these tests as a BinaryPredicate short-circuitung and set. See the documentation for The and method on BiPredicate.

Consider the following code which you can add-to as your conditions change:

private static final BiPredicate<QueueMsgPushConditionEntity, QueueUpdatedDto> MATCH = ((c, q) -> c != null && q != null)
    .and((c, q) -> matchQueueState(c.getQueueState(), q.getState()))
    .and((c, q) -> matchFirstPush(c.getFirstPush(), q.getSerialId())
    .and((c, q) -> matchPushType(c.getPushType(), PushType.QUEUE_CALL))
    .......

Now, with that static lambda/functional expression, you can change your match function to look like:

private Integer match(List<QueueMsgPushConditionEntity> conditions, 
                      QueueUpdatedDto queueUpdatedDto) {

    Optional<QueueMsgPushConditionEntity> first = conditions.stream()
        .dropWhile(c -> !MATCH.test(c, queueUpdatedDto))
        .findFirst();

    return first.map(c -> c.getQueueMsgPushConfigId()).or(null);

}

What the above stream does, is go through all the conditions, and it throws them all away until it finds one that matches all the MATCH tests, and then it stops looking. If/when it finds one, it returns it as the Optional, which is mapped to the Integer, or null if nothing was found.

Answer 2

You could take advantage of short circuit behavior, that is it will stop evaluating at the first "false"

for (QueueMsgPushConditionEntity condition : conditions) {

    if (matchQueueState(condition.getQueueState(), queueUpdatedDto.getState()) &&
        matchFirstPush(condition.getFirstPush(), queueUpdatedDto.getSerialId())) &&
        matchPushType(condition.getPushType(), PushType.QUEUE_CALL) && .. the rest  )
    {
       return condition.getQueueMsgPushConfigId();
    }
}

Answer 3

This is a classic chain-of-responsibility pattern. Assuming there's enough work to justify it (you would be pushing the contents of the matchXXX methods into the classes), the code would be called like this:

final MyMatcher queue = new QueueMatcher();
final MyMatcher firstPush = new FirstPushMatcher();
queue.setNext(firstPush);
...
final Integer configId = queue.handleCondition(condition, queueUpdate);

MyMatcher would be an interface or abstract class:

public interface MyMatcher {
    void setNext(final MyMatcher myMatcher);

    /** @return configId */
    Integer handleCondition(final QueueMsgPushConditionEntity condition, final QueueUpdateDto queueUpdate);
}

And each link in the chain would look like:

import java.util.Arrays;
import java.util.Set;
import java.util.stream.Collectors;

final class QueueStateMatcher {

    private  MyMatcher next;

    public QueueStateMatcher() {
        super();
    }

    public void setNext(final MyMatcher myMatcher) {
        this.next = myMatcher;
    }

    public Integer handleCondition(final QueueMsgPushConditionEntity condition, final QueueUpdateDto queueUpdate) {
        final String userQueueState = condition.getQueueState();
        final Integer configId = condition.getQueueMsgPushConfigId();

        if (StringUtil.isBlank(userQueueState)) {
            return configId;
        }

        final Set<Integer> states =
                Arrays.stream(userQueueState.split(","))
                .map(i -> Integer.valueOf(i))
                .collect(Collectors.toSet());

        if (CollectionUtils.isEmpty(states) || states.contains(UserQueueState.NOTHING.getCode())) {
            return configId;
        }

        if (states.contains(queueUpdate.getState())) {
            return configId;
        }

        return this.handleCondition(condition, queueUpdate);
    }
}

Obviously, "MyMatcher" is a terrible name and you should make it more appropriate for your domain. The matchXXX methods you have now would be removed and their contents moved into the corresponding classes. You can make the implementations immutable instead of using setNext as long as you're okay with defining them in the opposite order that they'll execute. Or, if you leave them mutable, you can reorder the chain (probably not useful for you in the problem as described).