Confused about error handling

[Gustaf]

I'm confused about structured error handling. The following piece of
code is a simplification of a class library I'm working on. It works,
and it does what I want, but I'm still not convinced that I have been
doing it right. I think I overdo it. Please have a look:

--

using System;
using System.IO;

namespace ErrorHandlingTest
{
class ErrorHandlingTest
{
[STAThread]
static void Main(string[] args)
{
try
{
Wrapper w = new Wrapper(args[0], args[1]);
}
catch (System.Exception e)
{
Console.WriteLine(e.Message);
}
}
}

class Wrapper
{
public Wrapper(string file1, string file2)
{
try
{
ReaderType1 reader1 = new ReaderType1(file1);
ReaderType2 reader2 = new ReaderType2(file2);
}
catch (System.Exception e)
{
throw new System.Exception(e.Message);
}
}
}

class ReaderType1
{
public ReaderType1(string file)
{
try
{
StreamReader sr = new StreamReader(file);
}
catch (System.IO.FileNotFoundException e1)
{
throw new System.IO.FileNotFoundException(e1.Message);
}
catch (System.IO.DirectoryNotFoundException e2)
{
throw new System.IO.DirectoryNotFoundException(e2.Message);
}
}
}

class ReaderType2
{
public ReaderType2(string file)
{
try
{
StreamReader sr = new StreamReader(file);
}
catch (System.IO.FileNotFoundException e1)
{
throw new System.IO.FileNotFoundException(e1.Message);
}
catch (System.IO.DirectoryNotFoundException e2)
{
throw new System.IO.DirectoryNotFoundException(e2.Message);
}
}
}
}

--

The part that makes up the class library is the Wrapper and reader
classes. These classes have no direct contact with the user interface,
but there are plenty of things that can go wrong inside them.

After writing and running this, I found that I could remove all error
handling except in Main() and the program ran just as fine (it could
still separate between not finding the folder and not finding the file).
It makes me wonder why I have to write several catch blocks, when all I
need is to catch System.Exception when I use my class library (like in
Main() above).

I want to write robust code, but I fail to see how the code gets *more*
robust by adding error handling inside the class library. It appears as
if classes that shouldn't talk with the user doesn't need error
handling! There must be something I'm missing here.

Gustaf

[Tom Porterfield]

Gustaf wrote:

I'm confused about structured error handling. The following piece of
code is a simplification of a class library I'm working on. It works,
and it does what I want, but I'm still not convinced that I have been
doing it right. I think I overdo it. Please have a look:

--

using System;
using System.IO;

namespace ErrorHandlingTest
{
class ErrorHandlingTest
{
[STAThread]
static void Main(string[] args)
{
try
{
Wrapper w = new Wrapper(args[0], args[1]);
}
catch (System.Exception e)
{
Console.WriteLine(e.Message);
}
}
}

class Wrapper
{
public Wrapper(string file1, string file2)
{
try
{
ReaderType1 reader1 = new ReaderType1(file1);
ReaderType2 reader2 = new ReaderType2(file2);
}
catch (System.Exception e)
{
throw new System.Exception(e.Message);
}
}
}

class ReaderType1
{
public ReaderType1(string file)
{
try
{
StreamReader sr = new StreamReader(file);
}
catch (System.IO.FileNotFoundException e1)
{
throw new System.IO.FileNotFoundException(e1.Message);
}
catch (System.IO.DirectoryNotFoundException e2)
{
throw new System.IO.DirectoryNotFoundException(e2.Message);
}
}
}

class ReaderType2
{
public ReaderType2(string file)
{
try
{
StreamReader sr = new StreamReader(file);
}
catch (System.IO.FileNotFoundException e1)
{
throw new System.IO.FileNotFoundException(e1.Message);
}
catch (System.IO.DirectoryNotFoundException e2)
{
throw new System.IO.DirectoryNotFoundException(e2.Message);
}
}
}
}

--

The part that makes up the class library is the Wrapper and reader
classes. These classes have no direct contact with the user interface,
but there are plenty of things that can go wrong inside them.

After writing and running this, I found that I could remove all error
handling except in Main() and the program ran just as fine (it could
still separate between not finding the folder and not finding the file).
It makes me wonder why I have to write several catch blocks, when all I
need is to catch System.Exception when I use my class library (like in
Main() above).

I want to write robust code, but I fail to see how the code gets *more*
robust by adding error handling inside the class library. It appears as
if classes that shouldn't talk with the user doesn't need error
handling! There must be something I'm missing here.

Gustaf

Your error handling in Wrapper, ReaderType1 and ReaderType2 makes no
sense. You are not doing anything when you catch the exception, you are
not adding any additional information to the exception, you are simply
wrapping the caught exception in another exception of the same type and
throwing that new exception. I would remove all of the exception
handling in those classes in this example as they provide nothing to the
overall value or robustness of the code.

