1
\$\begingroup\$
fun readLines(path: String): Stream<String> =
    try {
        Files.lines(Paths.get(path)).use { it }
    } catch (ex: Exception) {
        ex.printStackTrace()
        Stream.empty<String>()
    }

I added .use { it } to tack on try-with-resources like behaviour for autoclosing the file handle (?) in case of exceptions.

  1. Is this required?
  2. Are there better approaches?
\$\endgroup\$
3
  • 1
    \$\begingroup\$ The current question title, which states your concerns about the code, is too general to be useful here. Please edit to the site standard, which is for the title to simply state the task accomplished by the code. Please see How to get the best value out of Code Review: Asking Questions for guidance on writing good question titles. \$\endgroup\$ – Toby Speight Jan 6 '20 at 10:29
  • \$\begingroup\$ Your code doesn't work. It always throws a java.lang.IllegalStateException: stream has already been operated upon or closed. \$\endgroup\$ – Roland Illig Jan 6 '20 at 17:07
  • 1
    \$\begingroup\$ Maybe reformat the post to a stackoverflow post which asks how to return sequence that closes file at the end, or something like that? \$\endgroup\$ – tieskedh Jan 7 '20 at 7:51
2
\$\begingroup\$

Your situation

Use closes the resource at the end of the block (lambda).
This means that the stream is closed before returning it.

The problem with reading lines

The file needs to be closed once you have read the needed lines.
The stream could close the file once the stream last item is reached.
The problem with this approach is that functions like limit, take, splititerator etc. are making it possible that the last item is never reached.
Therefor, you can do two things. The first option is to let the caller close the file or sequence/stream/whatever you return.

The second option is to close inside your function, but then you need to do all the reading inside the function, so before you return from the function.

Options

You can read everything immediately by storing everything in a list using readlines. (This means you read the whole file into memory, which means you should not do this for huge, huge, huge files, you almost always can ignore this warning).

fun readLines(path: String): List<String> = try{
    Paths.get(path).toFile().readLines()
} catch(ex: Exception) {
    ex.printStackTrace()
    emptyList<String>()
}

The other approaches work by moving the code you would normally do outside the function inside the function.

For example the forEachLine:

fun forEachLine(path: String, action: (String)->Unit){
    try{
        Paths.get(path).toFile().forEachLine(action)
    } catch(ex: Exception) {
        ex.printStackTrace()
    }
}

With that function you could rewrite

val t = readLines("somePath")
    .filter{ it.startsWith("ok:") }
t.forEach{ println(it) }
t.close()

with

forEachLine("somePath"){
    if(it.startsWith("ok:")
        println(it)
}

If you have functions that operates on a sequence of strings, or want more than a simple forEach, you can access the sequence itself by using useLines, but again move the reading inside the function:

fun useLines(path: String, action: (Sequence<String>)->Unit){
    try{
        Paths.get(path).toFile().useLines(action)
    } catch(ex: Exception) {
        ex.printStackTrace()
    }
}

Can replace the former example with:

useLines("somePath"){ lines->
    lines.filter{ it.startsWith("ok") }
       .forEach{ println("it") }
} 

NOTE: this function is not for extracting or returning the sequence.
all the functions needs to be done in the lambda,until you come across a terminal operation, an operation that no longer returns a sequence.

NOTE: sequence is the Kotlin-version of streams with lots of benefits.

Is closing required

from the javadoc

"...Generally, only streams whose source is an IO channel (such as those returned by Files.lines(Path, Charset)) will require closing ..."

so yes.

huge files

You should not use readlines on huge files.
What is a huge file?

The bible has around 31,000 lines which can have 20 bytes reference, so 620,000 bytes or roughly 0,5 mb.
The bible has 3,000,000 characters which is roughly 12 mb.
Acording to this post, Android has a memory limit from around 15mb (in the worst case).
This means that reading the bible in it entirely would theoretically be OK.
So, untill you run into problems, this is nothing to worry about.

Edit:

I don't completely know how flatmap in Java excactly works at the moment.
There was a bug that flatmap was not lazy. This means that calling fileNames.flatMap(::readLines) with the readLines from your code was calculating the complete list before moving on: link.
This bug is fixed, so I don't know what it does at the moment.
If it still works the same way, it always closes the file for you, but it does so because it always parses the complete file.

In every case, if you are returning the Java-stream, calling close on the returning Stream is enough to close the file. With sequences, this is not possible, as sequences are not closeable (They also don't know about AutoCloseable/Closeable).

\$\endgroup\$
5
  • \$\begingroup\$ So now I'm returning File(path).useLines { it } as a Sequence. I understand from the doc that it "closes the reader once the processing is complete". However, nowhere is it mentioned if that is still done if an exception occurs. Any insights/ resources (this is server-side code and not Android, btw)? \$\endgroup\$ – Slaiyer Jan 6 '20 at 12:04
  • 1
    \$\begingroup\$ @Slaiyer Changed to examples, you can only access the sequence from inside the lambda. (otherwise the result is the same as your initial post) \$\endgroup\$ – tieskedh Jan 6 '20 at 16:56
  • \$\begingroup\$ I like the Paths.get(path).toFile().useLines(action) syntax, but I don't want to perform any action here. I just want to return a Sequence<String> of raw lines from each file from the surrounding flatMap that will be processed in a chain defined in the outer scope like .flatMap(::readLines).filter(CharSequence::isNotEmpty), as opposed to bringing in the filter in as an action block. What can I do with useLines to achieve this, if not .useLines { it }? \$\endgroup\$ – Slaiyer Jan 7 '20 at 0:11
  • \$\begingroup\$ The ideal would be a generic, non-blocking line reader that enables lazy evaluation of arbitrary operations without sacrificing flexibity (no mandatory action block) or housekeeping (autoclosing resources irrespective of exceptions), with fairly readable, concise syntax. \$\endgroup\$ – Slaiyer Jan 7 '20 at 0:52
  • 1
    \$\begingroup\$ If you are lazy reading, it is not possible for the function to know when you stop reading. Therefor, the function cannot close the stream for you. So, you need to call close in the stream once you stop reading. The other solution would be to implement the entire stream yourself. Note: if you would call flatmap on the stream (not on sequence), it would close the file for you. I bet, however, that you won't remember this all the time and so will run into problems problems later. It you need to place flatmap inside the function, but that would be strange, I think. So, your goal is impossible. \$\endgroup\$ – tieskedh Jan 7 '20 at 7:34

Not the answer you're looking for? Browse other questions tagged or ask your own question.