Catching expected exceptions

I've go开发者_运维百科t code that looks like this because the only reliable way for me to check if some data is an image is to actually try and load it like an image.

    static void DownloadCompleted(HttpConnection conn) {
        Image img;
        HtmlDocument doc;

        try {
            img = Image.FromStream(conn.Stream);
        } catch {
            try {
                doc = new HtmlDocument();
                doc.Load(conn.Stream);
            } catch { return; }
            ProcessDocument(doc);
            return;
        }
        ProcessImage(img);
        return;
    }

Which looks down right terrible!

What's a nice way of handling these situations? Where you're basically forced to use an exception like an if statement?

Your logical structure is

if( /* Loading Image Fails */ )
    /* Try Loading HTML */

so I would try to make the code read that way. It would probably be cleanest (though admittedly annoyingly verbose) to introduce helper methods:

bool LoadImage()
{
    Image img;
    try 
    {
        img = Image.FromStream(conn.Stream);
    } 
    catch( NotAnImageException /* or whatever it is */ )
    {
        return false;
    }

    ProcessImage(img);
    return true;
}


bool LoadDocument()
{
   // etc
}

So you can write

if( !LoadImage() )
   LoadDocument();

or extend it to:

if( !LoadImage() && !LoadDocument() )
{
     /* Complain */
}

I think empty catch blocks, the way you've coded them, are a very bad idea in general. You're swallowing an exception there. No one will ever know that something is amiss the way you've coded it.

My policy is that if I can't handle and recover from an exception, I should simply allow it to bubble up the call stack to a place where it can be handled. That might mean nothing more than logging the error, but it's better than hiding the error.

If unchecked exceptions are the rule in .NET, I'd recommend that you use just one try/catch block when you must within a single method. I agree - multiple try/catch blocks in a method is ugly and cluttering.

I'm not sure what your code is trying to do. It looks like you're using exceptions as logic: "If an exception isn't thrown, process and return an image; if an exception IS thrown, process and return an HTML document." I think this is a bad design. I'd refactor it into two methods, one each for an image and HTML, and not use exceptions instead of an "if" test.

I like the idea of Eric's example, but I find the implementation ugly: doing work in an if and having a method that does work return a boolean looks very ugly.

My choice would be a method that returns Images, and a similar method

Image LoadImage(HttpConnection conn)
{
    try 
    {
        return Image.FromStream(conn.Stream);
    } 
    catch(NotAnImageException)
    {
        return null;
    }
}

and do the work in the original method:

static void DownloadCompleted(HttpConnection conn)
{
    Image img = LoadImage(conn);

    if(img != null)
    {
         ProcessImage(img);
         return;
    }

    HtmlDocument doc = LoadDocument(conn);

    if(doc != null)
    {
         ProcessDocument(doc)
         return;
    }

    return;
}

In conjunction to duffymo's answer, if you're dealing with

• File input/output, use IOException
• Network sockets, use SocketException
• XML, use XmlException

That would make catching exceptions tidier as you're not allowing it to bubble up to the generic catch-all exception. It can be slow, if you use the generic catch-all exception as the stack trace will be bubbling up and eating up memory as the references to the Exception's property, InnerException gets nested as a result.

Either way, craft your code to show the exception and log it if necessary or rethrow it to be caught by the catch-all exception...never assume the code will not fail because there's plenty of memory, plenty of disk and so on as that's shooting yourself in the foot.

If you want to design your own Exception class if you are building a unique set of classes and API, inherit from the ApplicationException class.

Edit: I understand better what the OP is doing...I suggest the following, cleaner implementation, notice how I check the stream to see if there's a HTML tag or a DOC tag as part of XHTML...

static void DownloadCompleted(HttpConnection conn) {
        Image img;
        HtmlDocument doc;
        bool bText = false;
        using (System.IO.BinaryReader br = new BinaryReader(conn.Stream)){
            char[] chPeek = br.ReadChars(30);
            string s = chPeek.ToString().Replace(" ", "");
            if (s.IndexOf("<DOC") > 0 || s.IndexOf("<HTML") > 0){
                // We have a pure Text!
                bText = true;
            }
        }
        if (bText){
            doc = new HtmlDocument();
            doc.Load(conn.Stream);
        }else{
            img = Image.FromStream(conn.Stream);
        }
}

Increase the length if you so wish depending on how far into the conn.Stream indicating where the html tags are...

Hope this helps, Best regards, Tom.

There is no need to try to guess the content type of an HTTP download, which is why you are catching exceptions here. The HTTP protocol defines a Content-Type header field which gives you the MIME type of the downloaded content.

Also, a non-seekable stream can only be read once. To read the content of a stream multiple times, you would have to copy it to a MemoryStream and reset it before each read session with Seek(0, SeekOrigin.Begin).

Answering your exact question: you don't need a catch block at all, try/finally block will work just fine without a catch block.

try
{
    int i = 0;
}
finally {}