views:

141

answers:

9
+2  Q: 

Eating Exceptions

I m parsing a file that has MalFormed data from time to time.

and it s throwing an exception,

i d like to recover from the exception and ignore the bad formatted data.

What s the best way to do this?

try{
// parse file
}catch(Exception){
//eat it.
}

*EDIT:*I think, my question wasnt understood well. i d like to recover from the exception, any exception for that matter, i dont want my program to stop. but to continue.

A: 
try{
   // parse file
   }
   catch(Exception){
     //eat it.
   }
   finally 
   {
      //cleanup  must ... release system resources here (e.g. stream.Close() etc)
      // code here always executes...even if the exception is unhandled

   }

// code will resume execution from this point onwards...
mumtaz
A: 

That works - maybe you could log the errors though too in the event you wanted to see what the data was that caused the exception to happen and use that to improve your parsing logic.

cpound
+3  A: 

In short, you probably should not use exceptions for this. Something like a TryParse which returns false is much more efficient and easier to recover from. Exception handling is a bit of a blunt object approach.

MSDN Guidance on Exception Handling

hemp
TryParse is only relevant if you are reading some primitive types that provide TryParse methods. "TryParse" without exceptions isn't possible if you are parsing from a file stream and that file is missing - you will need exception handling *somewhere* to handle the FileNotFoundException.
Jason Williams
@Jason, I think you're picking nits. A TryParse method is available anytime you write one. If you've never written one, you're probably doing it wrong. MSDN calls it the ["Tester-Doer" pattern](http://msdn.microsoft.com/en-us/library/ms229009.aspx) and encourages it as a preferred practice (over handling exceptions). Don't open a file to parse it; instead, first test to see if it exists, then try to open it for reading, then try to parse the open stream. If you follow that pattern, no FileNotFoundException is needed, unless you decide to throw one.
hemp
@hemp: If you test if a file exists, then try to open it for reading, you will get a FileNotFound exception when another thread or another process deletes the file between your IsExists() call and actually opening the file. No matter how rare this is, your code is flawed unless you deal with the possible exception. The problem is that you *can't* always avoid exception handling (much as I'd love to). Yes, you can write a TryParse method, but to write it correctly, you are almost certainly going to have to include some exception handling inside it - you're just moving the site of the catch{}.
Jason Williams
@Jason, still nitpicking. You are correct that the tester-doer pattern has concurrency issues around shared state. Of course, that's also true of everything. Do you wrap every line of code in a try/catch looking for OutOfMemoryException? No, you catch it when (and only when) there's something you can *do* about it. Generally, exceptions should only be thrown when the state is **exceptional** and should only be handled when/where doing so adds value. Tester-doer doesn't just move the site of the catch, it also exponentially decreases the likelihood of hitting the exception.
hemp
@hemp: Precisely. So if you're writing a TryParse(), which is by definition a method you do not expect to throw exceptions, should you ignore *all* exceptions and let them hit your caller? I agree with most of your ideals, but unfortunately a lot of .net methods throw exceptions to pass back what I would describe as "status information" (FileNotFound being a great example), so unfortunately, to write robust code, exception handling is usually unavoidable. I don't agree with it, but if you call library functions it's not my choice where/how exceptions are thrown.
Jason Williams
@hemp: also, reducing the chance of an exception does not release you from the responsibility of handling it. "Reducing the chances" just make the bug more devastating and harder to find *when* it occurs.
Jason Williams
@Jason, and being forced to handle exceptions do to using poorly designed libraries does not excuse handling them when it isn't necessary. I suspect we both agree more than we disagree, but in my experience exceptions are heavily abused in C#/VB.NET. If you *really* want to do things *right*, then we should be talking about [Exception Guarantees](http://en.wikipedia.org/wiki/Abrahams_guarantees), but proper library design wasn't the caller's question.
hemp
@hemp: Yep, we're both on the same side of the fence all right. (I never said you should handle exceptions *unnecessarily*, btw).
Jason Williams
A: 

I would not catch Exception, I would catch the more specific exception type that gets thrown

Steve Ellinger
+1  A: 

There is some good background info here: http://stackoverflow.com/questions/204814/is-there-any-valid-reason-to-ever-ignore-a-caught-exception

Short answer: exceptions are expensive to use in this manner. If at all possible, test the input before sending it for processing and do your ignoring there rather than ignoring exceptions.

Also ensure you aren't casting too wide a net and eating legitimate exceptions you might want to know about.

Jason
+1  A: 

In general, something like this:

try
{
    // parse file
}
catch (FormatException)
{
    // handle the exception
}
finally
{
    // this block is always executed
}

You should avoid catching the general Exception case and instead catch a specific exception, whatever that may be.

Tyler
+1  A: 

Catching a general exception is a bad idea if you expect it to only throw one or two specific exception types for parsing-specific errors. It is usually better to catch each specific exception type and handle it independently so that your code won't suppress any other (perhaps unexpected) error (e.g. if the file is missing, or a network connection times out, should it be handled in the same way as if the file contains corrupt data?)

However, catching a general exception is a great idea if you deliberately need/want to catch all possible errors and gracefully continue. This may happen if the handling for all error types is the same (e.g. "if, for any reason, I cannot read this preference value, I will return the default value of 5" - that's infinitely better than your program crashing because you didn't realise it might throw an exception due to a network timeout). If used wisely, this approach can make your program bullet proof - but if used unwisely you can suppress errors that you need to know about and fix, and this can be very painful.

When suppressing any exception, you should always carefully consider error reporting - should you tell the user that you have had a problem? Should you log it in a trace file so that when a customer complains of something not working right, you can track back to the source of the problem? Or should you silently ignore it? Just be careful as over-zealous suppressions can make it very difficult to work out why a progam is behaving unpredictably.

Jason Williams
A: 

I get a feeling that you see things like black & white, and that doesn't feel right.. Yes, most of the times, it's not good to catch everything and be sloppy when it comes to validation of input, but not always. This might very well be a special case. I don't know anything about what kind of files that's going to be parsed, but exceptions might be the right thing.

Before we decide what's best, give us more details :)

Onkelborg
+3  A: 

I think what you're asking is this:

When parsing a file line by line, sometimes the current line has malformed data which causes an exception to be thrown in your code. Perhaps you simply need to structure your code such that the try/catch only surrounds the parsing code, not the line-reading code. For instance:

using System;
using System.IO;
using System.Windows.Forms;

public void ParseFile(string filepath)
{
    TextReader reader = null;

    try
    {
        reader = new StreamReader(filepath);

        string line = reader.ReadLine();
        while (!string.IsNullOrEmpty(line))
        {
            try
            {
                // parse line here; if line is malformed, 
                // general exception will be caught and ignored
                // and we move on to the next line
            }
            catch (Exception)
            {
                // recommended to at least log the malformed 
                // line at this point
            }

            line = reader.ReadLine();
        }
    }
    catch (Exception ex)
    {
        throw new Exception("Exception when trying to open or parse file", ex);
    }
    finally
    {
        if (reader != null)
        {
            reader.Close();
            reader.Dispose();
        }
        reader = null;
    }
}

The outer try/catch block is to handle closing the reader object properly if something happened when trying to open or read the file. The inner try/catch block is to swallow exceptions raised if the read data was malformed and couldn't be parsed correctly.

I agree with pretty much everyone else, though. Just swallowing the exception might not be good. At the very least, I recommend you log it somewhere.

Thorin