My basic principle on exception handling is only catch exceptions when
you can truly do something intelligent about the exception (note that
these situations are rare in typical programs). Other than that, only
catch exceptions and rethrow exceptions if you plan on adding truly
useful information to the exception that is thrown that will help you
resolve the issue that is causing the exception to be thrown.
--
Tom Porterfield

[Bruce Wood]

No, you're on the right track.

The code _doesn't_ get "more robust" as you add more error handling. It
gets "more robust" only when there is a specific exception that you
know what to do with it at a certain point in the code.

In your ReaderType2 and ReaderType1 classes, there's no need to catch
the exceptions you're catching, unless those classes want to:

1. Take some remedial action as a result of the exception.
2. Transform the exception into a different kind of exception because
the new exception would make sense to the caller (context).
3. Add more information to an exception.
4. Don't really care about the exception and can continue anyway
(swallow the exception).
5. Want to defer the exception to a later moment in time.

Other than that, you want to catch all other exceptions at the top
level, but not using the mechanism you're using in Main(). Because of
the structure of Windows applications, a top-level try...catch doesn't
always work.

Look at the AppDomain.UnhandledException and
Application.ThreadException events for how to build global exception
handlers for logging, etc.

[James Curran]

You have overdo it. In fact, you've made things worse!

public Wrapper(string file1, string file2)
{
try
{
ReaderType1 reader1 = new ReaderType1(file1);
ReaderType2 reader2 = new ReaderType2(file2);
}
catch (System.Exception e)
{
throw new System.Exception(e.Message);
}
}
}

All this is accomplishing is destroying information that was in the original
Exception (like the line number of the throw that originally cause it).
Had you written it as:
public Wrapper(string file1, string file2)
{
try
{
ReaderType1 reader1 = new ReaderType1(file1);
ReaderType2 reader2 = new ReaderType2(file2);
}
catch (System.Exception e)
{
throw;
}
}
}

It would be preserved the original information, making this version merely
pointless, but no harmful.

Basically, you ONLY put a try/catch at the level where you can handle the
error. Exceptions will automatically "bubble up" until it finds a catch.
Since the only thing you ever do with an exception is print the message and
exist your app, you can do that at the top level.

--
Truth,
James Curran
[erstwhile VC++ MVP]

Home: www.noveltheory.com Work: www.njtheater.com
Blog: www.honestillusion.com Day Job: www.partsearch.com
"Gustaf" <gu*****@algonet.se> wrote in message
news:uj*************@TK2MSFTNGP15.phx.gbl...

I'm confused about structured error handling. The following piece of
code is a simplification of a class library I'm working on. It works,
and it does what I want, but I'm still not convinced that I have been
doing it right. I think I overdo it. Please have a look:

--

using System;
using System.IO;

namespace ErrorHandlingTest
{
class ErrorHandlingTest
{
[STAThread]
static void Main(string[] args)
{
try
{
Wrapper w = new Wrapper(args[0], args[1]);
}
catch (System.Exception e)
{
Console.WriteLine(e.Message);
}
}
}

class Wrapper
{
public Wrapper(string file1, string file2)
{
try
{
ReaderType1 reader1 = new ReaderType1(file1);
ReaderType2 reader2 = new ReaderType2(file2);
}
catch (System.Exception e)
{
throw new System.Exception(e.Message);
}
}
}

class ReaderType1
{
public ReaderType1(string file)
{
try
{
StreamReader sr = new StreamReader(file);
}
catch (System.IO.FileNotFoundException e1)
{
throw new System.IO.FileNotFoundException(e1.Message);
}
catch (System.IO.DirectoryNotFoundException e2)
{
throw new System.IO.DirectoryNotFoundException(e2.Message); }
}
}

class ReaderType2
{
public ReaderType2(string file)
{
try
{
StreamReader sr = new StreamReader(file);
}
catch (System.IO.FileNotFoundException e1)
{
throw new System.IO.FileNotFoundException(e1.Message);
}
catch (System.IO.DirectoryNotFoundException e2)
{
throw new System.IO.DirectoryNotFoundException(e2.Message); }
}
}
}

--

The part that makes up the class library is the Wrapper and reader
classes. These classes have no direct contact with the user interface,
but there are plenty of things that can go wrong inside them.

After writing and running this, I found that I could remove all error
handling except in Main() and the program ran just as fine (it could
still separate between not finding the folder and not finding the file).
It makes me wonder why I have to write several catch blocks, when all I
need is to catch System.Exception when I use my class library (like in
Main() above).

I want to write robust code, but I fail to see how the code gets *more*
robust by adding error handling inside the class library. It appears as
if classes that shouldn't talk with the user doesn't need error
handling! There must be something I'm missing here.

Gustaf

