Optimize method by reducing amout of code/operations on list

Question

I want to reduce overhead by merging my operations into one but don't seem to quite figure out how to complete my code without errors

Currently I have this code that works:

public Map<String, Invoice> initialize(List<String> paths) {
    List<Invoice> invoices = paths
        .stream()
        .map(Invoice::new)
        .collect(Collectors.toList());

    invoices
        .forEach(e -> {
            e.setInvoiceInputStream(reader(e.getInvoicePath()));
            e.setInvoiceId(invoiceFinder.getInvoiceId(e.getInvoiceInputStream()));
        });

    Map<String, Invoice> invoiceMap = invoices
        .stream()
        .collect(
                Collectors.toMap(
                        e -> e.getInvoiceId(),
                        e -> e)
                );

return invoiceMap;

However, executing this code 3 times seems a waste of time. If I try something different like I get errors:

return invoicePaths
    .stream()
    .map(Invoice::new)
    .collect(
        Collectors.collectingAndThen(
            Collectors.toList(), list -> {
                list
                    .forEach(e -> {
                        e.setInvoiceInputStream(reader(e.getInvoicePath()));
                        e.setInvoiceId(invoiceFinder.getInvoiceId(e.getInvoiceInputStream()));
});

Constructor in Invoice class:

public Invoice(String invoicePath) {
    this.invoicePath = invoicePath;
}

How can I reduce and overhead by optimizing my code?

"However, executing this code 3 times seems a waste of time." Have you benchmarked this to confirm that it's actually an issue? How long is it taking, and for how many paths? — Jon Skeet, Jul 02 '19 at 13:22
this question seams better fit for https://codereview.stackexchange.com/ — HopefullyHelpful, Jul 02 '19 at 13:23
Have you thought about changing your data model instead of the method? It looks weird to create a partially initialized `Invoice` (that needs to somehow retrieve its Id from an extra service) and that stores an InputStream that is created externally. — sfiss, Jul 02 '19 at 14:11
@sfiss your suspicion is right. It points to the even more questionable thing, that apparently an `InputStream` (or is it actually a `Reader`) is treated like a property of the object while the operation looks like consuming it. — Holger, Jul 02 '19 at 16:31

score 2 · Accepted Answer · answered Jul 02 '19 at 13:21

2

 paths
    .stream()
    .map(Invoice::new)
    .map(e -> {
           e.setInvoiceInputStream(reader(e.getInvoicePath()));
           e.setInvoiceId(invoiceFinder.getInvoiceId(e.getInvoiceInputStream()));
           return e;

    })
    .collect(Collectors.toMap(
                  Invoice::getInvoiceId,
                  Function.identity())
            );

I hope I did not miss any brackets...

Think about it, you are collecting to List<Invoice> invoices only to forEach them and change something - in't that a Stream::map operation? And after that you are collecting these to a Map => just continue the stream pipeline after that.

answered Jul 02 '19 at 13:21

Eugene

117,005
15
201
306

I added the comment above already, but `map` is usually not supposed to mutate the element. – sfiss Jul 02 '19 at 14:07
@sfiss why not? this is not a change that brakes the integrity of the source of the stream - so this is more than ok – Eugene Jul 02 '19 at 14:09
I thought the contract of `map`mentions it, but it only mentions `non-interference` (see also https://stackoverflow.com/questions/32837415/non-interference-exact-meaning-in-java-8-streams). So I guess mutating the elements is fine, even though I always try to stay away from that if possible and reasonable. Maybe it is because state mutation and functional programming (which is the paradigm where I use `map` most often) don't fo together very well. – sfiss Jul 02 '19 at 14:20
1

The mutation can easily be removed by combining the two map calls, as the mutated object is just created on the previous step. Although it doesn't really make any difference, as the object itself is not immutable... – vlumi Jul 03 '19 at 00:02

vlumi · Answer 2 · 2019-07-03T00:00:17.397

It doesn't seem like you are doing anything useful in the middle loop -- why not put that in e.g. a map?

public Map<String, Invoice> initialize(List<String> paths) {
    return paths
        .stream()
        .map(path -> {
            Invoice e = new Invoice(path);
            e.setInvoiceInputStream(reader(e.getInvoicePath()));
            e.setInvoiceId(invoiceFinder.getInvoiceId(e.getInvoiceInputStream()));
            return e;
        })
        .collect(
                Collectors.toMap(
                        e -> e.getInvoiceId(),
                        e -> e)
                );
}```

Optimize method by reducing amout of code/operations on list

2 Answers2