using and finally statement

[tshad]

Do I still need to close an object in a finally statement after a catch if
it is part of a using statement?

For example:
FileStream fs = null;

try
{
using (FileInfo fInfo = new FileInfo(fromImagePath + "\\" +
(string)dr["originalFileName"]))
{
if (fInfo.Exists)
{
numBytes = fInfo.Length;

// Read the file into the byte array

using (fs = new FileStream(fromImagePath + "\\" +
(string)dr["originalFileName"], FileMode.Open, System.IO.FileAccess.Read))
{
using(BinaryReader br = new BinaryReader(fs))
myByteArray = br.ReadBytes((int)numBytes);
}

// Write out the file into the images folder with new name

using (fs = System.IO.File.OpenWrite(imagePath + imageName))
{
fs.Write(myByteArray, 0, myByteArray.Length);
}
}
}
}
catch (Exception exc)
{
}
finally
{
if (br != null)
br.Close();
if (fs != null)
fs.Close();
}

Do I need to use the finally code or does the "using" automatically close
the objects (fs and br)?

Also, when the you exit the using statement (with or without an error -
which would send you to the catch clause), would the an object that was
defined outside of the using statement be null?

Thanks,

Tom

 

[Jon Skeet [C# MVP]]

Do I still need to close an object in a finally statement after a catch if
it is part of a using statement?

No. The whole point of a using statement is to do that automatically.

I would tend to only declare the FileStream fs in each using statement,
rather than sharing one variable for two uses.

(Note that FileInfo doesn't implement IDisposable, so your code
wouldn't actually compile at the moment.)

Also, when the you exit the using statement (with or without an error -
which would send you to the catch clause), would the an object that was
defined outside of the using statement be null?

No, it would have the same value, it's just the object would have been
disposed and therefore not terribly useful. That's one reason to
declare the variable in the using statement itself - it stops you from
trying to use an object which has already been disposed.

 

[tshad]

No. The whole point of a using statement is to do that automatically.

I would tend to only declare the FileStream fs in each using statement,
rather than sharing one variable for two uses.

(Note that FileInfo doesn't implement IDisposable, so your code
wouldn't actually compile at the moment.)


No, it would have the same value, it's just the object would have been
disposed and therefore not terribly useful. That's one reason to
declare the variable in the using statement itself - it stops you from
trying to use an object which has already been disposed.

Makes sense.

In the following:

using (fs = new FileStream(fromImagePath + "\\" +
(string)dr["originalFileName"], FileMode.Open, System.IO.FileAccess.Read))
{
using(BinaryReader br = new BinaryReader(fs))
myByteArray = br.ReadBytes((int)numBytes);
}

Is putting the BinaryReader in the using statement a little overkill?

Thanks,

Tom

 

[Jon Skeet [C# MVP]]

Makes sense.

In the following:

using (fs = new FileStream(fromImagePath + "\\" +
(string)dr["originalFileName"], FileMode.Open, System.IO.FileAccess.Read))
{
using(BinaryReader br = new BinaryReader(fs))
myByteArray = br.ReadBytes((int)numBytes);
}

Is putting the BinaryReader in the using statement a little overkill?

Yes, but I tend to do it anyway - it's better to get into the habit of
always disposing of things, and there can be wrapping cases like this
where it *does* make a difference (because closing the inner resource
may flush pending writes to the outer resource).

 

[tshad]

No. The whole point of a using statement is to do that automatically.

I would tend to only declare the FileStream fs in each using statement,
rather than sharing one variable for two uses.

(Note that FileInfo doesn't implement IDisposable, so your code
wouldn't actually compile at the moment.)


No, it would have the same value, it's just the object would have been
disposed and therefore not terribly useful. That's one reason to
declare the variable in the using statement itself - it stops you from
trying to use an object which has already been disposed.

So there would be no reason to do this:

Font font2 = new Font("Arial", 10.0f);
using (font2)
{
// use font2
}


or

FileStream fs = null;
using (fs = new FileStream(originalFileName, FileMode.Open,
System.IO.FileAccess.Read))
{
using(BinaryReader br = new BinaryReader(fs))
myByteArray = br.ReadBytes((int)numBytes);
}

since fs would not be closed (or would it?).

Thanks,

Tom

 

[tshad]

No. The whole point of a using statement is to do that automatically.

I would tend to only declare the FileStream fs in each using statement,
rather than sharing one variable for two uses.

So you wouldn't do the following?:

FileStream fs = null;
try
{
fs = System.IO.File.OpenWrite(imagePath + imageName);
fs.Write(myByteArray, 0, myByteArray.Length);
fs.Close();

if (dr["thumbnailImage"].ToString() != "")
{
myByteArray2 =
Convert.FromBase64String(dr["thumbnailImage"].ToString());
if (myByteArray2.Length > 0)
{
thumbnailName = appraisalID.ToString() + "_" +
(imageKtr).ToString() + "_Thumb" + "." + imageExtension;
fs = System.IO.File.OpenWrite(imagePath + thumbnailName);
fs.Write(myByteArray2, 0, myByteArray2.Length);
fs.Close();
}
}
}
catch(Exception exc)
{
}
finally
{
if (fs=null)
fs.Close();
}

There could also be 2 or 3 other fs assignments for moving some other files.

Also, I am not using "new" for any of these - does that make a difference?

Thanks,

Tom

 

[tshad]

In each example, the resource will be _disposed_ (not necessarily
closed). The main issue is that the variable would still be in scope
after you disposed the object, which in most cases would be pointless
and could lead to incorrect code.

For many objects, Close() and Dispose() do the exact same thing. But
you shouldn't rely on that as a general rule. The "using" statement
disposes things, not closes things. If the docs promise you that
Dispose() and Close() are identical, it's a reasonable shortcut, but
otherwise you should get into the habit of doing both.

But if I change my code to:

using (File Stream fs = new FileStream(originalFileName, FileMode.Open,
System.IO.FileAccess.Read))
{
using(BinaryReader br = new BinaryReader(fs))
myByteArray = br.ReadBytes((int)numBytes);
}

Would I still do the Close() on both objects? Something like:

using (File Stream fs = new FileStream(originalFileName, FileMode.Open,
System.IO.FileAccess.Read))
{
using(BinaryReader br = new BinaryReader(fs))
myByteArray = br.ReadBytes((int)numBytes);
fs.Close();
br.Close();
}

I assume that you wouldn't use the "Closes" in a finally clause outside of
the using statement since the fs and br variables would be out of scope.

Thanks,

Tom

 

[tshad]

I can't speak for Jon. But I'd for sure make sure it at least
compiled (you have a syntax error in your final "if" statement).

Actually, it does compile. Which error?

Seriously though, especially assuming you've got the try/catch there
anyway, I don't see anything wrong with that particular pattern per
se.
However, I personally would more likely have a complete "using" with
variable declaration around each place I'm actually using a
FileStream. Is it necessary to write it that way? No. But it is
IMHO slightly cleaner looking, and helps avoid code that might
mistakenly use the "fs" variable when it's not currently representing
something useful.

I guess it is cleaner looking.

So you would write it like (without the trys)?:

using(fs = System.IO.File.OpenWrite(imagePath + imageName))
fs.Write(myByteArray, 0, myByteArray.Length);

if (dr["thumbnailImage"].ToString() != "")
{
myByteArray2 =
Convert.FromBase64String(dr["thumbnailImage"].ToString());
if (myByteArray2.Length > 0)
{
thumbnailName = appraisalID.ToString() + "_" +
(imageKtr).ToString() + "_Thumb" + "." + imageExtension;

using(fs = System.IO.File.OpenWrite(imagePath + thumbnailName))
fs.Write(myByteArray2, 0, myByteArray2.Length);
}
}

Thanks,

Tom

[...]

Pete

 

[Jon Skeet [C# MVP]]

But if I change my code to:

using (File Stream fs = new FileStream(originalFileName, FileMode.Open,
System.IO.FileAccess.Read))
{
using(BinaryReader br = new BinaryReader(fs))
myByteArray = br.ReadBytes((int)numBytes);
}

Would I still do the Close() on both objects? Something like:

No, because FileStream.Dispose and BinaryReader.Dispose are equivalent
to closing them.

 

[Jon Skeet [C# MVP]]

Actually, it does compile.

No, it doesn't.

Which error?

if (fs=null)

should be

if (fs!=null)

Seriously though, especially assuming you've got the try/catch there
anyway, I don't see anything wrong with that particular pattern per
se.
However, I personally would more likely have a complete "using" with
variable declaration around each place I'm actually using a
FileStream. Is it necessary to write it that way? No. But it is
IMHO slightly cleaner looking, and helps avoid code that might
mistakenly use the "fs" variable when it's not currently representing
something useful.

I guess it is cleaner looking.

So you would write it like (without the trys)?:

using(fs = System.IO.File.OpenWrite(imagePath + imageName))
fs.Write(myByteArray, 0, myByteArray.Length);

if (dr["thumbnailImage"].ToString() != "")
{
myByteArray2 =
Convert.FromBase64String(dr["thumbnailImage"].ToString());
if (myByteArray2.Length > 0)
{
thumbnailName = appraisalID.ToString() + "_" +
(imageKtr).ToString() + "_Thumb" + "." + imageExtension;

using(fs = System.IO.File.OpenWrite(imagePath + thumbnailName))
fs.Write(myByteArray2, 0, myByteArray2.Length);
}
}

No, I'd write it as:

using (FileStream fs = File.OpenWrite(imagePath + imageName))
{
fs.Write(myByteArray, 0, myByteArray.Length);
}

....

There's no need to reuse the fs variable - just declare it once per
using statement. Why do you feel the need to use one variable for two
different pieces of code?

(In fact, I'd use File.WriteAllBytes to make the code simpler in the
first place, but...)

 

[tshad]

No, it doesn't.


if (fs=null)

should be

if (fs!=null)

You're right it wouldn't have compiled. Didn't even see that one.

It compiles because the you have is how I actually have it in my code
(fs!=null) . I typed it in just for the example and obviously mistyped it.

Seriously though, especially assuming you've got the try/catch there
anyway, I don't see anything wrong with that particular pattern per
se.
However, I personally would more likely have a complete "using" with
variable declaration around each place I'm actually using a
FileStream. Is it necessary to write it that way? No. But it is
IMHO slightly cleaner looking, and helps avoid code that might
mistakenly use the "fs" variable when it's not currently representing
something useful.

I guess it is cleaner looking.

So you would write it like (without the trys)?:

using(fs = System.IO.File.OpenWrite(imagePath + imageName))
fs.Write(myByteArray, 0, myByteArray.Length);

if (dr["thumbnailImage"].ToString() != "")
{
myByteArray2 =
Convert.FromBase64String(dr["thumbnailImage"].ToString());
if (myByteArray2.Length > 0)
{
thumbnailName = appraisalID.ToString() + "_" +
(imageKtr).ToString() + "_Thumb" + "." + imageExtension;

using(fs = System.IO.File.OpenWrite(imagePath +
thumbnailName))
fs.Write(myByteArray2, 0, myByteArray2.Length);
}
}

No, I'd write it as:

using (FileStream fs = File.OpenWrite(imagePath + imageName))
{
fs.Write(myByteArray, 0, myByteArray.Length);
}

...

There's no need to reuse the fs variable - just declare it once per
using statement. Why do you feel the need to use one variable for two
different pieces of code?

You're right.

I guess I just felt that if I am going to use the FileStream in 2 or 3
different places that it was better to use the same variable instead of the
creation of the object 3 times. Seemed like the overhead (which is neglible
anyway) would make for better code. Probably wrong there

(In fact, I'd use File.WriteAllBytes to make the code simpler in the
first place, but...)

Actually, I would also. I just didn't know it was there. I have always
used fs.Write. Is there a downside to using WriteAllBytes?

Thanks,

Tom

 

[Jon Skeet [C# MVP]]

You're right it wouldn't have compiled. Didn't even see that one.

It compiles because the you have is how I actually have it in my code
(fs!=null) . I typed it in just for the example and obviously mistyped it.

That's why it's always better to cut and paste than retype.

You're right.

I guess I just felt that if I am going to use the FileStream in 2 or 3
different places that it was better to use the same variable instead of the
creation of the object 3 times. Seemed like the overhead (which is neglible
anyway) would make for better code. Probably wrong there

The compiler can deal with the performance overhead if it chooses to.
It's almost certainly negligible, if it's even present. The
*readability* overhead of reusing the same variable *isn't* negligible,
however. You've conceptually got two different variables, so make them
two different variables in reality. Always go for readability over
performance until you've proven that the performance difference is
worth the readability difference

Actually, I would also. I just didn't know it was there. I have always
used fs.Write. Is there a downside to using WriteAllBytes?

It's not easily transferrable if you want to write to something other
than a file, but that's about it as far as I'm aware.

 

[tshad]

That's why it's always better to cut and paste than retype.
True.

The compiler can deal with the performance overhead if it chooses to.
It's almost certainly negligible, if it's even present. The
*readability* overhead of reusing the same variable *isn't* negligible,
however. You've conceptually got two different variables, so make them
two different variables in reality. Always go for readability over
performance until you've proven that the performance difference is
worth the readability difference

Agreed.


It's not easily transferrable if you want to write to something other
than a file, but that's about it as far as I'm aware.

If it's there - I may as well use it.

Thanks,

Tom