3
\$\begingroup\$

I have a simple list of results, and I need to return only one value depending on content, for example:

public static void main(String[] args) {
    List<String> results = new ArrayList<>();
    results.add("ERROR");
    results.add("ERROR");
    results.add("OK");
    System.out.println(search(results));
}

public static String search(List src) {
    if (searchValueInStream(src, "ERROR")) {
        return "ERROR";
    } else if (searchValueInStream(src, "OK")) {
        return "OK";
    }
    return "NA";
}

public static boolean searchValueInStream(List src, String str) {
    return src.stream().anyMatch(v -> v.equals(str));
}

This code works fine, but it does not look good, and I want to use it in same stream like:

String result = results.stream()
 .someFilter(v->v.equals(ERROR)? return "ERROR")
 .someFilter(v->v.equals(OK)? return "OK")
 .orElse("NA");

without "if then else" statement. Is this possible?

\$\endgroup\$
0

4 Answers 4

4
\$\begingroup\$

You can convert every value into an integer, with higher integers taking priority over lower integers. Then call max and map the highest integer to its value. I went with ERROR = 1, OK = 0, NA = -1. Note that List.indexOf conveniently returns -1 if no match is found, which is why I chose to have higher integers have higher priority (rather than higher integers having lower priority).

// note: ordered in ascending priority
// which means last element has highest priority
public static final List<String> codes = Arrays.asList("OK", "ERROR");

public static int codeToInt(Object code) {
    return codes.indexOf(code); // -1 means NA
}

public static String intToCode(int i) {
    if (i < 0) // -1 means NA
        return "NA";
    return codes.get(i);
}

public static String search(List src) {
    // note: if the class is Test, use Test::code_to_int
    int i = src.stream().mapToInt(Test::codeToInt).max().orElse(-1);
    return intToCode(i);
}

With this approach, the number of passes over the stream does not increase with the number of distinct values. This would improve efficiency if you make codes an unordered map for O(1) indexing. If you do so, consider changing the value type into something with faster hashing.

In cases where the highest-priority code appears early, this code is inefficient in that max will not return early. Since worst-case scenarios require you to search the whole stream (e.g. all values are OK until the last value which is ERROR), I think this is workable.

With respect to style, I prefer to declare intToCode and codeToInt separately, but you could use lambda functions if you like. Hope this helps - I appreciate your question clarity!

\$\endgroup\$
4
  • 1
    \$\begingroup\$ Thanks for the catch. I made the appropriate edits to replace codes[i] with codes.get(i). Also fixed some other bugs: map to mapToInt, orelse to orElse, and generalized to Object streams. \$\endgroup\$ Commented Aug 10 at 0:08
  • 1
    \$\begingroup\$ This seems overly complicated. It would be much more readable and easy to understand if you loop over the results once, maintain a boolean flag for "OK was found" and return immediately if "ERROR" occurs. No need to make a detour via integers. I feel like you went this route because you expect there to be more values than just ok/error? No need to complicate the code when the need was not specified. \$\endgroup\$ Commented Aug 13 at 5:54
  • \$\begingroup\$ Meaning no offense, the code you presented is neither clean nor type-safe. 1) Get a better familiar with the language type system, changing the return type to java.lang.Object is not an improvement here 2) Learn about Generics in Java 3) The code is not aligned with the Java Language naming convention. Since this site is focused on given practical advices on writing clean and maintainable code, you might want to address these points and improve your answer. \$\endgroup\$ Commented Aug 14 at 10:17
  • \$\begingroup\$ Revised accordingly. \$\endgroup\$ Commented Aug 14 at 18:41
3
\$\begingroup\$

Other people have brought up the possibility of assigning a numerical priority to each possible value. I think this is an intuitive approach.

