How to efficiently manage a Stream with try / catch / finally C#

Question

I recently discussed with a coworker who told me that I was managing incorrently a stream into a try / catch / block. So I wanna know what would be a good approach for you.

try
{
    StreamReader sr = new StreamReader("TestFile.txt");
    //After that, here an operation of about 30 seconds to fulfill;
}
catch (IOException ioex)
{
    throw new IOException("An error occurred while processing the file.", ioex);
}
catch (Exception ex)
{
    throw new Exception("An generic error ocurred.");
}
finally
{
    if(sr != null){
        stream.Close();
        stream = null;
    }
}

He stated that having 2 Exception are unnecessary, even using the IOException. We can use only Exception. But the only thing that I want is to recognize where exactly the exception has been produced, because after opening the file, an operation of about 30 seconds will be performed.

So what would you think? We saw this MS example (http://msdn.microsoft.com/fr-Fr/library/system.io.streamreader.aspx) which it's simplier but in terms of performance or clean code, you find something strange?

Your opinions please!

-EDIT-----------------

Ok, I see the point but we were discussing about the Catch IOException and just using the Exception. In my opinion, like as in the example above you can know where the error ocurred; at the moment of managing the file or in the process after opening the file. That's my first question. And now, what do you think about this change below.

try
{
    using(StreamReader sr = new StreamReader("TestFile.txt"))
    {
        //After that, here an operation of about 30 seconds to fulfill;
    }
}
catch (Exception ex)
{
    throw new Exception("An generic error ocurred.");
}
finally
{
    if(sr != null){
        stream.Close();
        stream = null;
    }
}

-------------------EDIT 2------------------------

Finally, I hope this would be my final solution. Thank you so much for your answers. So using is faster, efficient and just one exception is necessary.

try
{
    using (StreamReader stream = sr = new StreamReader("TestFile.txt"))
    {
        //Operation
    }
}
catch (Exception e)
{
    throw new Exception(String.Format("An error ocurred while executing the data import: {0}", e.Message), e);
}

Any other comment would be appreciated!

Solution 1

you can use using block as below, and it will dispose the stream even on exception occurred

using (StreamReader sr = new StreamReader("TestFile.txt"))
{
   // do something with sr
}

Catch Exception if you're going to do something about. If you can't fix the problem, there's no point in catching it.

if you can't resolve the exception, it's best to just let the exception bubble up the exception and catch it there.

try
{
    using(StreamReader sr = new StreamReader("TestFile.txt"))
    {
       // your code 
    }
}
catch (IOException ioex)
{
    // do something to fix the problem 
    // log the exception 
}

Solution 2

Don't catch an exception only to throw the same exception immediately, only now with less information and missing the stack frame of where the exception actually occurred.

If I came across something like

catch (Exception ex)
{
   throw new Exception("An generic error ocurred.");
}

in a code review I would fail that review (not just for the grammar and spelling mistake either ;-)

At the very least, you should throw it with the original exception as an inner exception.

catch (Exception ex)
{
    throw new Exception("A generic error occurred.", ex)
}

But to be perfectly frank, in this sample code it adds nothing whatsoever, it would be better removed entirely imo.

Solution 3

If you're rethrowing the exception instead of catching it, why bother creating a new exception? You're throwing away valuable information. There's literally no point in catching an exception only to throw it again. Nor is there any point replacing a helpful exception (as in, it's got all the diagnostic information anyone upstream might need) and replacing it with a generic exception.

For your situation, I'd simply:

using(var sr=new StreamReader())
{
    //some code
}

all your other improvements are quite the opposite.

Comments

[Maximus Decimus]

Thanks for your answer. See my EDIT section please.

[Scott Chamberlain]

@MaximusDecimus Your edit is wrong, you don't need to use the finally block as the using statement does that job for you.

[Maximus Decimus]

Ok. I'm gonna consider that. What about the exceptions. It would be necessary to manage 2 catch block or just 1 would be it's enough (Exception).

[Damith]

@KirillPolishchuk can you explain?

[ta.speot.is]

You probably want throw new Exception("A generic ...", ex).

[Rodrick Chapman]

@Darmith - Doesn't using desugar into a try...finally? In that case, your two catch clauses won't ever be reached by an exception thrown in the using block.

[ta.speot.is]

@RodrickChapman Yes, it desugars into try...finally. There is no catch.

[ta.speot.is]

You're throwing away valuable information Exceptionally good pun.

[Scott Chamberlain]

@RodrickChapman a try...finally does not affect the exception propagating to the parent try...catch, see for yourself

[spender]

I just cannot see the point in simply rethrowing a new exception without performing any kind of other operation in the catch statement. It's pointless.