Catching expected exceptions

Question

I've got 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?

Answer 1

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.

Answer 2

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 */
}

Answer 3

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.

Answer 4

My recommendations:

Check to see if the connection is null as a first step.

if (conn == null){
return;
}

There is no point in processing if it is null. There seems to be some school of thought out there that says try statements are magic. You don't have to put anything inside a try statement. So therefore to address one of the questions you had, if you don't care don't even bother wrapping it in the statement.

Also look into using if statements, as it appears that is what you were going for above.

Answer 5

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;
}

Answer 6

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).

Answer 7

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 {}