Multiple filters and maps java stream

Question

I need to find the odd occurring numbers as a list. For example the below list should give 1 and 2 as output.

Is there a way that I can do it in a better way?

ArrayList<Integer> numbers = new ArrayList<>(Arrays.asList(1,2,2,4,2,4));
List<Integer> OddOcuranceNumbers =
numbers.stream()
.collect(Collectors.groupingBy(Function.identity(), Collectors.counting()))
.entrySet().stream()
.filter(map -> map.getValue() % 2 == 1).map(s -> s.getKey())
.collect(Collectors.toList());
System.out.println(OddOcuranceNumbers);
 }

Answer 1

That depends on how you define better. Better might be more CPU or memory efficient, or it could be fewer bytes in the compiled code, or better parallelization.

But I am going to take a chance on assuming that this code runs on a reasonably normal platform and has no special requirements, and therefore being better generally means better readability.

Readability Improvements

In which case, your code is fine. But we can make it easier to read by splitting it into named steps and wrapping it in a good function.

public List<Integer> getNumbersOccuringOddTimes(final List<Integer> numbers) {
 final Map<Integer, Integer> numberCounts = numbers.stream().collect(groupingBy(Function.identity(), counting()));
 final Predicate<Entry> isOdd = e -> e.getValue() % 2 == 1;
 return numberCounts.entrySet().stream().filter(isOdd).map(Entry::getKey).collect(toList());
}

I've made five minor adjustments to your code to help with readability. I have;

1. Added a descriptive method name that wraps the steps.
2. Split the two streams into separate statements.
3. Used static imports (not shown) for the Collectors functions.
4. Moved the modulus check into a predicate variable (isOdd) so it can be named.
5. Swapped out the lambda in .map to a method reference because it is shorter and more descriptive.

• TorbenPutkonen: Surely you're not suggesting that two 100 character one liner stream operations are more readable than... well anything? :)

Answer 2

As Rudi Kershaw said, there isn't much to fix technically. Whatever your way of implementation is, you need to count the occurences, select the odd ones and collect the values. Streams are not inherently readable so what you can do is to concentrate on the presentation a bit more. The variable names you use are either straight out confusing or just not very descriptive.

For example, you're processing Map.Entry objects, not Map objects, when filtering out the odd ones. Thus use entry in the lambda instead of map:

.filter(entry -> entry.getValue() % 2 == 1)

Likewise when picking the keys from the entries, use a variable name that describes the object being processed:

.map(entry -> entry.getKey())

With those changes and some formatting, your code becomes this. For someone who is familiar with common stream concepts, this should be pretty clear.

List<Integer> oddOcuranceNumbers = numbers.stream()
 .collect(Collectors.groupingBy(Function.identity(), Collectors.counting()))
 .entrySet().stream()
 .filter(entry -> entry.getValue() % 2 == 1)
 .map(entry -> entry.getKey())
 .collect(Collectors.toList());