Java stream use in loop

Question

I am quite new to java streams. Do I need to re-create the stream each time in this loop or is there a better way to do this? Creating the stream once and using the .noneMatch twice results in "stream already closed" exception.

for ( ItemSetNode itemSetNode : itemSetNodeList )
{
  Stream<Id> allUserNodesStream = allUserNodes.stream().map( n -> n.getNodeId() );

  Id nodeId = itemSetNode.getNodeId();
  //if none of the user node ids match the node id, the user is missing the node
  if ( allUserNodesStream.noneMatch( userNode -> userNode.compareTo( nodeId ) == 0 ) )
  {
    isUserMissingNode = true;
    break;
  }
}

Thank you !

This seems like a less than optimal situation to use streams at all. I'd use a TreeSet (as Id seems to implement Comparable) or a HashSet instead. — Kayaman
– Kayaman, Commented Mar 12, 2018 at 19:21
Can you give an example of how you'd use a treeset? You'd iterate through the collection and turn it into a treeset and then use a set.contains()? — Fresh Codemonger
– Fresh Codemonger, Commented Mar 12, 2018 at 23:48

Simon Arsenault · Accepted Answer · 2018-03-12 19:22:26Z

2

I would suggest you make a list of all the user ids outside the loop. Just make sure the class Id overrides equals() function.

List<Id> allUsersIds = allUserNodes.stream().map(n -> n.getNodeId()).collect(Collectors.toList());

for (ItemSetNode itemSetNode : itemSetNodeList)
{
    Id nodeId = itemSetNode.getNodeId();

    if (!allUsersIds.contains(nodeId))
    {
        isUserMissingNode = true;
        break;
    }
}

answered Mar 12, 2018 at 19:22

Simon Arsenault

1,8451 gold badge18 silver badges35 bronze badges

Sign up to request clarification or add additional context in comments.

Comments

Kayaman · Accepted Answer · 2018-03-13 06:39:45Z

1

The following code should be equivalent, except that the value of the boolean is reversed so it's false if there are missing nodes.

First all the user node Ids are collected to a TreeSet (if Id implements hashCode() and equals() you should use a HashSet). Then we stream itemSetNodeList to see if all those nodeIds are contained in the set.

TreeSet<Id> all = allUserNodes
                        .stream()
                        .map(n -> n.getNodeId())
                        .collect(Collectors.toCollection(TreeSet::new));

boolean isAllNodes = itemSetNodeList
                        .stream()
                        .allMatch(n -> all.contains(n.getNodeId()));

There are many ways to write equivalent (at least to outside eyes) code, this uses a Set to improve the lookup so we don't need to keep iterating the allUserNodes collection constantly.

You want to avoid using a stream in a loop, because that will turn your algorithm into O(n²) when you're doing a linear loop and a linear stream operation inside it. This approach is O(n log n), for the linear stream operation and O(log n) TreeSet lookup. With a HashSet this goes down to just O(n), not that it matters much unless you're dealing with large amount of elements.

answered Mar 13, 2018 at 6:39

Kayaman

73.8k6 gold badges91 silver badges128 bronze badges

2 Comments

egorlitvinenko Over a year ago

According to his code (there is break for true), he doesn't need allMatch.

Kayaman Over a year ago

@egorlitvinenko that's because I flipped the (code) logic. The logic is "is each itemSetNode is contained in allUserNodes?", which is actually clearer in the inverted logic version.

egorlitvinenko · Accepted Answer · 2018-03-13 06:53:09Z

1

You also could do something like this:

Set<Id> allUserNodeIds = allUserNodes.stream()
    .map(ItemSetNode::getNodeId)
    .collect(Collectors.toCollection(TreeSet::new));
return itemSetNodeList.stream()
    .anyMatch(n -> !allUserNodeIds.contains(n.getNodeId())); // or firstMatch

Or even:

