Storing a reference to a Stream

user_68283 0
Viewed 190

I have a class which manages a Stream:

class MyStreamManager {
    private Stream<Object> currentStream = null;

    boolean hasMoreData() {
        //code here to assert currentStream is null

        final Optional<Stream<Object>> maybeAStream = somethingWhichMightProvideAStream.getNextStream();

        currentStream = maybeAStream.orElse(null);

        return currentStream != null;
    }

    @MustBeClosed
    Stream<Object> getCurrentStream() { return currentStream; }

    void finish() {
        currentStream.close();
        currentStream = null;
    }
}

Which is used in the following style:

while (myStreamManager.hasMoreData()) {
  try {
       myStreamManager.getCurrentStream().map(...).filter(...); //etc
  } finally {
       myStreamManager.finish();
  }
}

Is storing a reference to a Stream like this bad practice? While this works, it definitely doesn't feel right, and ErrorProne is flagging it (hence the @MustBeClosed annotation).

MyStreamManager is a Spring @Bean but is only used by one thread (this is running in a batch).

I can think of two different approaches which are probably better:

  • instantiate MyStreamManager and wrap it in a try-with-resources, delegating the close() call to the Stream
  • use the Spliterators class to create a Spliterator that delegates to many Streams?
java-stream java
2 Answers

I don't think that it's as much the fact you're storing a Stream per se that makes this feel awkward, but rather that you've got sequential coupling.

You have to call hasMoreData; then getCurrentStream(); then finish(). If you're only using the class in a limited number of places, you will probably be able to get it right in all of those; but every place you use it is a new opportunity to use it incorrectly.

I would say that your manager class is actually just making things harder for yourself.

for (Optional<Stream<Object>> opt = somethingWhichMightProvideAStream.getNextStream();
     opt.isPresent();
     opt = somethingWhichMightProvideAStream.getNextStream()) {
  try (Stream<Object> stream = opt.get()) {  // try-with-resources auto-closes the stream
    stream.map(...).filter(...); //etc
  }
}

or:

Optional<Stream<Object>> opt;
while ((opt = somethingWhichMightProvideAStream.getNextStream()).isPresent()) {
  try (Stream<Object> stream = opt.get()) {
    stream.map(...).filter(...); //etc
  }
}

The loop declarations in either case are not especially pretty; but this is way shorter (roughly as long as the while/try/finally loop you already have), and harder to use wrong, I think.

(Admittedly, you've still got sequential coupling here: you have to remember to close the stream returned in the optional. Sigh.)

user_37881760

Mixing imperative (while loop, try-finally) and declarative (streams) code together doesn't seem right. If all of these opeartions are synchronous I guess it could be done in one pipeline (without MyStreamManager at all).

I think that you could think of focusing on moving some logic to object containing method somethingWhichMightProvideAStream because mixing imperative iterator pattern with stream API doesn't look like idiomatic. For example it can return List (or even better a Stream!) of Streams instead of Optional

Think twice if you really need to close this stream. From documentation:

Streams have a BaseStream.close() method and implement AutoCloseable, but nearly all stream instances do not actually need to be closed after use. Generally, only streams whose source is an IO channel (such as those returned by Files.lines(Path, Charset)) will require closing.

user_70552060
Related