Indexing when using a hashmap with streams

Question

I have a HashMap of teams that I need to print out. They need to be indexed and sorted by some parameters(not relevant). Everything works fine except for the indexing part which I got working in a way but it doesn't feel right.

int i=0;

public void printTable()
{
    StringBuilder sb = new StringBuilder();

    i = 0;

    table.keySet().stream()
                .map(key -> table.get(key))
                .sorted(Comparator.comparing(Team::getPoints).thenComparing(Team::goalDifference).reversed().thenComparing(Team::getName))
                .forEach(team -> sb.append(String.format("%2d%s %-15s%5d%5d%5d%5d%5d\n", ++i, ".", team.getName(), team.getPlayed(),
                         team.getWins(), team.getDraws(), team.getLosses(), team.getPoints())));

    System.out.print(sb.toString());
}

Is there a better way of doing this? I don't think Intstream would help here because I wouldn't be able to get the object from the HashMap using an index.

Jos · Accepted Answer · 2018-01-25T17:31:00.643

2

Updated Answer

You should avoid stateful streams / lambda expressions. reference. Due to this reason, my original answer below is a bad idea.

A better approach is to separate stream and printing.

List<Team> teams = table.values()
        .stream()
        .sorted(...)
        .collect(Collectors.toList());

StringBuilder sb = new StringBuilder();
for(int i=0; i<teams.size(); i++) {

    Team team = teams.get(i);
    sb.append(String.format("%2d. %-15s%5d%5d%5d%5d%5d\n", 
            i,
            team.getName(),
            team.getPlayed(),
            team.getWins(),
            team.getDraws(),
            team.getLosses(), 
            team.getPoints()));
}

System.out.println(sb.toString());

Original Answer [Bad Idea]

You cannot modify the reference of the variables/modify primitives that you are used within a stream or lambda. You can rewrite your code like below,

AtomicInteger i = new AtomicInteger();
String result = table.values()
        .stream()
        .sorted(...)
        .map(e -> i.incrementAndGet() + "_" + e)
        .collect(Collectors.joining());

AtomicInteger will give you mutability without changing the reference/modifying the primitive. You don't need to do look-ups in the map, you can directly iterate over values.

Edit

As @Eugene pointed out in the comment, the approach with AtomicInteger cannot be used if you are using parallel stream.

edited Jan 25 '18 at 17:31

answered Jan 24 '18 at 16:30

Jos

2,015
1
17
22

this is wrong. To simplify `Stream.of("a", "b", "c", "d") .map(x -> i.incrementAndGet() + "-" + x) .parallel() .forEach(System.out::println);` one of the possible results `2-d` or `1-c` is totally possible – Eugene Jan 24 '18 at 18:46
but then if you know that this will fail with a parallel stream and you advocate against it, you could use an `int[]` with a single value instead of `AtomicInteger` – Eugene Jan 24 '18 at 19:03
may i know, how does using int[] solve the issue? afik you will face the same issue. Also just because it doesn't work with parallel stream doesn't mean you should advocate against it. If that is the case, you should advocate against sort in streams! (since you shouldn't do sort in parallel stream). Also OP's stream doesn't seems to have the requirement of using a paralle stream. – Jos Jan 24 '18 at 19:07
*of course* you will. The point was that if you *know* that this will fail for parallel stream, why use `AtomicInteger` in the first place? – Eugene Jan 24 '18 at 19:13
you don't have to (and shouldn't) make every stream a parallel stream!! please read my comment again.. – Jos Jan 24 '18 at 19:15
parallel is *just* to prove a point, well, that your solution is at best too expensive, at worst plain wrong. Care to explain what you actually mean with sort and parallel? – Eugene Jan 24 '18 at 19:21
1

The fact that this happen to do the desired thing with a sequential stream, is an implementation detail. There is no guaranty that the `map` function is evaluated in the encounter order, regardless of the execution mode. – Holger Jan 25 '18 at 11:50
Thanks @Holger & Eugene for the suggestions. I have edited the answer. Even though I have seen several documentation saying not to use stateful streams, all of them demonstrate the issue using parallel streams. I couldn't find a reference which explains the actual issue (why an operation like map wont be evaluated in encounter order). Can you help me on this? – Jos Jan 25 '18 at 17:34
1

OP here. I saw in another thread that using a parallel stream would give me wrong results, and using an AtomicInteger works fine for now since I don't need to do parallel streaming. Both yours and @federico work fine for me, I just chose yours because it was simpler. Just a side question regarding the other approach. I know that the get method of ArrayLists has a complexity of O(1) and LinkedList has a complexity of O(n). What's the get method complexity of List? If it isn't O(1), then there is no point in doing this approach since we loop through the list n*n times. – Andrej Jan 26 '18 at 05:29
1

Never mind. To answer my own question, in another thread I saw that "There are no guarantees on the type, mutability, serializability, or thread-safety of the List returned" when we do `.collect(Collectors.toList())`. And to get the implementation we want, we can do `.collect(Collectors.toCollection(() -> new ArrayList()))` – Andrej Jan 26 '18 at 05:55
@JosAngelGeorge well, there is no such example, because there is no issue *with the current implementation* that could be demonstrated. But relying on properties that are not guaranteed means, the code may break with a different implementation. One practical example was relying on side effects in functions in a stream whose terminal operation is `count()`, which worked in Java 8 but may break in Java 9 when the count get be determined without evaluating the functions (and that opportunity hasn’t maxed out yet). Even when we might not imagine how this could break in the future, it could… – Holger Jan 26 '18 at 09:23

fps · Answer 2 · 2018-01-24T16:39:09.320

I would separate the sorting from the printing.

Sorting:

List<Team> sorted = table.values().stream()
    .sorted(Comparator.comparing(Team::getPoints)
            .thenComparing(Team::goalDifference)
            .reversed()
            .thenComparing(Team::getName))
    .collect(Collectors.toList());

Printing:

String table = IntStream.range(0, sorted.size())
    .mapToObj(i -> String.format("%2d%s %-15s%5d%5d%5d%5d%5d", 
            i, ".", 
            sorted.get(i).getName(), 
            sorted.get(i).getPlayed(),
            sorted.get(i).getWins(), 
            sorted.get(i).getDraws(), 
            sorted.get(i).getLosses(), 
            sorted.get(i).getPoints()))
    .collect(Collectors.joining("\n"));

This could be improved if the Team class had a method that accepts the position and returns the formatted string:

public String formatWithPosition(int position) {
    return String.format("%2d%s %-15s%5d%5d%5d%5d%5d", 
            position, ".", 
            name, played, wins, draws, losses, points);
}

Then, you could use it as follows:

String table = IntStream.range(0, sorted.size())
    .mapToObj(Team::formatWithPosition)
    .collect(Collectors.joining("\n"));

Indexing when using a hashmap with streams

2 Answers2

Updated Answer

Original Answer [Bad Idea]