[Peter Bromberg [C# MVP]]

Besides the good stuff that Tom et al added, in general,
you should only throw exceptions when a condition outside of your code's
assumptions occurs.

In other words, don't use exceptions as a way to provide your intended
functionality even if something bad happens.

An exception should be generated if a real, legitimate unexpected condition
occurs, such as an unavailable database.

You can design your classes so that an exception is not thrown in normal use.

For example, the FileStream class exposes a way of determining whether the
end of the file has been reached.
This avoids the exception that is thrown if you read past the end of the
file.

The following short example shows how to read to the end of the file.

class FileRead {
void Open() {
FileStream stream = File.Open("myfile.txt", FileMode.Open);
byte b;

// ReadByte returns -1 at EOF.
while ((b == stream.ReadByte()) != true) {
// Do something here. No Exceptions will be thrown.
}
}
}

As Tom mentioned, its important not only to code defensively with exceptions
that really serve a purpose, but also to decide what the caller will do when
an exception is propagated up the stack.
Peter

--
Co-founder, Eggheadcafe.com developer portal:
http://www.eggheadcafe.com
UnBlog:
http://petesbloggerama.blogspot.com


"Gustaf" wrote:

I'm confused about structured error handling. The following piece of
code is a simplification of a class library I'm working on. It works,
and it does what I want, but I'm still not convinced that I have been
doing it right. I think I overdo it. Please have a look:

--

using System;
using System.IO;

namespace ErrorHandlingTest
{
class ErrorHandlingTest
{
[STAThread]
static void Main(string[] args)
{
try
{
Wrapper w = new Wrapper(args[0], args[1]);
}
catch (System.Exception e)
{
Console.WriteLine(e.Message);
}
}
}

class Wrapper
{
public Wrapper(string file1, string file2)
{
try
{
ReaderType1 reader1 = new ReaderType1(file1);
ReaderType2 reader2 = new ReaderType2(file2);
}
catch (System.Exception e)
{
throw new System.Exception(e.Message);
}
}
}

class ReaderType1
{
public ReaderType1(string file)
{
try
{
StreamReader sr = new StreamReader(file);
}
catch (System.IO.FileNotFoundException e1)
{
throw new System.IO.FileNotFoundException(e1.Message);
}
catch (System.IO.DirectoryNotFoundException e2)
{
throw new System.IO.DirectoryNotFoundException(e2.Message);
}
}
}

class ReaderType2
{
public ReaderType2(string file)
{
try
{
StreamReader sr = new StreamReader(file);
}
catch (System.IO.FileNotFoundException e1)
{
throw new System.IO.FileNotFoundException(e1.Message);
}
catch (System.IO.DirectoryNotFoundException e2)
{
throw new System.IO.DirectoryNotFoundException(e2.Message);
}
}
}
}

--

The part that makes up the class library is the Wrapper and reader
classes. These classes have no direct contact with the user interface,
but there are plenty of things that can go wrong inside them.

After writing and running this, I found that I could remove all error
handling except in Main() and the program ran just as fine (it could
still separate between not finding the folder and not finding the file).
It makes me wonder why I have to write several catch blocks, when all I
need is to catch System.Exception when I use my class library (like in
Main() above).

I want to write robust code, but I fail to see how the code gets *more*
robust by adding error handling inside the class library. It appears as
if classes that shouldn't talk with the user doesn't need error
handling! There must be something I'm missing here.

Gustaf

[Bruce Wood]

Good advice, but if I may make one picky correction, don't write code
like this:

while ((b == stream.ReadByte()) != true)

you should instead write this:

while (b != stream.ReadByte())

[Jeff Louie]

Gustof...Looking at your code you seem to be concerned about exceptions
in the constructor, a special situation. Since a constructor does not
return a value, one way to signal a failure of construction is to throw
an exception. So you can let the exception bubble up or you can create
your own custom construction exception and throw that.

The alternative in a nothrow framework would be to catch all exceptions
and if fatal, set a flag in the object noting the object isValid ==
false. This requires methods to check the flag and do innocuous things
if the object state is invalid.

If you are allocating any unmanaged resources in the constructor, your
constructor code should be "transactional," the code should complete to
success or release any unmanaged resources in try catch and then throw.

Regards,
Jeff

*** Sent via Developersdex http://www.developersdex.com ***

[Bruce Wood]

Oh, rats. I wasn't paying attention. My correction should have read:

while ((b == stream.ReadByte()) != true)

you should instead write this:

while ((b = stream.ReadByte() >= 0)

Or something like that, no? In any event, the original code didn't make
sense.

[Jon Skeet [C# MVP]]

Bruce Wood <br*******@canada.com> wrote:

Good advice, but if I may make one picky correction, don't write code
like this:

while ((b == stream.ReadByte()) != true)

you should instead write this:

while (b != stream.ReadByte())

In fact, you should almost always actually read a buffer at a time -
and when the call to Read returns 0, that means you've read to the end
of the stream. The differences in efficiency can be really important
there (and you probably know I'm very much against premature
optimisation normally).

--
Jon Skeet - <sk***@pobox.com>
http://www.pobox.com/~skeet Blog: http://www.msmvps.com/jon.skeet
If replying to the group, please do not mail me too

[Gustaf]

Many thanks for all the help here. I was learning a lot reading your
answers.

Gustaf