I have had some really interesting discussions recently on a variety of well intentioned but poorly constructed “Try-Catch” blocks. The worst and most most egregious I  have seen follow the anti-pattern that purposefully uses the exception to determine if a method succeeded or failed. This is usually appropriate as a by-product, but certainly not by design. Try-Catch is slow and cumbersome. Do not do the following: 

//The worst of the worst...
private bool SomeFunction()
{
    try
    {
        ThrowMe();
    }
    catch (Exception ex)
    {
        return false;
    }
    return true;
}

So how about when you are trying to allow an exception to bubble upward through the call stack. There are four ways to do this, two of which are generally wrong, two of which are usually appropriate. The following two versions are bad because we ultimately lose data when we bubble up the stack. Any situation where you prohibit the details in the call stack should usually be avoided.

private void BadThrow()
{
    try {
        ThrowMe();
    }   
    catch (IOException ex) 
    { 
        throw new Exception("Some error"); // bad - no details of the exception 
        //or
        throw ex;  // bad - we lose the call stack information ...
    }
}

Usually I am encouraging developers just to do a simple throw. That way whatever is handling this method one level up gets exactly what was thrown at this level. Additionally you can consider throwing a new exception and sending the current exception as an inner exception:

private void GoodThrow()
{
    try
    {
        ThrowMe();
    }
    catch (IOException ex)
    {
        throw;     // good - simply throws the details on
        //or
        throw new Exception("Some other error", ex); //good - Gives details + inner exception
    }
}

Finally, and I expect push back here, I like to see initialization of an object (file, db, etc) outside of the try statement. In this example the ‘new’ statement occurs within the try statement, therefore we must do additional work within the finally block in order to ensure that we are not using the method on a null object. Here is the bad version:

private void TryCatch()
{
    Stream s = null; 
    try { 
        s = new FileStream("");      
        //...
    }  
    finally { 
        if(s!=null)  
           s.Close();  
    }
}

Here is the improved version with ‘new’ statement outside the try, and no additional validation within the finally block:

private void TryCatchBetter()
{
    Stream s = new FileStream("");  
    try { 
        //...  
    } 
    finally { 
        s.Close();
    }
}

I will grant that the last issue is kind of picky, so let the heated discussions begin! What else should I include here?



Comment Section








Comments are closed.