Exception (Mis)Handling

Exception handling was originally created to try to give developers a way to separate out error handling so that it wouldn’t clutter up and distract from the core functionality trying to be accomplished. When done well, exception handling can provide a clean way to instrument and separate truly exceptional conditions from the core flow of your methods as well as a way to prevent nasty crashes and untraceable bugs. When done badly, exception handling can be misused for decision making, masking errors and bugs, and distracting developers from doing real work.Go read these when you have time, and you have time if you are reading this – admit it.Chapter 19 of CLR via C# and Guidelines: Exception Handling.There is not a lot that can be said that isn’t said here about .NET exception handling that isn’t in the resources above, but I’ve picked up on a common anti-pattern that stems from a misunderstanding of what exception handling is. I call it the “safety-net catch”, and it goes a little something like this:

public bool Import()
{
	bool isSuccess = false;
   	try    
	{     
		if (SomethingIsWrong())         
			throw new ApplicationException("Start panic sequence now.");
	        bSuccess = DoSomething(WithThis, AndThis);     
		foreach (string databaseThing in theDatabase)        
		{            
			if (bSuccess) bSuccess = DoSomethingUseful(databaseThing);
	        }    
	}    
	catch (Exception ex)    
	{        
		PublishException(ex);        
		isSuccess = false;    
	}    
	return isSuccess;
}

In this example, DoSomethingUseful, DoSomething, and most likely PublishException all have this same pattern of a ‘catch all’ at the end. We aren’t getting much benefit from this style of exception handling as it merely serves to make sure that this method always returns bSuccess so that execution should continue with any failure.I think it would be easier on everyone if this code was changed to simply not handle exceptions that it can’t actually handle. The top level threads of your application should have a “catch all” that publishes the exception, and then displays a nice message for the user in the case of a web application. So if you want to fail, just let it fail if you can’t recover from it. The contract of the method Import above doesn’t say that it doesn’t throw exceptions – if it can’t do its job it should throw or allow an exception to bubble up.There are cases where you don’t want the code to break out in the case of any exception, but these are rare. In the case of row level handling where we are parsing a file and you don’t want row to ruin the whole file you can simply move exception handling down to that import piece. In this case non- exception based mechanisms can be used or you can throw a custom exception type or an exception with well understood semantics like InvalidOperationException or ArgumentException.Please do:

  • Throw an exception in a method if you can’t do the method’s job. If you don’t know what the method should do because it does 23 things, 18 of which can happen if it isn’t given a valid value for AccountNumber, refactor until you have 23 methods, one of which throws InvalidArgumentException when it gets an invalid AccountNumber.
  • Only do a ‘catch’ if you are going to handle it (maybe the syntax should say handle). Just publishing or setting a variable doesn’t count in the pattern above.
  • If you for some reason catch and rethrow (like if you want to add to the exception object), do a throw and not a throw origEx to keep the call stack.
  • Remember that your most common ‘exceptional condition’ might be a database timeout or other database exception (System.Data.SqlClient.SqlException) and is not an ApplicationException but most likely can’t be recovered from anyway.

Please don’t do this:

try
{
	bc.Save(o); 
	DB.UpdateProcessFlag(conn, Util.GetInt32(dr, "Key"), 1);}
	catch
	{ 
		//Error on save, so mark this one as errored. 
		DB.UpdateProcessFlag(conn, Util.GetInt32(dr, "Key"), -1);
	}
}

There are many reasons that this .Save() could fail, and a SqlException or (ObjectReferenceException on dr) are going to be badly mishandled here. If you find yourself feeling like you need to use the try/catch mechanism for this sort of stuff perhaps take a step back and think about how to handle it in terms of design.And please don’t do this unless you like hearing the sound of me dying inside. (Use the TryParse pattern instead)

try { o.Something = -aNumba; }catch { o.Something = 0; }
try { amt = Util.GetDecimal(dr, "FieldName"); } catch { amt = 0; }if (amt == 0) return;