0

Is there any easy way to convert this code into java 8 using streams and forEach.

List<List<IStitchable>> listOfEnvelopes = new ArrayList<>();
List<IStitchable> statementsInEnvelope = new ArrayList<>();
int totalPages = 0;

for(byte[] byteArray :byteArrays){
    totalPages = totalPages + getPagesInDocument(byteArray);

    if(totalPages/2>MAX_SHEETS_FOR_OVERSIZE_ENVELOPE){
        totalPages = 0;
        listOfEnvelopes.add(statementsInEnvelope);
        tatementsInEnvelope = new ArrayList<IStitchable>();
    }

    statementsInEnvelope.add(createStitchableFromSnapshot(byteArray));
} 

I tried to do something like

byteArrays.stream().forEach(byteArray->{
    totalPages = totalPages + getPagesInDocument(byteArray);            
    if(totalPages/2>MAX_SHEETS_FOR_OVERSIZE_ENVELOPE){
        totalPages = 0;
        listOfEnvelopes.add(statementsInEnvelope);
        statementsInEnvelope = new ArrayList<IStitchable>();
    }
    statementsInEnvelope.add(createStitchableFromSnapshot(byteArray));
});

but then I cannot use totalPages and statementsInEnvelope within ForEach, they cannot be accessed from the forEach scope
Lastly is there any cleaner way to refactor this thing. Any ideas would be appreciated.

Imran Jawaid
  • 471
  • 1
  • 10
  • 27
  • I am questioning whether the resulting code is better than the previous one. The effect on readability is debatable. Also, now you are paying the price of using an AtomicInteger (extra allocation, extra pointer dereference in the heap, reading and updating a volatile field) – JnRouvignac Dec 08 '17 at 19:08
  • the "easiest" thing to do here is to spin a custom collector, it's not that complicated to write it and here is a reference for one for example: https://stackoverflow.com/questions/47645399/java-8-stream-defining-collectors-based-on-other-collectors/47649011#47649011 – Eugene Dec 10 '17 at 20:11

1 Answers1

1

The simplest solution is to use AtomicInteger:

List<List<IStitchable>> listOfEnvelopes = new ArrayList<>();     
List<IStitchable> statementsInEnvelope = new ArrayList<>();
AtomicInteger totalPages = new AtomicInteger();

byteArrays.stream().forEach(byteArray -> {
  totalPages.addAndGet(getPagesInDocument(byteArray));

  if(totalPages.get() / 2 > MAX_SHEETS_FOR_OVERSIZE_ENVELOPE){
    totalPages.set(0);
    listOfEnvelopes.add(new ArrayList<>(statementsInEnvelope));
    statementsInEnvelope.clear();
  }
  statementsInEnvelope.add(createStitchableFromSnapshot(byteArray));
});
Adam Siemion
  • 15,569
  • 7
  • 58
  • 92
  • but even that does not solve the problem of accessing statementsInEnvelope within the forEach. It is declared outside the scope of forEach. – Imran Jawaid Dec 08 '17 at 16:26
  • make statementsInEnvelope `final`? – Ivonet Dec 08 '17 at 16:31
  • Yes, it does solve it. Please note that i changed the assignment to `clear()` – Adam Siemion Dec 08 '17 at 16:41
  • variables accessed in lambdas have to be final or effectively final - in the above snippet they are effectively final – Adam Siemion Dec 08 '17 at 16:42
  • and please note that `listOfEnvelopes` gets a clone of `statementsInEnvelope` (`new ArrayList<>(statementsInEnvelope)`) – Adam Siemion Dec 08 '17 at 16:44
  • 1
    @AdamSiemion I think you could use an array with a single element instead of `AtomicInteger` here, it is a too expensive holder for this sort of the problem – Eugene Dec 10 '17 at 20:06
  • 1
    @AdamSiemion not to speak of the fact that `forEach` updates an external collection which is explicitly forbidden by documentation. The cleanest way would be to spin a custom collector instead, like this for example: https://stackoverflow.com/questions/47645399/java-8-stream-defining-collectors-based-on-other-collectors/47649011#47649011 – Eugene Dec 10 '17 at 20:07
  • @Eugene I fully agree, can you however provide the documentation that says `forEach` cannot update an external collection? – Adam Siemion Dec 14 '17 at 12:31
  • @AdamSiemion this was just a matter of saying that streams disallow side-effects – Eugene Dec 14 '17 at 12:47