1

I've read (here) that it's not a good idea to do Try/Catch blocks and avoid exceptions in a Web API setting. However, if you want to catch and log errors as they happen in your app... wouldn't a try/catch be the best way to go about things? Here's an example of what I've done in my application - I'm curious if anyone has a better way. My ErrorHander class saves the error to the database and emails administrators the details of the error.

Controller

namespace MyApp.Controllers
{
    [Route("api/[controller]")]
    public class AuthController : Controller
    {
        private readonly IAuthRepository _repo;
        private readonly IErrorHandler _errorHandler;

        private AuthController(IAuthRepository repo, IErrorHandler errorHandler) {
            _errorHandler = errorHandler;
        }

        [Authorize]
        [HttpPut("changePassword")]
        public async Task<IActionResult> ChangePassword(
            [FromBody] UserForChangePasswordDto userForChangePasswordDto)
        {
            var userFromRepo = await _repo.Login(userForChangePasswordDto.Username,
                    userForChangePasswordDto.OldPassword, null);

            try
            {
                //logic to update user's password and return updated user

                return Ok(new { tokenString, user });
            }
            catch (Exception ex)
            {
                // emails error to Admin, saves it to DB, then returns HttpStatusCode
                return await _errorHandler.HandleError(
                    HttpStatusCode.InternalServerError, Request, new Error
                {
                    Message = ex.Message,
                    StackTrace = ex.StackTrace,
                    User = userFromRepo != null ? userFromRepo.Username : "Invalid User"
                });
            }
        }
    }
}
Improve this question
6
  • 2
    The answer you linked to said it's best not to catch fatal exceptions, and to properly handle other exceptions when you can handle them. In your case you are handling the exception by logging it, however, this should be handled by Web API's global error handler, not in the controller. Commented Nov 13, 2018 at 18:46
  • 2
    Handling errors in Web API is a cross-cutting concern. Every controller action method needs some form of error handling. Web API provides facilities that does this for every controller you have. learn.microsoft.com/en-us/aspnet/web-api/overview/… Commented Nov 13, 2018 at 18:49
  • I think linked post perfectly answers your question. Though it doesn't contain any examples for each case, you generally should avoid try/catch, not just log it and forget, but avoid it in root. So, in your example what could be possibly go wrong in ChangePassword what can't happen in other methods of same controller? I think, this particular catch should be global, not in method. Method should always return Ok if not said otherwise by business rules. Commented Nov 13, 2018 at 18:49
  • 2
    It's better to look here at the ASP.NET Core docs rather than the ASP.NET Web Api docs linked above. Commented Nov 13, 2018 at 19:10
  • 1
    As per these suggestions, I ended up instantiating my ErrorHandler class in the Startup class's app.UseExceptionHandler. This is much easier. And then I can still use the same ErrorHandler class to give (and log) custom responses. Commented Nov 13, 2018 at 20:24

2 Answers 2

Reset to default
6

I have a few recommendations:

  1. Avoid try..catch entirely when possible. For example, use methods like TryParse, TryCreate, etc, instead of methods that throw exceptions. The same goes with the *OrDefault LINQ methods, e.g. always use SingleOrDefault rather than Single, etc. Basically, if there's a way to avoid throwing exceptions at all, use that.

  2. When you do need to handle exceptions do so in your abstractions, not your app code. For example, presumably, the //logic to update user's password and return updated user line is using your IAuthRepository. Your repo's method you're calling should not throw an exception itself. If an exception is thrown by the code inside, catch it there and handle it there. Your method itself then can return something like a boolean that you can use to determine whether the operation was successful or not and branch accordingly. Although, the actual error handling logic (email admin, etc.) is contained in your IErrorHandler abstraction, your app code still is working with that and has a dependency on that, which is unnecessary domain knowledge.

  3. When you catch exceptions, catch specific exceptions. You should know exactly what you're catching and why. Catching something as generic as Exception is generally a sign of lazy coding. You may not even know if an exception could be returned at all, but you're still using a performance-draining try..catch block. In some scenarios it might be appropriate to catch any possible exception, but then, you should always re-throw the exception. Swallowing every possible exception is a huge no-no. If you feel you cannot re-throw the exception, then you should be targeting a specific exception instead.

    try
{
    // do something
}
catch (Exception e)
{
    // log exception
    throw;
}

  4. Separate your error handling from your request processing. Your app simply needs to return a response for the request. It should not care about stuff like emailing your admins and such. You need some sort of record that something went wrong, understandably, but simple logging is sufficient for that, and much lighter-weight. If you want to email admins, you can set up a separate service to monitor your logs and gen emails when appropriate, which takes that process appropriately out of band.

Improve this answer
Sign up to request clarification or add additional context in comments.

Comments

1

If you want to catch exceptions and do some general things with them, like you're doing in your example, then your way is not the good way in ASP.NET :)

When you have multiple controllers and multiple actions inside each controller, you're going to have a lot of code duplication.

ASP.NET Core allows you to create a Filter where you can put all that kind of logic in. It means that you'll have only one place to handle uncaught exceptions. You should definetely have a look at the possibilities that are provided with IExceptionFilter

Improve this answer

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.