Can this while loop be made cleaner

Question

Moved to Code Review as per comments received on https://stackoverflow.com/questions/20907502/can-this-while-loop-be-made-cleaner

Is there a way to make the following while loop a little more optimized? What bugs me in particular is the fact that I have to repeat code (closing buffers and returning a value) both inside and outside the if condition and I wanted to get opinions on whether there might be a better /more performance-oriented way to handle such code.

While I've posted the entire method, the part I'm more interested in comments on is how the while loop can be optimized. Ofcourse, comments on the remainder of the code are also welcome, but not essential.

    private String getRandomQuote(int lineToFetch)
        throws IOException {
    //1. get path
    AssetManager assets = getApplicationAssets();
    String path = null;
    path = getAssetPath(assets);

    //2. open assets
    InputStream stream = assets.open(path);
    InputStreamReader randomQuote = new InputStreamReader(stream);

    //3. Get BufferedReader object
    BufferedReader buf = new BufferedReader(randomQuote);

    String quote = null; 
    String line = null;
    int currLine = 0;

    //4. Loop through using the new InputStreamReader until a match is found
    while ((line = buf.readLine()) != null) {
        // Get a random line number
        if (currLine == lineToFetch) {
            quote = line;
            Log.v("LINE", line);
            randomQuote.close();
            buf.close();
            return quote;
        } else
            currLine++;
    }
    randomQuote.close();
    buf.close();
    return quote;
}

Answer 1

Nice little system, I understand though why you think this code could be better.

I think there are a few things which would make a difference. The first is a try-finally block, and the second is a simpler loop

First, the try-finally:

    InputStream stream = assets.open(path);
    try {
    //3. Get BufferedReader object
        BufferedReader buf = new BufferedReader(new InputStreamReader(stream));
        .... do things with the buffer.
    } finally {
        stream.close();
    }

This is a bit of a pre-Java7 cheat I use. It is safe to close the inner stream, and let the outer Readers (BufferedReader,InputStreamReader) not be explicitly closed. In Java7 I would put them all in a try-with-resources, but, Android, alas!.

With the above code you don't need to worry about closing the stream at all inside the try block.

OK, so now the try-block is simple, I would put two conditions (the readLine first!) in to the read-loop:

String line = null;
int currLine = 0;

//4. Loop through using the new InputStreamReader until a match is found
while ((line = buf.readLine()) != null && currLine < lineToFetch) {
    currLine++;
}

// OK, at this point, line is either null,
// or the actual line to fetch.... Whatever it is, return it,
return line;

Putting it all together, I would have something like:

private String getRandomQuote(int lineToFetch)
        throws IOException {
    //1. get path
    AssetManager assets = getApplicationAssets();
    String path = null;
    path = getAssetPath(assets);

    //2. open assets
    InputStream stream = assets.open(path);
    try {
        //3. Get BufferedReader object
        BufferedReader buf = new BufferedReader(new InputStreamReader(stream));
        String line = null;
        int currLine = 0;

        //4. Loop through using the new InputStreamReader until a match is found
        while ((line = buf.readLine()) != null && currLine < lineToFetch) {
            currLine++;
        }

        // OK, at this point, line is either null,
        // or the actual line to fetch.... Whatever it is, return it,
        return line;

    } finally {
        stream.close();
    }
}

Answer 2

There are simplifications that can be made to rolfl's answer:

private String getRandomQuote(int lineToFetch) throws IOException {
    Reader buf = getQuoteDataSource();

    try {
        String line = buf.readLine();

        while( --lineToFetch > 0 && line != null ) {
          line = buf.readLine();
        }

        return line == null ? "" : line;
    } finally {
        buf.close();
    }
}

How the assets are opened is superfluous. Maybe the quotes come from a database, maybe they come from a file, or maybe they are downloaded across an HTTP connection.

I seldom return null values, preferring valid objects that won't inadvertently cause a NullPointerException when referenced. (See also Tony Hoare's Billion Dollar mistake.)

If a null value is truly unexpected, throw an exception instead. Force the calling code to handle missing quotes at compile time, rather than relying on developer memory.

Note: There might be an off-by-one error, so test.