Trying to understand exceptions in C#

I do not really use any try/catches in my code ever but I'm trying to break that habit and now get in to using exceptions.

I figure the most important place to have it in my application would be reading a file and I'm trying to implement that now开发者_如何学运维 but I'm unsure of the "best-practices" for doing so. Currently I'm doing something like this:

private void Parse(XDocument xReader)
{
    IEnumerable<XElement> person = xReader.Descendants("Person").Elements();

    foreach (XElement e in person)
        personDic[e.Name.ToString()] = e.Value;

    if (personDic["Name"] == null || personDic["Job"] == null || personDic["HairColor"] == null)
        throw new KeyNotFoundException("Person element not found.");
}

But I am unsure if this is correct. I have this for handling it:

try
{
    personsReader.Read(filename, persons);
}
catch (KeyNotFoundException e)
{
    MessageBox.Show(e.Message);

    return;
}

// Do stuff after reading in the file..

However when showing e.Message it just shows the generic KeyNotFoundException error message and not by custom error message. Also I'm not sure if in general I am going about this whole "exception handling stuff" properly. I do return in the catch because if the file is not read successfully obviously I just want to pretend like the user never tried to open a file and let him try again with another file.

Am I doing this properly? Again I am fairly new to using exceptions and I want to make it sure I got it down right before continuing on and applying this to the rest of my program.

Also, why do people say not to do catch (Exception e)? It seems like in this case I would want to do that because regardless of what error occurs when reading in a file, if there is an error, I want to stop reading the file, display the error message, and return. Wouldn't that always be the case? I can understand not wanting to handle Exception e if you would want to handle something differently based on the exception but in this case wouldn't I want to just handle the base exception class in case anything goes wrong?

---

You should catch exceptions when you can handle the condition and do something useful. Otherwise you should let it bubble up the call stack and perhaps someone above you can handle it. Some apps have unhandled exception handlers to handle it at the outer most layer but in general, unless you know you have some useful way to handle it, let it go.

In your case, you're handling not being able to read a resource and informing the user. You're handling it. Concerning the generic exception, one thing you can do is catch and re-throw a better exception. If you do that, make sure you incude the root cause exception as the inner exception. You can also trace or log the details if appropriate.

throw new MyGoodExceptionType ("Could not read file", e);  // e is caught inner root cause.

Now the UI shows a good error and perhaps the inner root cause is in a log etc...

Some typical mistakes:

• Handling exceptions deep in the stack in a generic library method: Remember that a common library function may get called in many different code paths. You likely don't have the context whether it should be handled and whether it's appropriate to handle it. the caller higher in the stack likely has context and knows whether it's safe to handle. Typically that means higher layers of code decide to handle. In lower layers, typically you let them flow.

• Swallowing Exception: Some code catches exceptions (especially lower in the stack) and then the root condition just evaporates making it maddening to debug. Once agan, if you can handle it, do so. If not, let it go.

• Exceptions should be exceptional: Don't use excpetions for flow control. For example, if you're reading a resource, don't try and read and then catch the exception and make a decision point. Instead, call ifexists, check bool and make decisions in your code. this especially helps when you set the debugger to break on exceptions. You should be able to run clean and if the debugger breaks, it should be a real issue. Having the debugger break constantly when debugging is problematic. I personally like throwing exceptions very rarely and always try to avoid for flow control.

Hope that helps.

---

Ok, first...

... This iss not KeynotFoundException, it should be ArgumentException.... the Argument provided is not valid.

The documentation clearly states:

The exception that is thrown when the key specified for accessing an element in a collection does not match any key in the collection.

Compare that with:

The exception that is thrown when one of the arguments provided to a method is not valid

Now:

Also, why do people say not to do catch (Exception e)?

