0
\$\begingroup\$

I have a method that iterate a list of maps if a certain element is found then set value of certain variables and break the loop. I'm trying to revamp this into Java 8. And further if it can be improved, please suggest.

public static void doAction(List<Map<String, String>> decisionTree, String code, StringBuilder destination) {
    boolean conditionFound = false;
    boolean decisionFound = false;
    for (int i = 0; i < 100; i++) {
        if (null != decisionTree) {
            for (Map<String, String> decisionMap : decisionTree) {
                if (null != decisionMap && code.equalsIgnoreCase(decisionMap.get("code"))) {
                    decisionFound = true;
                    String decision = decisionMap.get("is_success");
                    String decisionAction = decisionMap.get("action");

                    BiPredicate<String, String> isApplicable = (d, a) -> "N".equalsIgnoreCase(d) && "Z".equalsIgnoreCase(a);

                    if (isApplicable.test(decision, decisionAction)) {
                        destination.append(decisionMap.get("destination"));
                        conditionFound = Boolean.TRUE;
                    }
                    break;
                }
            }
            if(decisionFound){
                if(conditionFound){
                    // do something
                    break;
                }
            }
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ Does this work? If yes, what is the point of the parameters decisionFound and conditionFound? It looks like you are attempting to return values to the caller, but that isn't possible in Java like that. \$\endgroup\$ – RoToRa Feb 3 at 12:16
  • \$\begingroup\$ @RoToRa I just added more details. decisionFound and conditionFound are being used later either to break the outer loop or continue. \$\endgroup\$ – Tishy Tash Feb 3 at 13:26
5
\$\begingroup\$

@Joop Eggen has already made some good suggestions based around your original code. You've made some slight modifications in response to comments to the parameters of your function, however your code still doesn't really make sense in some ways.

for (int i = 0; i < 100; i++) {

It's unclear why you'd want to perform this operation 100 times. If it doesn't work the first time, it's not going to work the 100th, you're not changing anything based on i, so unless the list is getting updated whilst you're processing on it by another thread this seems like a waste.

if(decisionFound){
    if(conditionFound){
        // do something
        break;
    }
}

As it stands, if conditionFound is true, then decisionFound must be true. There's no reason for you to check decisionFound here.

One thing I don't agree with @Joop Eggen about is that using BiPredicate helps your code. To me, this:

String decision = decisionMap.get("is_success");
String decisionAction = decisionMap.get("action");

BiPredicate<String, String> isApplicable = (d, a) -> "N".equalsIgnoreCase(d) && "Z".equalsIgnoreCase(a);

simply makes it harder to know what a and d are when looking at the logic. There's no requirement at the moment to have these temporary variables, what's wrong with just putting it in an old fashioned if statement?

if ("N".equalsIgnoreCase(decisionMap.get("is_success")) 
    && "Z".equalsIgnoreCase(decisionMap.get("action"))) {

Whilst I agree that it would be better to return the decision and let the caller decide what to do with the value I would probably go with several filters as I think it makes the code easier to follow:

decisionTree.stream()
        .filter(Objects::nonNull)
        .filter(m -> code.equalsIgnoreCase(m.get("code")))
        .filter(m -> "N".equalsIgnoreCase(m.get("is_success")))
        .filter(m -> "Z".equalsIgnoreCase(m.get("action")))
        .findFirst()
        .ifPresent(m -> destination.append(m.get("destination")));

You could return the result of findFirst if you decide you don't want to do the append processing in your doAction method.

For what it's worth, some of the strings being used seem like they could benefit from being in constants/enums 'code', 'is_success', 'action', 'N', 'Z'. If these get out of step between the caller and the code it's going to stop working.

| improve this answer | |
\$\endgroup\$
5
\$\begingroup\$

Probably not the right forum.

In java f(x) the method f will never change the variable x to another value; it cannot be make it null or true or whatever. (A rule to prevent a category of errors.)

So you want a result:

public static class Decision {
    String destination;
    boolean conditionFound;
}

As in java null's should in normal code not be valid, I did not test them. A test however would not need:

if (null != decisionTree) { // Typical C style.

but can be

if (decisionTree != null) {

As the following is not legal java:

if (decisionTree = null) { // *** Illegal java. // Run-time error in C (assignment).

Searching the code entry result results in an Optional<Decision>.

public static Optional<Decision> doAction(List<Map<String, String>> decisionTree,
            String code) {
    decisionTree.stream()
        .findAny(decisionMap -> code.equalsIgnoreCase(decisionMap.get("code")))
        .map(decisionMap -> {
            Decision decision = new Decision();

            String success = decisionMap.get("is_success");
            String action = decisionMap.get("action");
            BiPredicate<String, String> isApplicable = (s, a) ->
                    "N".equalsIgnoreCase(s) && "Z".equalsIgnoreCase(a);

            if (isApplicable.test(success, action)) {
                decision.destination = decisionMap.get("destination");
                decision.conditionFound = true;
            }
            return decision
        });
}

Optional<Decision> optionalDecision = doAction(decisionTree, code);
optionalDecision.ifPresent(decision -> {
    if (decision.conditionFound) {
        System.out.println(destination);
    }
});

A BiPredicate is not really needed, but not bad.

Instead of a result object, one could also pass an in-out parameter:

public static class DecisionParameter {
    boolean decisionFound;
    StringBuilder destination;
    boolean conditionFound;
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I just edited the question. Some of variables were not passed to the function instead they were used in the same method. For example: decisionFound and conditionFound are being used later either to break the outer loop or continue. \$\endgroup\$ – Tishy Tash Feb 3 at 13:27
  • 2
    \$\begingroup\$ CodeReview is primarily for working code. You'll get nice detailed answers when you post a new question. Maybe better on StackOverflow with for-loops. I am now leaving for two days, so you'll need someone else answering anyhow. Success. \$\endgroup\$ – Joop Eggen Feb 3 at 13:33
  • \$\begingroup\$ @JoopEggen I don't see anything about this code that is not working? I don't see a need to point out that "CodeReview is primarily for working code"? \$\endgroup\$ – Simon Forsberg Feb 3 at 14:42
  • 2
    \$\begingroup\$ @SimonForsberg in the original code, the decisionFound was a boolean parameter being passed into the method. Assignments made to that variable from within the function wouldn't have been observable outside of it, which is probably where the question of 'does this code really do what the op thinks it does' came from. As it stands, the code still seems to smell a bit of example code (such as the added for loop that doesn't make any difference), but I think there's enough actual code that the question is answerable. \$\endgroup\$ – forsvarir Feb 3 at 17:58

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.