patternjavaMinor
Can this while loop be made cleaner
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.
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:
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:
Putting it all together, I would have something like:
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.