@Pspeed : Could you please detail your example ? I can’t see what’s bad in it… Except that not null does not equal to opened, of course.
@yang71 said: @Pspeed : Could you please detail your example ? I can't see what's bad in it.... Except that not null does not equal to opened, of course.
There are variations on this idiom, I’m sure you will recognize if if you’ve been around the open source web a few times:
Variation 1:
[java]InputStream in = null;
try {
in = new FileInputStream(“someFile”);
read from in
} catch( IOException e ) {
log the exception
} finally {
if( in != null ) {
in.close();
}
}[/java]
The above is just wrong… no reason to fold the new FileInputStream() into the try/catch block in the first place and it’s littered with state problems… but the issue is the same if you move it out…
Variation 2:
[java]InputStream in = new FileInputStream(“someFile”);
try {
read from in
} catch( IOException e ) {
log the exception
} finally {
if( in != null ) {
in.close();
}
}[/java]
So, the problem here is that in will never be null in finally{} unless there is something really dodgy going on in “read from in” where it sets it to null. So in the best case, the “if( in != null )” is doing nothing. In the worst case, it’s hiding a bug and leaking file handles.
It’s a similar issue to the:
[java]
Object someValue = null;
if( foo ) {
// do a bunch of stuff
someValue = something;
} else if( bar ) {
// do a bunch of stuff
someValue = somethingElse;
} else {
someValue = somethingElseAgain;
}
// Do something with someValue
[/java]
If I were to glance through this code, I’d assume the if/else block did not always initialize someValue and that there should be an if( someValue != null ) check below the block. And in a code review, if the developer argued otherwise “Because the block will always set it” then I’d say that a buggy if/else block would go unnoticed until runtime. The initializing someValue to null prior to the block hides potential bugs and solves no problems. At best it does nothing, at worst is hides an edge case where the if block falls through without initializing some of its values, ie: there is effectively no default branch.
So it’s a case where “I just wanted to be safe” is actually potentially hiding a bug and is less safe than not setting it at all. Like the input stream case.
1 Like
Ok, I understand what you mean in variation 2 and 3.
However, in variation 1, what occurs if the File-Open fails ? In this case, the ‘in’ would be null, actually.
In variation 2, this case is not even caught…
So the right way to do this would be a double try/catch (?) :
[java]try{
FileInputStream in = new FileInputStream(fileName);
try {
// Read from in
} catch (IOException ex) {
…
} finally {
in.close();
}
} catch (FileNotFoundException e) {
…
}[/java]
Normally I agree with @pspeed on pretty much everything he says but I’m not entirely convinced by the try/catch/finally one (whereas I agree about the initialize to null).
try/finally is the correct way to open a resource and then ensure it is closed whatever route is used to exit the method. Unless you mean the checking for it not being null? In which case I have a counter argument where that check is valid:
[java]try {
in = new FileInputStream(“someFile”);
read from in
} catch( IOException e ) {
log the exception
} finally {
if( in != null ) {
in.close();
}
}
[/java]
In this case the only way in is null on the finally is if the new FIS() call threw an exception. However if it did (which would then be logged) there would be then a second error logged from the NPE and the entire method would throw a NPE from the finally block as opposed to having caught and handled the error internally.
[java]try {
in = new FileInputStream(“someFile”);
read from in
} finally {
if( in != null ) {
in.close();
}
}
[/java]
In this case your method is not catching (and hence is throwing) whatever exception happens inside the try. If the new FIS had failed for whatever reason then a new exception would be generated in the finally block and I’m honestly not sure what would happen now but it wouldn’t surprise me in the slightest if the original exception just got wiped out - and now you have no useful information at all.
[java]try {
in = new FileInputStream(“someFile”);
out = new FileOutputStream(“someOtherFile”);
read from in
write to out;
} catch( IOException e ) {
log the exception
} finally {
if( in != null ) {
in.close();
}
if (out != null) {
out.close();
}
}
[/java]
Here it is even worse, depending on what happened you could hit the finally block with one, either, both or neither of in and out initialised - and a failure to open one would then cause the other to not be closed.
Yes you could remove the null check and simply make sure you always close in the order in which you open - but that’s now very fragile (particularly as this method grows so you can’t see it all in one glance) as making a change up in the top of the block that happens to change the order of things means you need to remember to go make the same order change down in the finally block…
So…while I agree about the “always initialize to null” thing - I disagree about the finally null check thing in this specific case 
1 Like
@yang71 said: Ok, I understand what you mean in variation 2 and 3.However, in variation 1, what occurs if the File-Open fails ? In this case, the ‘in’ would be null, actually.
In variation 2, this case is not even caught…So the right way to do this would be a double try/catch (?) :
[java]try{
FileInputStream in = new FileInputStream(fileName);
try {
// Read from in
} catch (IOException ex) {
…
} finally {
in.close();
}
} catch (FileNotFoundException e) {
…
}[/java]
Usually you would throw IOException from the method… you need to catch it for in.close() also anyway. So you have to have some outer handling of IOException or you make really ugly granular try/catches in the middle.
Ok, thanks for the advice.
@zarch said: Normally I agree with @pspeed on pretty much everything he says but I'm not entirely convinced by the try/catch/finally one (whereas I agree about the initialize to null).try/finally is the correct way to open a resource and then ensure it is closed whatever route is used to exit the method. Unless you mean the checking for it not being null? In which case I have a counter argument where that check is valid:
[java]try {
in = new FileInputStream(“someFile”);
read from in
} catch( IOException e ) {
log the exception
} finally {
if( in != null ) {
in.close();
}
}
[/java]
In this case the only way in is null on the finally is if the new FIS() call threw an exception. However if it did (which would then be logged) there would be then a second error logged from the NPE and the entire method would throw a NPE from the finally block as opposed to having caught and handled the error internally.
in.close() will also throw IOException (my rant on checked exceptions is a separate discussion) so you will already have to be handling IOException at the outer level. Might as well move the new FileInputStream outside the try.
As written, if your try{} block inadvertently sets in to null then you leak a file handle. Admittedly, it’s a small chance but why ugly up code and potentially hide a bug when there is a simpler idiom?
@zarch said: [java]try { in = new FileInputStream(“someFile”); read from in } finally { if( in != null ) { in.close(); } } [/java] In this case your method is not catching (and hence is throwing) whatever exception happens inside the try. If the new FIS had failed for whatever reason then a new exception would be generated in the finally block and I'm honestly not sure what would happen now but it wouldn't surprise me in the slightest if the original exception just got wiped out - and now you have no useful information at all.
In this code, there is no reason to have the new FileInputStream inside the try{} because the exception handling will be identical either way and you’ve only complicated your finally{} state. The same potential bug lurks if the try{} block sets in to null.
@zarch said: [java]try { in = new FileInputStream(“someFile”); out = new FileOutputStream("someOtherFile"); read from in write to out; } catch( IOException e ) { log the exception } finally { if( in != null ) { in.close(); } if (out != null) { out.close(); } } [/java] Here it is even worse, depending on what happened you could hit the finally block with one, either, both or neither of in and out initialised - and a failure to open one would then cause the other to not be closed. Yes you could remove the null check and simply make sure you always close in the order in which you open - but that's now very fragile (particularly as this method grows so you can't see it all in one glance) as making a change up in the top of the block that happens to change the order of things means you need to remember to go make the same order change down in the finally block....
In this last case, I wouldn’t structure the code that way at all anyway. But if you are going to nest all of this input/output stream stuff together like that it seems reasonable to expect an ordering. But it’s an ugly block anyway. You will never get around at least one null check and you’ve created some completely one-use code.
Instead:
[java]
void writeToFile( File f, InputStream in ) throws IOException {
FileOutputStream out = new FileOutputStream(f);
try {
read from in
write to out
} finally {
out.close();
}
}
void copyFile( File from, File to ) throws IOException {
FileInputStream in = new FileInputStream(from);
try {
writeToFile( to, in );
} finally {
in.close();
}
}
[/java]
And then there is no issue and you can write from any stream source, as well.
Even if you want to generically handle some copy( InputStream in, OutputStream out) it can still be outer nested this way. Something has to create the streams and it’s generally best if that something is the same thing that closes them. (For example, the inner method cannot know if it’s proper to close any of the streams… maybe you are appending a bunch of stuff together, etc…)
1 Like