5
\$\begingroup\$

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;
}
\$\endgroup\$
4
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ This is a very good method, but my java version is 1.8. \$\endgroup\$ – dai Jun 29 '18 at 1:40
  • \$\begingroup\$ For 1.8, replace dropWhile(x -> condition) with filter(x -> !condition) - the rest should work. And as java 8 has end-of-life in just half a year, you should upgrade soon anyway. \$\endgroup\$ – mtj Jun 29 '18 at 6:40
3
\$\begingroup\$

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();
    }
}
\$\endgroup\$
  • \$\begingroup\$ I don't think this is a good idea. This method causes the if statement to be too complicated. \$\endgroup\$ – dai Jun 28 '18 at 5:25
0
\$\begingroup\$

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

\$\endgroup\$
  • \$\begingroup\$ I understand your idea, can you write the details of the code? \$\endgroup\$ – dai Jun 29 '18 at 2:04
  • \$\begingroup\$ @Frank.Dai What does matchQueueState look like? \$\endgroup\$ – Eric Stein Jun 29 '18 at 11:27
  • \$\begingroup\$ I added some code. \$\endgroup\$ – dai Jun 29 '18 at 14:46
  • \$\begingroup\$ @Frank.Dai Updated with a more concrete example. \$\endgroup\$ – Eric Stein Jun 29 '18 at 16:08
  • \$\begingroup\$ If I have ten judgment conditions, will I add ten classes like this? \$\endgroup\$ – dai Jun 29 '18 at 16:46

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.