HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Can this while loop be made cleaner

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
thiscanwhilemadeloopcleaner

Problem

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;
}

Solution

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();
    }
}

Code Snippets

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();
    }
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;
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();
    }
}

Context

StackExchange Code Review Q#38513, answer score: 6

Revisions (0)

No revisions yet.