Proper disposal of filestreams and binary streams

Question

As it is, I tried error-proofing my code and ended up making it look quite messy.

I have a function set up to read a certain type of file. I want the function to return false if there was a problem, or true if everything worked. I'm having trouble figuring out how to structure everything.

I have an initial try-catch block that tries to open a file stream. After that though, I have certain other checks I make during the reading process such as file size and values at certain offsets. The way I set it up was with if else statements. Such as:

if(condition){

}
else{
    MessageBox.Show("There was an error");
    br.Dispose();
    fs.Dispose();
    return false;
}

...br being the binary reader and fs the filestream. There are many blocks like this, and it seems like bad practice to write the same thing so many times. The first thing that comes to mind is to wrap the entire thing in a try-catch statement and throw exceptions instead of using the if else blocks. I remember when reading about try-catch statements that it's good to have them, but not to wrap everything with them. To be honest, I still don't completely understand why it would be bad practice to wrap everything in try catch statements, as they only have an effect when there's an error, in which case the program is going south anyway...

Also, do I have to close the binary reader and file stream, or will closing one close the other? Is there any way to use them without having to dispose of them?

Answer 1

How about making use of the using keyword? this wraps your use of an IDisposable in a try - finally block;

bool success = true;

using(var fs = new FileStream(fileName, FileMode.Create)))
using(var br = new BinaryReader(fs))
{
  // do something
  success = result;
}

return success;

The nested using blocks will ensure that both the filestream and binary reader are always properly closed and disposed.

You can read more about using in MSDN. It makes the use of IDisposable a little neater, removing the need for explicit excpetion handling.

Regarding your statement:

I remember when reading about try-catch statements that it's good to have them, but not to wrap everything with them.

I always use the simple rule that if I cannot handle and recover from an exception within a particular block of code, don't try to catch it. Allow the exception to 'bubble up' the stack to a point where it makes more sense to catch it. With this approach you will find that you do not need to add many try-catch blocks, you will tend to use them at the point of integration with services (such as filesystem, network etc ...) but your business logic is almost always free of exception handling mechanisms.

Answer 2

I think the best way to make sure filestreams are disposed is to wrap their usage with the following using block

using (FileStream)
{
    ....
}

Answer 3

Use the using keyword. With using you can rewrite something like this:

public static int CountCars()
{
    SqlConnection conn = new SqlConnection(connectionString);
    try
    {
      SqlCommand cmd = conn.CreateCommand();
      conn.Open();
      try
      {
        cmd.CommandText = "SELECT COUNT(1) FROM Carsd";

        return (int)cmd.ExecuteScalar();
      }
      finally
      {
        if(cmd != null)
          cmd.Dispose();
      }
    }
    finally
    {
      if(cmd != null)
        conn.Dispose();
    }
}

into this:

public static int CountCars()
{
    using(SqlConnection conn = new SqlConnection(connectionString))
    using(SqlCommand cmd = conn.CreateCommand())
    {
        conn.Open();
        cmd.CommandText = "SELECT COUNT(1) FROM Carsd";

        return (int)cmd.ExecuteScalar();
    }

}

Both code snippets will produce exactly the same IL code when compiled. The examples are from http://coding.abel.nu/2011/12/idisposable-and-using-in-c/ where I wrote some more details about using and IDisposable.

Answer 4

Just use the using keyword for your disposable objects. Within the block of the using keyword you can throw exceptions or return without having to worry about disposal; it will happen automagically for you.

try-catch blocks are not a very good idea only because there exists a much better alternative: the try-finally blocks. But the using keyword is even better because it essentially expands into a try-finally block and it takes care of object disposal.

Closing the file stream will also close the binary reader, and so will disposing of them do. Why do you want to use them without disposing them? Disposing them is better, and disposing them via using is the best thing to do.

Answer 5

Yes, it is bad practice.

Instead of returning booleans that indicate whether a problem occurred or not, you should throw exceptions. Example:

if (headNotValid)
   throw new Exception("Header was not valid");

In some cases it may be advisable to make a new exception class.

When working with classes that inherit from IDisposable you should use the using directive.

using (var stream = new FileStream(filename))
{
}

That guarantees, that your stream is disposed, even if a exception is thrown within the using block.

In summary, I would prefer something like this:

private void string ParseFile(string filename)
{
     using (var stream = new FileStream(filename))
     {
           if (somethingNotValid)
              throw new Exception(...);

           return ...;
     }
}

And in your main:

{
     try
     {
          var value = ParseFile(filename);
     }
     catch (Exception)
     {
          Console.WriteLine(..);
     }
}