I am making an ASP.NET Core Web API project with controllers. All API controllers derive from ControllerBase that has NotFound() method. I use this method from controllers whenever I can't find a resource that the client requested. However, sometimes I want to encapsulate the whole logic out of the controller action into a separate service. In this case, I can't use the NotFound() method directly.
I was wondering should this service throw a custom MyNotFound exception when a resource can't be found. Then in the global exception handler, I can handle this exception and return a 404 status code to the client.
Or should the service return IActionResult and then in the service method I can return new NotFoundObjectResult() (just like ControllerBase.NotFound()) instead of throwing the exception method?
Trade-offs
What makes my mind tickle is the decision I need to make and the tradeoff it brings. If I decide that the service will be throwing an exception that makes code cleaner because the service is not dependent on ASP.NET Core abstractions such as IActionResult and NotFoundObjectResult. However, throwing exceptions is an expensive operation and it takes more time for the server to process it than simple object returns.
On the other hand, if the service returns IActionResult from the operation, it makes things faster in case of an error but it couples the service with ASP.NET Core types.
Both approaches have pros and cons and I can't decide which one to use.
Approach with throwing an exception
Here is the example code with the approach where I throw exceptions:
[Route("api/users")]
[ApiController]
public class UsersController : ControllerBase
{
// ... unimportant code
[HttpDelete("{id}")]
[ProducesResponseType(StatusCodes.Status204NoContent)]
[ProducesResponseType(StatusCodes.Status404NotFound)]
public async Task<IActionResult> DeleteUser([Required] string id)
{
if (!ModelState.IsValid)
{
return BadRequest();
}
await userManagementService.DeleteUser(id); // all logic inside this service method
return NoContent();
}
}
Where UserManagementService is:
public class UserManagementService : IUserManagementService
{
// ... unimportant code
public Task DeleteUser(string id)
{
var user = await _dbContext.Users.FindAsync(id);
if (user == null)
{
throw new MyNotFoundException($"User with id: {id} not found");
}
// ... code that deletes the user and cleanups all resources associated
// with it (eg. things not just in the db but also user files on the content server)
}
}
Approach with returning IActionResult
[Route("api/users")]
[ApiController]
public class UsersController : ControllerBase
{
// ... unimportant code
[HttpDelete("{id}")]
[ProducesResponseType(StatusCodes.Status204NoContent)]
[ProducesResponseType(StatusCodes.Status404NotFound)]
public async Task<IActionResult> DeleteUser([Required] string id)
{
if (!ModelState.IsValid)
{
return BadRequest();
}
return await userManagementService.DeleteUser(id); // all logic inside this service method
}
}
Where UserManagementService is:
public class UserManagementService : IUserManagementService
{
// ... unimportant code
public Task<IActionResult> DeleteUser(string id)
{
var user = await _dbContext.Users.FindAsync(id);
if (user == null)
{
return new NotFoundObjectResult($"User with id: {id} not found");
}
// ... code that deletes the user and cleanups all resources associated
// with it (eg. things not just in the db but also user files on the content server)
// ... success
new NoContentResult();
}
}
Should UserManagementService.DeleteUser() method return IActionResult
Rule of thumb that I have established in recent years is that exceptions should be thrown only in truly exceptional situations (i.e. resulting in the 500 Internal Server Error, and in general servers should not return 500s). There are several reasons:
The main point being that other usages can be considered as the infamous "exceptions as control flow" which is generally considered as antipattern:
You can't expose the exceptions in method contracts (in .NET), so you should always be aware about actual implementations (which affects the reusability of the code outside of ASP.NET environment).
As you mentioned being performance (though it is rarely a deciding factor because rarely exceptions overhead is that significant).
And you should not return
IActionResultfrom services, there are multiple options for return types which are better, like:FindSomethingmethod which returns nullable type which is mapped intoNotFoundResultin the controller)FluentResultsEither/Maybemonads for example from C# Functional Programming Language Extensions or from CSharpFunctionalExtensionsOneOfThe basic idea being that you get the result and map it into response (possibly using some kind of chained fluent calls like
MapSuccessif you don't want multipleif-elses orif-returns). TBH I have not found ideal one for me which would easily cover all cases but still prefer this to throwing/catching exceptions which can quickly can become cumbersome with multiple catch blocks.