1

I have an Answer class and an User class.

Answer has a getUser() and User has a getPoints()

From a list of Answer I would like to get a HashSet of User sorted by points. I tried following:

 Set<User> collectSet = list.stream().map(Answer::getUser)
            .sorted(Comparator.comparing(User::getPoints))
            .collect(Collectors.toCollection(HashSet::new));

 collectSet.forEach(a -> System.out.println(a.toString()));

Unfortunately this doesn't seem to preserve the order. The output is always different.

Interesting is that the same example with list does work correctly

List<User> collectList = list.stream().map(Answer::getUser)
            .sorted(Comparator.comparing(User::getPoints))
            .collect(Collectors.toList());

collectList.forEach(a -> System.out.println(a.toString()));

What am I doing wrong?

isADon
  • 3,433
  • 11
  • 35
  • 49
  • 5
    A HashSet doesn't have any order. If you want to preserve insertion order, use a List, or a LinkedHashSet – JB Nizet Mar 02 '18 at 12:47
  • I don't want any duplicate elements. That's why I need a set. – isADon Mar 02 '18 at 12:50
  • then use `.distinct()` – Lino Mar 02 '18 at 12:50
  • 4
    And what do you think a LinkedHashSet is? Why don't you read the javadoc? – JB Nizet Mar 02 '18 at 12:51
  • [`forEach`](https://docs.oracle.com/javase/8/docs/api/java/util/stream/Stream.html#forEach-java.util.function.Consumer-) is not deterministic. Use at least [`forEachOrdered`](https://docs.oracle.com/javase/8/docs/api/java/util/stream/Stream.html#forEachOrdered-java.util.function.Consumer-) – Johannes Kuhn Mar 02 '18 at 13:19

3 Answers3

5
 Collectors.toCollection(LinkedHashSet::new)

First you are sorting those entries according to some Comparator - giving them "some" order, but then you put them into a HashSet (order is broken), thus different results. Use LinkedHashSet to collect them in order to preserve the sorting order.

If you still want to collect them to a List, you can:

 yourList.stream()
         .sorted(....)
         .distinct() 
         .collect(Collectors.toList())

The order of the operations themselves (distinct vs sorted) matters

Eugene
  • 117,005
  • 15
  • 201
  • 306
  • `assuming hashcode/equals implemented` should also be assumed when just using a plain old `Set` – Lino Mar 02 '18 at 12:53
  • 1
    @Lino :) right, will edit – Eugene Mar 02 '18 at 12:54
  • 1
    The order of `distinct` and `sorted` only matters for the natural order, which should be compatible with the equality. For a custom comparator, like the OP’s one comparing only the “points” property, the order can not be used to optimize the `distinct` operation. – Holger Mar 02 '18 at 13:50
  • @Holger oh! I guess i never understood it than... thank you! – Eugene Mar 02 '18 at 13:57
  • Thinking about it, it would be possible to optimize this too, but it would be a different algorithm than the one currently used for the natural order. – Holger Mar 02 '18 at 13:59
  • @Holger since you said you don't mind on a previous such request... would you provide your input here also? https://stackoverflow.com/questions/49070380/local-type-inference-vs-instance/49071129#49071129 – Eugene Mar 02 '18 at 14:31
3

HashSet does not preserve the insertion order but List does. You can try with LinkedHashSet instead.

Edit: An other alternative is to use a TreeSet. It's a set so the duplicated values are deleted. And the elements are sorted when they are inserted. The sort is made using the method compareTo (provided by the Comparable interface). For example :

// Answer class
public class Answer {

    private final User user;

    public Answer(final User user) {
        this.user = user;
    }

    public User getUser() {
        return user;
    }
}

// User class
public class User implements Comparable<User> {

    private final int points;

    public User(final int points) {
        this.points = points;
    }

    public int getPoints() {
        return points;
    }

    @Override
    public int compareTo(User other) {
        // Sort user by their points (ascending order)
        return points - other.points;
    }

    @Override
    public String toString() {
        return "User{" + "points=" + points + '}';
    }
}

Then in your main code :

// Main
List<Answer> answers = Arrays.asList(
    new Answer(new User(0)),
    new Answer(new User(20)),
    new Answer(new User(1)),
    new Answer(new User(20)),
    new Answer(new User(10))
);

answers
    .stream()
    .map(Answer::getUser)
    .collect(Collectors.toCollection(TreeSet::new))
    .forEach(System.out::println);

Output :

User{points=0}
User{points=1}
User{points=10}
User{points=20}
Junior Dussouillez
  • 2,327
  • 3
  • 30
  • 39
  • 5
    This code only works as long as a user only consists of the points property. In real life scenarios, you surely don’t want to consider all users having the same points value as equal, which is what what will happen with `TreeSet` here. – Holger Mar 02 '18 at 13:47
1

May be you want to use TreeSet.

Two users with same no. of points are not equal, So there should be some uniqueId for user.

I am considering it as userId

No hashCode, equals required here ( but good to have, see documentation ) as TreeSet uses Comparator ( alternatively User class can implement Comparable ) to determine whether two elements are equal or not.

TreeSet<User> users = answers.stream()
    .map(Answer::getUser)
    .collect(Collectors.toCollection(() -> new TreeSet<User>(
       Comparator.comparingInt(User::getPoints).thenComparing(User::getUserId))));
Venkata Raju
  • 4,866
  • 3
  • 27
  • 36