Avoiding side-efects of peek() and forEach()

Question

This is a bit of a fundamental question with an example project, as I'm still learning the best practices of Java 8 features.

Say I have an Order object, that cointains a List of OrderDetail. At the same time, OrderDetail contains a source and a destiny, along with quantity and product. OrderDetail also has an order (fk).

For this example, I will be moving Products from source to destiny, which both are ProductWarehouse objects with an availableStock property, which will be affected by the result of the Order.

Now, I need to update the availableStock for all the sources and destiny-ies. The stock should increase for destiny-ies and decrease for sources by the quantity of the OrderDetail. Since destiny and source are of the same type I can update them at the same time. The problem is that, of course, they are different properties.

My first thought was as follows:

//Map keeps record of the sum of products moved from the source. Key is the id the Product
HashMap<Integer, Double> quantities;

ArrayList<OrderDetail> orderDetails;

/**
*  This could've been a single stream, but I divided it in two for readability here
*  If not divided, the forEach() method from ArrayList<> might been enough though
*/
public void updateStock() {
    List<ProductWarehouse> updated = new ArrayList<>();
    orderDetails.stream()
        .map(OrderDetail::getSource)
        .forEach(src -> {
            src.setAvailableStock(src.getAvailableStock - quantities.get(src.getProductId()));
            updated.add(src);
        });
    orderDetails.stream()
        .map(OrderDetail::getDestiny)
        .forEach(dst -> {
            dst.setAvailableStock(dst.getAvailableStock + quantities.get(dst.getProductId()));
            updated.add(dst);
        });
    productWarehouseRepository.save(updated);
}

While this works, there is a problem: This is like the example in the Javadoc about "unnecesary side efects". It is not strictly the same, but given how similar they are makes me think I'm taking avoidable risks.

Another option I thought of is using peek() which has it's own implications since that method was thought mainly as a debugging tool, according to the javadoc.

HashMap<Integer, Double> quantities;
ArrayList<OrderDetail> orderDetails;

/**
* Either make it less readable and do one save or do one save for source and one for destiny
* I could also keep the updated list from before and addAll() the result of each .collect()
*/
public void updateStock() {
    productWarehouseRepository.save(
        orderDetails.stream()
            .map(/*Same as above*/)
            .peek(src -> {/*Same as above*/})
            .collect(toList()) //static import
    );
}

I've also read in a few blog post around that peek should be avoided. This, again, because the doc says what it's usage should be (mostly).

Hence the question: If I'd want to modify the properties of a Collection using the Stream API, what would be the best way to do so, following best-practices? For an example like this, is it better to use Iterable.forEach() or a simple enhanced for-loop? I honestly don't see where the side-effect might arise with operations like these using Stream, but that's probably due lack of experience and understanding of the API.

The code in this quesiton is just for example purposes, but if it's easier to understand with models or more info, I can add it.

It seems to me you're in a case where using a stream doesn't make sense and just complicates your problem. A simple for loop would make for obvious code — Aaron, Sep 02 '21 at 18:07
@Aaron yeah, I was thinking that too. Other than the `.map()`, I don't find anything particularly useful out of `Stream` for this case and I need to worry about side-effects. — Jetto Martínez, Sep 02 '21 at 19:14
@OneCricketeer Sorry, it was meant to be `src` and `dst`. My bad, I'll fix it. — Jetto Martínez, Sep 02 '21 at 19:35

score 3 · Answer 1 · answered Sep 03 '21 at 10:03

You want to perform two different operations.

Update each ProductWarehouse object
Collect all ProductWarehouse objects into a list, for the save

Don’t mix the two operations.

public void updateStock() {
    orderDetails.forEach(o -> {
        ProductWarehouse src = o.getSource();
        src.setAvailableStock(src.getAvailableStock()-quantities.get(src.getProductId()));
        ProductWarehouse dst = o.getDestiny();
        dst.setAvailableStock(dst.getAvailableStock()+quantities.get(dst.getProductId()));
    });
    List<ProductWarehouse> updated = orderDetails.stream()
        .flatMap(o -> Stream.of(o.getSource(), o.getDestiny()))
        .collect(Collectors.toList());
    productWarehouseRepository.save(updated);
}

There is no point in trying to perform everything in a single stream operation at all costs. In the rare case where iterating the source twice is expense or not guaranteed to produce the same elements, you can iterate over the result list, e.g.

public void updateStock() {
    List<ProductWarehouse> updated = orderDetails.stream()
        .flatMap(o -> Stream.of(o.getSource(), o.getDestiny()))
        .collect(Collectors.toList());
    for(int ix = 0; ix < updated.size(); ) {
        ProductWarehouse src = updated.get(ix++);
        src.setAvailableStock(src.getAvailableStock()-quantities.get(src.getProductId()));
        ProductWarehouse dst = updated.get(ix++);
        dst.setAvailableStock(dst.getAvailableStock()+quantities.get(dst.getProductId()));
    }
    productWarehouseRepository.save(updated);
}

This second iteration is definitely cheap.

I'm yet to properly understand `flatMap`, but I take it pretty much does ithe same as `map` (in this example), yes? Since they both take a Function. I didn't thought I could map them at the same time. It makes it easier to read IMO, especially modifying them first. Thank you! — Jetto Martínez, Sep 03 '21 at 20:13
@JettoMartínez `map` always produces a single element in its output stream for each element in its input stream. `flatMap` on the other hand can produce multiple elements on its output stream from a single element of its input stream. A typical example of its use would be to produce a collection of orders from a collection of clients, clients which may have passed multiple orders. — Aaron, Sep 04 '21 at 11:21

score 1 · Answer 2 · answered Sep 02 '21 at 19:31

1

In both cases, forEach is a terminal action, so you should use map instead

If you want to avoid side-effects, then call your setter method within a map, then collect that to a list, which you add to the outer list.

updated.addAll(orderDetails.stream().map()...collect(Collectors.toList())

Related solution - How to add elements of a Java8 stream into an existing List

answered Sep 02 '21 at 19:31

OneCricketeer

179,855
19
132
245

So yeah, I will need at least three new lists to use `save` once or two lists and use two `save`s, performing my changes on each List. `map` seems like the only useful feature for this particular case. Btw thank you for the link. It is a very good read. – Jetto Martínez Sep 02 '21 at 19:52

Avoiding side-efects of peek() and forEach()

2 Answers2