Collectors.toCollection(() -> new TreeSet<>(new YourComparator()));

answered Mar 13, 2018 at 6:53

egorlitvinenko

2,7762 gold badges20 silver badges37 bronze badges

5 Comments

Kayaman Over a year ago

Here the logic fails. You return true if even one of itemSetNodeList exists in allUserNodes. Not if even one of itemSetNodeList doesn't exist in allUserNodes.

egorlitvinenko Over a year ago

I'm reversed condition of the author. "None match of all elements if A" logically equals to "At least one match from elements if not A". And if you look more attentively, I return true even if one of itemSetNodeList not exists in allUserNodes.

Kayaman Over a year ago

Ah you're right I missed the negation. It's tricky when you start reversing the logic.

egorlitvinenko Over a year ago

Yeah, I agree with you. Readable negation in java streams is quite complex.

egorlitvinenko Over a year ago

In production code, I prefer to use method references or constants with readable names for such cases.

davidxxx · Accepted Answer · 2018-03-18 10:34:30Z

1

Terminal operations of a Stream such as noneMatch() close the Stream and make it so not reusable again.
If you need to reuse this Stream :

Stream<Id> allUserNodesStream = allUserNodes.stream().map( n -> n.getNodeId() );

just move it into a method :

public Stream<Id> getAllUserNodesStream(){
   return allUserNodes.stream().map( n -> n.getNodeId());
}

and invoke it as you need it to create it :

if (getAllUserNodesStream().noneMatch( userNode -> userNode.compareTo( nodeId ) == 0 ))

Now remember that Streams become loops in the byte code after compilation.
Performing multiple times the same loop may not be desirable. So you should consider this point before instantiating multiple times the same stream.

As alternative to create multiple streams to detect match with nodeId :

if (allUserNodesStream.noneMatch( userNode -> userNode.compareTo( nodeId ) == 0 ) ) {
  isUserMissingNode = true;
  break;
}

use rather a structure of type Set that contains all id of allUserNodes :

if (idsFromUserNodes.contains(nodeId)){
  isUserMissingNode = true;
  break;
}

It will make the logic more simple and the performance better.
Of course it supposes that compareTo() be consistent with equals() but it is strongly recommended (though not required).

edited Mar 18, 2018 at 10:34

answered Mar 12, 2018 at 19:23

davidxxx

132k23 gold badges231 silver badges228 bronze badges

3 Comments

Eugene Over a year ago

streaming again for each element... also a Supplier would more suited than a method. And while you are answering this, you could also provide may be a non-stream way that does not do so much work

Simon Arsenault Over a year ago

Unless allUserNodes can change while looping, there's no reason to execute the lambda multiple times.

davidxxx Over a year ago

@Eugene I updated with a Set that makes more sense here I think. About supplier, why would it more suited ? I am not sure to get it.

Mahesh Chikkanna · Accepted Answer · 2019-04-08 08:39:27Z

0

It will take each item from the itemSetNodeList and check it if present in the using the noneMatch(). If it is not present will get true returned. The anyMatch if atleast once item is not found will stop the search and return false. If all the item is found, we will return true.

Stream<Id> allUserNodesStream = allUserNodes.stream().map( n -> n.getNodeId() );
            boolean isUserMissing=itemSetNodeList.stream()
                            .anyMatch(n-> allUserNodes.stream().noneMatch(n));

edited Apr 8, 2019 at 8:39

answered Apr 8, 2019 at 7:14

Mahesh Chikkanna

212 bronze badges

1 Comment

Matthijs Over a year ago

Please explain your solution. More info can be found on the How do I write a good answer?

Collectives™ on Stack Overflow

Java stream use in loop

5 Answers 5

Comments

2 Comments

5 Comments

3 Comments

1 Comment

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

5 Answers 5

Comments

2 Comments

5 Comments

3 Comments

1 Comment

Your Answer

Sign up or log in

Post as a guest

Related