I've been reviewing the source code of a mid-size .netcore application used as a backend for a mobile application, and something caught my attention momentarily - the unusually (to me) high usage of exceptions throughout the code. It is a fairly standard aspnetcore MVC application with all standard architectural pieces such as controllers, services, database context, entities, etc.
Control Flow
As I spent more time reading the code it became clear that exceptions were used to control flow.
For example, a simple code for retrieving an account for a given account ID had 2 exceptions thrown and looked like this:
public async Task<Account> GetById(Guid id)
{
if (id == Guid.Empty)
{
throw new AccountServiceException("Invalid Account Id");
}
var account = await _dbContext.Accounts.FirstOrDefaultAsync(a => a.Id == id);
if (account == null)
throw new AccountServiceException("Account not found");
return account;
}
Expected Behavior
Since this example deals with an Account entity from the database, the non-existence of such an entity for provided parameters is considered expected behavior. So, throwing an exception (even a custom and controlled one) is unjustified. Checking the validity of incoming arguments is considered good practice (defensive programming if I recall correctly), however in this case new ArgumentException()
will be more appropriate than the custom exception. On a second look, even if we won't be checking Guid
for being empty it won't hurt, since it is a struct and won't be null anyway, at least they don't use nullable Guids (and I believe there won't be Accounts with such Id)
Developer Perspective
Reading and maintaining this type of code adds additional mental burden to the developers working with this codebase. It took me a while to understand and wire all references between these exceptions and places where they were caught. As I decided to run and debug this application locally it was very inconvenient and disorienting when my debugger was jumping all over the code when an exception was thrown and caught. Essentially my debugging was constantly interrupted by these Exceptions try/catch blocks.
What would do?
I'd replace the first exception with the ArgumentException
. As far as other exceptions that control the flow of the application I'd replace them with a Result object (I recommend using the CSharpFunctionalExtensions NuGet package by Vladimir Khorikov) and return it with Success or Fail.