Becasue this swallows the exception and makes it impossible to have central error handling / logging in place. ONLY handle exception you expect, UNLESS it is a catch / close something or log it / rethrow (i.e. throw;). Then have a central appdomain handler that gets every uncaptured exceptin and logs it ;) It can not handle anything - because exceptions at that level are unexpected,. It should basically write the excption to a file and be done, possibly with a UI (application has t obe restartet).

---

As far as what you're doing, it looks mostly ok. I can't speak to whether or not you should be throwing exceptions at that particular point, but the throwing and catching is correctly done. As far as the message, it should be working the way it stands. Try displaying e.ToString() to see the call stack. It could be that simply doing person["Name"] is throwing the KeyNotFoundException first.

As to the question of catching Exception, it's not always bad. Sometimes you cannot predict all possible exceptions, and it's sometimes a good thing to handle any possible failure. However, it gives you no way to handle particular exceptions differently.

As an example, if you get KeyNotFoundException, you might mention something about how the file is incorrectly formatted, and maybe display the file to the user on the screen. If you get FileNotfoundException, you could show them the path and open up an OpenFileDialog to have them select a new file. Exceptions due to security permissions you could display instructions to have them elevate your permissions. Some exceptions may even be recoverable (perhaps one element is badly formatted, but the rest are ok; should it fail the whole thing?)

But, it's ok to catch everything if that's how you want to design it. The most solid program out there would catch every possible exception and handle it in very specific ways, instead of presenting the raw exception to the user. It makes for a better user experience, and gives you ways to work around the problems that can happen.

---

Most of the time you may not care about the type of exception you get so catching the generic Exception is fine, however there are specific situations in which you would actually want to catch the relevant exception (not just the generic Exception).

One particular example is if you have a thread and you want to interrupt it from a blocking call, in that case you have to distinguish between the InterruptException and the Exception.

