Compare values of elements in Stream

Question

I'm having fun with Java's Stream library and lambdas.

The following code looks for persons within a list that have the same ID (which might indicate that something's wrong with the data) and prints out each group of people that share one ID.

I'm not sure if I'm doing this the most concise way, though.

This is the Person class:

public class Person {
    private String name;
    private String id;

    public Person(String name, String id) {
        this.name = name;
        this.id = id;
    }

    public String getId() {
        return id;
    }

    @Override
    public String toString() {
        return "Person [name=" + name + ", id=" + id + "]";
    }
}

This is the code I'd love to get feedback on:

// Set up test data
List<Person> people = Arrays.asList(new Person("Michael", "1"),
        new Person("Tobias", "2"), new Person("Nicole", "3"),
        new Person("Sarah", "3"));

// Group persons by their ID
Map<String, List<Person>> peopleById = people.stream().collect(
        Collectors.groupingBy(Person::getId));

// Print out groups of people that share one ID
peopleById
        .values()
        .stream()
        .filter(peopleWithSameId -> peopleWithSameId.size() > 1)
        .forEach(
                peopleWithSameId -> System.out
                        .println("People with identical IDs: "
                                + peopleWithSameId));

The code does what I want and I think it's readable but I doubt that this is the most I can get out of Java's functional features.

I wonder if you know a more elegant way to solve the problem.

Answer 1

There cannot be a simpler solution:

• To know the duplicated ids, you must iterate over the entire collection.
• To print all the persons with duplicated ids, you must keep their full list.

As such, you will need to load the entire collection of persons in memory. There's no way around that. If you needed only the duplicate ids but not the Person objects, then you could keep just the ids with their counts, and throw away the Person objects as you go after use, that would be more efficient. (But that's not the case here.)

In any case, your solution can be more concise if you skip the intermediary map variable with the mapping of ids to lists of users:

people.stream()
        .collect(Collectors.groupingBy(Person::getId)).values().stream()
        .filter(peopleWithSameId -> peopleWithSameId.size() > 1)
        .forEach(peopleWithSameId -> System.out.println("People with identical IDs: " + peopleWithSameId));

Btw, in case you're wondering if the .stream() there could be .parallelStream(), it would be pointless, due to the synchronization in the println method of System.out (a PrintStream). (And without synchronization println wouldn't be thread safe anyway.)

Answer 2

Your code and Java 8 usage looks fine in general to me.

I do see an issue with the Person class, it looks like you are intending it to be an immutable class, if so, then you should also enforce it.

You need to ensure that the name and id fields can never be changed, you can do this by adding final to them. Your code currently seems to be safe, but it is not. I can extend Person and offer a method there to change the name and id fields, which violates the assumed variant of that those fields in Person are immutable.

Simply changing it to the following will do:

public class Person {
    private final String name;
    private final String id;
    ...
}

Onto the Java 8 usage now.

It is a good thing that you use the Collectors.groupingBy to provide a Map<String, List<Person>>, you cannot do it much faster either way if you want it to work with any kind of List<Person> as input and in this way you'll save yourself from nasty bugs and reimplementing what lots of people have already done, namely the grouping by operation.

Printing the offending values using Stream seems fine as well, except that you may rewrite it to look a little bit cleaner, something like this could work:

peopleById.values().stream()
    .filter(personList -> personList.size() > 1)
    .forEach(personList -> System.out.println("People with identical IDs: " + personList);

This is my personal preference on how to format it though, the only real change is to rename peopleWithSameId to personList, as it is simply a List<Person> and nothing more or less.

You've done a good job overall.

Answer 3

This problem space is not a 'textbook' match to the streams concept. There are going to be some rough edges. The most significant thing is that System.out.println is a 'Side Effect' (and also the collect-to-Map) and is an anti-pattern for a Stream because it makes a parallel stream harder to do. You cannot use the parallel features of streams with the system you have. As a consequence, the only benefit the stream provides is the 'syntax' it has, and not any performance.

Additionally, you have a number of intermediate 'nexus' points, where everything comes together. For example, the .values() on the peopleById requires creating an additional Set, so, at that point, you have a full Map, as well as a full Set of people Lists.

A more efficient use would be to use the native foreach on the Map, and to have a conditional on the map-pair:

    // Print out groups of people that share one ID
    peopleById.forEach( (id, peopleWithSameId) -> {
                if (peopleWithSameId.size() > 1) {
                    System.out.printf("People with identical ID %s are : %s%n", id, peopleWithSameId);
                }
            });

This removes the intermediate Set, but at the cost of using a traditional filter in the function.

Note, if you do not need to keep the Mapping of ID's to People lists, the whole thing can be condensed to:

    people.stream()
            .collect(Collectors.groupingBy(Person::getId))
            .forEach((id, peopleWithSameId) -> {
                if (peopleWithSameId.size() > 1) {
                    System.out.printf("People with identical ID %s are : %s%n", id, peopleWithSameId);
                }
            });