In this post I use the categorization of exceptions by @Eric Lippert which you can find here: Vexing exceptions
The most important ones in this case:
Boneheaded exceptions are your own darn fault, you could have prevented them and therefore they are bugs in your code. You should not catch them; doing so is hiding a bug in your code. Rather, you should write your code so that the exception cannot possibly happen in the first place, and therefore does not need to be caught.
Exogenous exceptions appear to be somewhat like vexing exceptions except that they are not the result of unfortunate design choices. Rather, they are the result of untidy external realities impinging upon your beautiful, crisp program logic. Always handle exceptions that indicate unexpected exogenous conditions; generally it is not worthwhile or practical to anticipate every possible failure. Just try the operation and be prepared to handle the exception.
Like every developer probably experienced it is not possible to avoid boneheaded exceptions to 100% in a large enterprise software.
In the unfortunate situation that a boneheaded exceptions is thrown, i want to inform the user, so that he will report the bug to us (third level support). In addition, I want to log a message with log level "Error" in this case.
For exogenous exceptions i want to show the user a more specific message with some hints, because he can probably fix the problem himself (maybe with help of first or second level support)
The way I currently achieve this, is by catching ONLY exogenous exceptions explicitly in low level components and wrapping them into custom exceptions. In the top layer (in my case the ViewModel of a MVVM WPF application) I then explicitly catch the custom exception, to show the warning. In a second catch block I catch general Exceptions to show the error.
Is this a common and good practices to distinguish between boneheaded exceptions and exogenous exceptions in enterprise applications? Is there a better approach? Or is it not necessary at all?
After reading this article dotnetpro - Implementierungsausnahmen I am also wondering, if I should wrap all (also boneheaded) exceptions into custom exceptions to provide more context information when logging them?
About wrapping all exceptions I found following posts: stackoverflow - Should I catch and wrap general Exception? and stackoverflow - Should I catch all possible specific exceptions or just general Exception and wrap it in custom one? It seams to be pretty controversial and depended on the use-case, so I am not sure in my case.
Example for a high level catch handler in a ViewModel:
public class MainWindowViewModel
{
private readonly ICustomerRepository _customerRepository;
public MainWindowViewModel(ICustomerRepository customerRepository)
{
_customerRepository = customerRepository;
PromoteCustomerCommand = new DelegateCommand(PromoteCustomer);
}
public ICommand PromoteCustomerCommand { get; }
private void PromoteCustomer()
{
try
{
Customer customer = _customerRepository.GetById(1);
customer.Promote();
}
catch (DataStoreLoadException ex)
{
// A expected exogenous exception. Show a localized message with some hints and log as warning.
Log(LogLevel.Warning, ex);
ShowMessage("Unable to promote customer. It could not be loaded. Try to...", ex);
}
catch (Exception ex)
{
// A unexpected boneheaded exception. Show a localized message, so that the users contacts the support and log as error.
Log(LogLevel.Error, ex);
ShowMessage("Unable to promote customer because of an unknown error. Please contact [email protected]", ex);
}
}
}
Example for a low level exception wrapping:
public class SqlCustomerRepository : ICustomerRepository
{
public Customer GetById(long id)
{
try
{
return GetFromDatabase(id);
}
catch (SqlException ex)
{
// Wrap the exogenous SqlException in a custom exception. The caller of ICustomerRepository should not depend on any implementation details, like that the data is stored in a SQL database.
throw new DataStoreLoadException($"Unable to get the customer with id {id} from SQL database.", ex);
}
// All other exceptions bubble up the stack without beeing wrapped. Is it a good idea, or should I do something like this to provide additional context? (Like the id in this case)
/*catch (Exception ex)
{
throw new DataStoreException($"Unknown error while loading customer with id {id} from SQL database.", ex);
}*/
}
}
Although there's no classification within our code that's quite so precise, our exception handling often implicitly indicates whether or not we thought a specific exception was possible (exogenous) or whether we're just accounting for possible mistakes.
Using Eric's example, if we access a file, put it in a try/catch, and explicitly catch FileNotFoundException, that should indicate that we realized the FileNotFoundException was a possible outcome even though we checked a millisecond earlier to see if it exists.
If, on the other hand, our code includes this:
try
{
// do some stuff
}
catch(Exception ex)
{
// maybe log it
}
...this suggests that we're taking into account the boneheaded exception, something that could have happened anywhere in the code executed within the try.
What (sort of) distinguishes them is that one indicates that we realized it was possible and accounted for it, while the other says, "Let's hope nothing goes wrong here."
Even that distinction isn't perfectly clear. Our file access code might be in a "vague" try/catch(Exception ex) block. We know that it's remotely possible that, due to a race condition, the file might not exist. In that case we'll just let that vague exception handling catch it. That might depend on what needs to happen. If we were deleting the file and it turns out that it doesn't exist, we don't need to do anything. If we needed to read it and now it's gone, that's just an exception. It might not do us any good to catch that specific exception if the outcome will the same as for any other exception.
Similarly, just because we catch an exception explicitly doesn't guarantee that it's not "boneheaded." Maybe I'm doing something wrong, and sometimes my code throws ObjectDisposedException. I have no idea why it does that, so I add catch(ObjectExposedException ex). At first glance it might look like I know what's going on in my code, but I really don't. I should have figured out the problem and fixed it instead of catching an exception without having any idea why it happens. If the application doesn't work once in a while and I don't know why, the fact that I caught an exception is at best useless or at worst harmful because it hides what's really going on.
This is not to say that we should add try/catch statements in every single method to catch "boneheaded" exceptions. That's just an example of exception handling that accounts for the possibility of an exception which may or may not have been a mistake. It's usually not useful to do that in every method. We might put just enough around the edges to make sure that any exception thrown will at least get logged.
As for catching and wrapping exceptions in new exceptions, it often comes down to what you plan to do with that extra information you're creating. Quite often the answer is nothing.
We could have one layer of an application that throws all sorts of cleverly wrapped custom exceptions. Then another layer calls it and does this:
try
{
_otherLayer.DoSomeStuff();
}
catch(Exception ex)
{
_logger.Log(ex);
}
What did we do with our fancy custom exception? We just logged it, the same as we would have if we hadn't wrapped it. When we look at our logs we ignore the custom exception and just look at the stack trace of the original exception. It tells us what assembly, class, and method the exception came from. That's likely all we need.
If the purpose of the wrapped exception is to add context, like
throw new DataStoreLoadException($"Unable to get the customer with id {id} from SQL database.", ex);
...that could be useful. If we didn't do that then the exception would be "The sequence contains no elements." That's not as clear.
But is it possible that we would get the exact same mileage from this?
throw new Exception($"Unable to get the customer with id {id} from SQL database.", ex);
If there's not a line of code anywhere that says catch(DataStoreLoadException ex) and does something different as a result than it would do with any other exception, then chances are we're getting no benefit from it.
It's worth noting that years ago Microsoft recommended that our custom exceptions inherit from ApplicationException instead of Exception. That would distinguish between custom and system exceptions. But it became apparent that the distinction added no value. We weren't saying, "If it's an ApplicationException do this, otherwise do that." The same is often true for the other custom exceptions we define.