One simple way to find the most "important" value in a stream, given we can define "importance" as an ordering, is collecting the stream using Collectors.maxBy (or minBy I suppose - in this case it's pretty arbitrary).

For deciding what the order is, we can either write a comparison function of our own, or we can do the simpler thing by using Comparator.comparingInt. Since all possible messages are distinct and known in advance, the ints we compare can simply be kept in a Map.

With all that, we might end up writing something like:

private static final Map<String, Integer> OK_ERROR_PRIORITIES = Map.of("OK", 1, "ERROR", 2);

public static String search(Collection<? extends String> src) {
    return search(src, OK_ERROR_PRIORITIES, "NA");
}

public static T search(Collection<? extends T> src, Map<? super T, Integer> priorities, T defaultValue) {
    Collector<T, ?, Optional<T>> highestPriority = Collectors.maxBy(Comparator.comparingInt(priorities::get));

    Optional<T> result = src.stream()
        .filter(priorities::containsKey)
        .collect(highestPriority);

    return result.orElse(defaultValue);
}
\$\endgroup\$
2
\$\begingroup\$

There is absolutely no point in using streams for this or integers to represent the values. Don't complicate your life with streams just because you think they're cool. Streams are a hammer that make all your problems look like nails. Don't use streams unless they make sense. Don't try to prepare for more response types than you have (which is why people suggest the integers) unless you actually know you are going to need them.

Keep it simple and you will be able to maintain the code in the future.

boolean okFound = false;
for (String result: results) {
    if (ERROR.equals(result) {
        return ERROR;
    }

    if (OK.equals(result)) {
        okFound = true;
    }
}

return okFound ? OK : UNKNOWN;

There is nothing wrong with writing a plain old for loop in 2024. They are still a completely valid programming structure. Don't let the fashionable new tools cloud your vision.

This loop will also out-perform any stream based solution. Every dot in a stream is an object creation. Every -> is an additional method invocation. Every autoboxed Integer is a throwaway object creation. All of these cost memory and processor time. Once you start working with large data, you will have to pay attention what the nice and fancy libraries actually do under the covers.

This for-loop will short circuit on error, so it's worst case scenario is O(N).

\$\endgroup\$
1
\$\begingroup\$

I might have misunderstood what the code of @zond should produce, but the principle stays the same. So this code might reverse conditions (returns "OK" first and then "ERROR" instead of the other way round).


    if (searchValueInStream(src, "ERROR")) {
        return "ERROR";
    } else if (searchValueInStream(src, "OK")) {
        return "OK";
    }
    return "NA";

The problem with your code is that you're running through the list twice, once for the "ERROR" search, once for the "OK" search. This can be a costly operation (though, it most likely is not).

I'm not on good terms with streams (I believe they are overhyped and rarely reduce complexity/cognitive load) so I can only provide you with an example without them. What you want is to loop once through the list and keep state for this time. The most simple solution would be something like this:

boolean errorEncountered = false;

for (String item : items) {
    if (item.equals("OK")) {
        return "OK";
    } else if (item.equals("ERROR")) {
        errorEncountered = true;
    }
}

if (errorEncountered) {
    return "ERROR";
} else {
    return "NA";
}

In the best case the loop exits at the first item, as it is "OK", the worst case is that it loops through the whole list once.

However, this approach has two problems:

  1. If you want the item itself, you need to store that.
  2. With more items this becomes quite cumbersome.

The first point is rather easily solved:

Item firstEncounteredErrorItem = null;

for (Item item : items) {
    if (item.getState().equals("OK")) {
        return item;
    } else if (firstEncounteredErrorItem == null && item.getState().equals("ERROR")) {
        firstEncounteredErrorItem = item;
    }
}

if (firstEncounteredErrorItem != null) {
    return firstEncounteredErrorItem;
} else {
    return Item.NA;
}

A little bit more complex, but still quite manageable.

Now, the second point, extensibility, is harder to solve, as we may need to "stack" differently prioritized items. For that, we'd require something like "Tuple", which we most likely need to write ourselves. The basic idea would be keeping a List of encountered items which match their "priorization".

List<String> prioritziedItemStates = Item.PRIORITIZED_ITEM_STATES;
Item[] encounteredItems = new Item[Item.MAX_VALUE_COUNT];

for (Item item : items) {
    if (item.getState().equals("OK")) {
        return item;
    } else {
        int itemPrioritizedIndex = prioritziedItemStates.indexOf(item.getState());
        
        if (encounteredItems[itemPrioritizedIndex] == null) {
            encounteredItems[itemPrioritizedIndex] = item;
        }
    }
}

for (Item encounteredItem : encounteredItems) {
    if (encounteredItem != null) {
        return encounteredItem;
    }
}

return Item.NA;

This has still the upside that it runs through the list only once, but does a lookup for every item in another (short?) list that is not "OK". Whether that trade is worth it or not is depending on how large the original list is and how many states you need to keep.


String result = results.stream()
 .someFilter(v->v.equals(ERROR)? return "ERROR")
 .someFilter(v->v.equals(OK)? return "OK")
 .orElse("NA");

I'm not sure that's possible with streams, at least not without wrapping them multiple times. There is findFirst but that terminates the stream, but you want to keep going. Basically what you want would be something like this:

String result = results.stream()
        .takeFirstOrElseContinue(result -> result.equals("OK"))
        .takeFirstOrElseContinue(result -> result.equals("ERROR"))
        .orElse("NA");

I'm not aware that streams would work like that.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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