Please Don’t Do This

4 April, 2007. 307 words.

Over the last couple weeks I’ve been enjoying a lovely little application that repeatedly polls for xml data from a web service. The main loop of this application looks something like this:

public void WebServicePoller()
{
    int numberOfWebExceptions = 0;
    bool continueDownload = true;
    bool abortDownload = false;
    DateTime pollingDate;

    while( pollingDate < DateTime.Now )
    {
        try
        {
            try
            {
                // 1. retrieve xml data from web service
                // 2. validate xml data
                // 3. store xml data in file
            }
            catch( System.Xml.XmlException ex )
            {
                throw new Exception( "Unexpected XML data." );
            }
            catch( WebException ex )
            {
                numberOfWebExceptions++;
                if( numberOfWebExceptions > 20 )
                    throw new Exception( "Failed too many times." );
                else
                    continueDownload = false;
            }
            catch( Exception ex )
            {
                abortDownload = true;
                throw new Exception( ex.Message );
            }
        }
        catch( Exception ex )
        {
            abortDownload = true;
        }

        if( abortDownload )
            break;

        if( continueDownload )
            pollingDate = pollingDate.AddDays( 1 );
    }
}

That is not exact, of course, because I’m going from memory. The real method was several pages long, but that’s the basic structure. (I left out the numerous areas where exceptions were logged and displayed.)

I would like to encourage any young developers out there to never, ever write anything that resembles that. I would like to think it’s obvious what the problems are, but in case it’s not, here’s one way to refactor it:

public void BetterWebServicePoller()
{
    int numberOfWebExceptions = 0;
    DateTime pollingDate;

    try
    {
        while( pollingDate < DateTime.Now )
        {
            try
            {
                // 1. retrieve xml data from web service
                // 2. validate xml data
                // 3. store xml data in file
                pollingDate = pollingDate.AddDays( 1 );
            }
            catch( System.Net.WebException ex )
            {
                numberOfWebExceptions++;
                if( numberOfWebExceptions > 20 ) throw;
                // log/print the exception
            }
        }
    }
    catch( Exception ex )
    {
        // log/print the exception
    }
}

Related