Consider this example: you have a thread which runs the Read every minute for 5 minutes (it's not a very realistic example, but it should give you an idea of why you want to handle different exceptions). You have to stop the thread after 5 minutes because your application is going to shut down and you don't want to wait another minute for the running flag to be read... after all, you don't want your user to be waiting for an entire minute just to shut down the application. In order to stop the thread right away, you set the flag to false and you call Interrupt on your thread. In this case you specifically have to catch the ThreadInterrupted exception, because it tells you that you should exit the loop. If you catch another exception, then you've failed to perform the task, but you don't want to give up on the job all together and you would like to try and read again the next minute. This depicts how your requirements dictate the type of exceptions you need to handle. Here is the example in code:

bool running = true;

Thread t = new Thread(()=>
{
    while(running)
    {
        try
        {
            // Block for 1 minute
            Thread.Sleep(60*1000); 

            // Perform one read per minute
            personsReader.Read(filename, persons);
        }
        catch (KeyNotFoundException e)
        {
            // Perform a specific exception handling when the key is not found
            // but do not exit the thread since this is not a fatal exception
            MessageBox.Show(e.Message);
        }
        catch(InterruptException)
        {
            // Eat the interrupt exception and exit the thread, because the user
            // has signalled that the thread should be interrupted.
            return;
        }
        catch(Exception e)
        {
            // Perform a genetic exception handling when another exception occurs
            // but do not exit the thread since this is not a fatal error.
            MessageBox.Show("A generic message exception: " + e.Message);
        }
    }
});

t.IsBackground = true;
t.Start();

// Let the thread run for 5 minutes
Thread.Sleep(60*5000);

running = false;
// Interrupt the thread
t.Interrupt();

// Wait for the thread to exit
t.Join();

Now on to your other problem with the exception not showing up: note that you're accessing person[e.Name.ToString()] = e.Value which requires a key lookup and if the key is not in the map, then you may get a KeyNotFoundException. That would be the generic exception that you're catching and your custom exception will never be thrown because person[e.Name.ToString()] may throw before you even get to your code.

foreach (XElement e in person)
    person[e.Name.ToString()] = e.Value; // <-- May be throwing the KeyNotFoundException

if (person["Name"] == null || person["Job"] == null || person["HairColor"] == null)
    throw new KeyNotFoundException("Person element not found.");

Furthermore, you don't want to throw a KeyNotFoundException when you've actually found the key but you didn't find a corresponding value: if person["Name"] == null evaluates to true, then the key "Name" was actually found in the person dictionary, so throwing the KeyNotFoundException would be misleading to anybody who catches that exception. In the case that your value is null, then it would probably not be advisable to throw an exception anyway... it really isn't an exceptional case. You could return a flag indicating that the key was not found:

public bool PerformRead(/*... parameters ...*/)
{
    foreach (XElement e in person)
    {
        // Avoid getting the KeyNotFoundException
        if(!person.ContainsKey(e.Name.ToString()))
        {
            person.Add(e.Name.ToString(), "some default value");
        }
        person[e.Name.ToString()] = e.Value;
    }

    if (person["Name"] == null || person["Job"] == null || person["HairColor"] == null)
    {
        return false;
    }
    else
    {
        return true;
    }
}

---

I'm not quite sure why you're not getting your custom error message. That should be happening (unless it's something else throwing a KeyNotFoundException, not the one that you're explicitly throwing).

Also, generally you should put all the code that relies on the file read being successful inside the try, which is often the rest of the body of your method. You no longer would need a return inside your catch block, and subsequent code that doesn't rely on the file read being successful could still execute after a failure.

Example:

public static void Main()
{
    var filename = "whatever";

    try
    {
        personsReader.Read(filename, persons);
        var result = personsReader.DoSomethingAfterReading();
        result.DoSomethingElse();
    }
    catch (KeyNotFoundException e)
    {
        MessageBox.Show(e.Message);
    }
    finally
    {
        personsReader.CloseIfYouNeedTo();
    }

    DoSomeUnrelatedCodeHere();
}

And the reason it's good practice not to catch any old Exception e is because you only want to catch and handle the exceptions you're expecting to get. If you get a different kind of exception that you weren't expecting to get, typically this means that something novel failed in a way you didn't anticipate, and you want this behavior to be noticeable, not just get swept under the rug with all the regular error-handling code.

A lot of production-level systems will have one big try/catch around the entire program that catches any exception and performs logging and cleanup before crashing gracefully. This is complemented by having specific try/catch blocks deeper inside the code that handle expected exceptions in a well-defined manner. For unexpected exceptions, you could always just let the CLR bomb ungracefully and figure out what happened from that.

Here's an example of a novel exception. What if something goes terribly wrong and in this line:

IEnumerable<XElement> person = xReader.Descendants("Person").Elements();

...you get an OutOfMemoryException? Should you really just display a popup to the user and allow your program to try to carry on like normal, even though there's simply no way it will be able to? And what if, because you failed silently on an OutOfMemoryException, you later attempt to dereference a null reference, and get a NullReferenceException that causes your program to crash? You'll have a devil of a time trying to track down the root cause of why that reference was null.

Best way to suss out a bug is to fail fast and fail noisily.

---

"Exceptions are for Exceptional circumstances" - unknown

Don't use the to essentially pass a message out of a method to the calling method. Always try to gracefully handle things. When something really odd happens then throw an exception. This is something that dev not real familiar with how to use exceptions do a lot.

---

In your code you are triggering the xxx when you evaluate the condition inside the if statement.

Asking person["Name"] == null || person["Job"] == null || person["HairColor"] == null will fail if any of those keys are not in your dictionary.

You need to do this instead:

if (!person.ContainsKey("Name"] ||
    !person.ContainsKey("Job"] ||
    !person.ContainsKey("HairColor"))

So, your call to throw the exception is never executed! And that's why you never see your message.

I would keep your habit of not doing exceptions for this kind of coding. Exceptions are expensive and can cause real issues in your code to be hidden.

Don't catch general exceptions and don't create exceptions for non-exceptional circumstances.