The main reason for exceptions (from a developer point of view) is to detect malfunctioning code. Because once this malfunctioning code is executed, the result of an operation and therefore the state of an application can be considered as undefined, since unexpected/unpredictable side effects will transition the application to an unpredictable state.
Data might be in an unpredictable state i.e. data integrity is not guaranteed any more. Unmanaged resources might be initialized or reserved but not released, which may even affect other applications. So most of the time you want the application to stop execution to avoid unpredictable behavior.
Exceptions are designed to hold a program. They are controlled on OS level so that Operating System can stop the application and also provide a snapshot from the stack memory, that was occupied by the faulting application.
We can say that there are expected errors e.g., not enough disk space to save a file or wrong user credentials and there are unexpected errors e.g. a null reference exception.
So if the application could recover/rollback from an error, this error will be an expected error. You know in advance what can fail and you know how to handle this situation to keep the application operating reliably. You preferably would want to handle such errors "silently", without throwing exceptions, as exceptions have impact on application performance and therefore user experience.
You also don't want the exception to propagate through your complete application, which is expensive in terms of performance. Therefore you would like to handle it directly where the exception occurred. This implies that you better don't handle exceptions in a big catch block around your application. You always catch exceptions where they occur. This is a best practice for the reasons explained.
It's also best practice to use multiple catch blocks and catch specific exceptions only. This way your code becomes more readable. Specific exceptions are those and only those you can handle. And logging is not considered handling (if this exception is not relevant you should have avoided it in the first place). Every other exception must be thrown as they are unexpected or have heavy impact on the stability (predictability) of the application.
This means catching Exception, like it is happening in your code, is a very bad idea:
try
{
// Create application main directory
Directory.CreateDirectory(Reference.AppPath);
}
catch (Exception ex) // Bad: this will swallow every exception although unhandled by this catch block
{
Log.Error(ex);
if (ex is DirectoryNotFoundException || ex is IOException || ex is NotSupportedException)
{
MessageBox.Show("Error");
}
}
Bad exception handling as all exceptions are swallowed. Logging an exception is not handling an exception.
What is happening here, is that all exceptions are swallowed. Especially those that are critical to the application. By catching Exception you basically handle every exception. Catching Exception at application level makes it even worse: a NullReferenceException will never see the light. A IndexOutOfRangeException will never get attention too, as you didn't even re-throw anything (re-throwing an exception is also generally considered as bad practice). A NullReferenceException must crash your application and requires the developer's full attention. If Reference.AppPath throws a NullReferenceException because Reference or AppPath is null we will never notice it without consulting any log file.
In real life, nobody is reading log files. Every developer avoids it. And for sure, nobody reads them daily to check if any of the hundreds of customers experienced a critical runtime error. Indeed, it's an uncomfortable situation when a customer experiences an application crash, but at least it will draw everybody's attention to look for an immediate fix. An application error reporter can be a very powerful enhancement to provide quick and proper bug support.
Nevertheless log files can be a very valuable support to reproduce and find bugs.
Also, exceptions of the same type might get thrown at different code segments in a totally different context. A global exception handler will have a hard time to get the context of the exception to find out if NotSupportedException was thrown in context of a invalid user input (recoverable error) or a wrong call to an API method (non-recoverable).
To solve the issue of swallowing exceptions, you have to get rid of the smelly switch statement, which already indicates that you are doing something "wrong". Type checking in a switch statement (or ugly and unreadable if-else constructs) is a classic code smell.
The goal is to handle exceptions that are non-critical i.e. the application can recover from and leave every other exception unhandled:
try
{
// Create application main directory
Directory.CreateDirectory(Reference.AppPath);
}
catch (DirectoryNotFoundException ex)
{
// Log
Log.Error(ex);
MessageBox.Show("Error");
}
catch (UnauthorizedAccessException ex)
{
// Log
Log.Error(ex);
MessageBox.Show("Error");
}
Now, any developer that comes across this code knows immediately what is going on here. The code has become more readable and the ugly and smelly switch was successfully eliminated. But more important, every unhandled exception will still crash the app, to indicate a critical bug. If Reference.AppPath throws a NullReferenceException we will now notice it first hand this time, without the need of consulting any log file.
Don't catch what you can't handle.
If you wish to log unhandled exceptions, instead subscribe to the Application.DispatcherUnhandled event:
public partial class App : Application
{
public App()
{
this.DispatcherUnhandledException += (sender, args) => this.logger.Error(args.Exception.Message);
}
}
Free Resources
As a note: it's best practice to take care of managed/unmanaged resources and use finalizers i.e. finally or using blocks, to make sure that these resources are released i.e. disposed properly under any circumstances before letting the application crash.
In case the following code throws an exception, the using statement will execute a finalizer before the forced exit:
using (var sqlConnection = new SqlConnection(args))
{
sqlConnection.ExecuteQuery(queryArgs);
}
This is equal to:
try
{
var sqlConnection = new SqlConnection(args);
sqlConnection.ExecuteQuery(queryArgs);
}
finally
{
sqlConnection.Dispose();
}
Both implementations won't handle the critical exception, but will release unmanaged resources before the method is put on hold and the exception routing is executed by the OS.
Re-throwing Exceptions
Although re-throwing exceptions is considered bad practice, it makes sense if you need to add data to the exception or like to throw a more specialized maybe custom exception type. But make sure to keep the stack trace intact.
Bad re-throw as it will break the stack trace:
try
{
// Create application main directory
Directory.CreateDirectory(Reference.AppPath);
}
catch (DirectoryNotFoundException directoryNotFoundException)
{
Log.Error(directoryNotFoundException);
// The previous stack trace of 'directoryNotFoundException' will be lost
throw new UnknownUserInputException("Custom Message");
}
Good re-throw as it will preserve the full stack trace by setting
the original exception as inner exception of the wrapping exception type:
try
{
// Create application main directory
Directory.CreateDirectory(Reference.AppPath);
}
catch (DirectoryNotFoundException directoryNotFoundException)
{
Log.Error(directoryNotFoundException);
// Use constructor to set inner exception
throw new UnknownUserInputException("Custom Message", directoryNotFoundException);
}
Avoiding Exceptions in the First Place
Microsoft best practices recommends to "Design classes so that exceptions can be avoided".
This is especially true for user input, since this are always errors we can recover from. In general, data input whether the source is a user or a client application, can be validated before it enters the application. The goal is to have the application to deal with valid data only. The logic should not process invalid data and throw an exception. The data persistence layers should never persist invalid data.
To prevent invalid data from being processed, you would typically use a form of data validation. In case of a WPF application, which is what you are implementing, I highly recommend to apply the MVVM pattern and implement the INotifyDataErrorInfo interface (example) which is part of the WPF framework and enables error propagation to the UI to provide a visual feedback. If the user enters an invalid file path you can easily make the UI draw e.g. a red border around the concerning input field, with messages or hints, forcing the user to correct the input.
In this special scenario of picking a file or folder, the best way is to use a file or folder picker dialog. This way, invalid filesystem path inputs are almost impossible.
Another example is file saving. Before saving a file, you can check whether the operation can be executed by checking the available disk space to avoid I/O exceptions.
Almost all expected exceptions can be avoided by checking the preconditions before executing a critical operation.
The bottom line is, that you should never catch Exception. Swallowing exceptions is dangerous as it will conceal serious bugs. To log an exception is not handling an exception. Logging an exception and done is equal to swallowing it, as you can't be sure somebody will ever read the log data or search it for random errors.
If you wish to log all unhandled exceptions by default, subscribe to the Application.DispatcherUnhandledException event, but never use a global catch block at application level.
Especially in your case where the errors or exceptions are triggered by invalid user input, you are advised to use data validation. This makes exception handling obsolete and is the preferred way. User input validation e.g., by implementing INotifyDataErrorInfo, includes a convenient way to give the user feedback and prompt him to correct the input.
In case the exception is not critical, in a away that it requires the application to halt, consider to roll it back to a defined state before continuing execution in order to prevent dirty data or undefined states.
Don't forget to free resources using a finally or using block to properly close/dispose those objects.
