6
\$\begingroup\$

Yesterday someone guided me through this article and I'm wondering whether I should use try-catch implementation or not? I have the following code below which is actually a RESTful service and I have implemented the try-catch pattern. I'm throwing errors only in debug mode just for testing purpose and during the release mode, I'm catching errors and returning the meaningful error messages such as Http Status Codes.

After reading this article, I'm confused whether I'm on the right track and if not, do I need to remove try-catch and throw errors instead of such meaningful messages to the end user?

Any code review/feedback is appreciated.

namespace NewEmployeeBuddy.Facade.Controllers.Employee
{
    [RoutePrefix("api/employee")]
    [RequireHttps]
    [BasicAuthorization]
    public class EmployeeController : ApiController
    {
        #region Properties
        private IEmployeeService _employeeService { get; set; }
        #endregion

        #region Constructor
        public EmployeeController()
        {
          //_employeeService = new EmployeeService();
        }

        public EmployeeController(IEmployeeService employeeService)
        {
            _employeeService = employeeService;
        }
        #endregion

        #region Actions
        [HttpPost, Route("add")]
        [ValidateRequest]
        public IHttpActionResult AddEmployee(AddEmployeeRequestDTO request)
        {
            try
            {
                if (request == null)
                    return Content(HttpStatusCode.BadRequest, ErrorCodes.E002);

                var response = _employeeService.AddEmployee(request);
                if (response.OperationResult)
                    return Content(HttpStatusCode.Created, response);

                return Content(HttpStatusCode.InternalServerError, ErrorCodes.E001);
            }
            catch (Exception ex)
            {
#if (DEBUG)
                Logger.Instance.Error(ex);
                throw ex;

#endif
                //Logger.Instance.Error(ex);
                //return Content(HttpStatusCode.InternalServerError, ErrorCodes.E001);
            }
        }
        #endregion
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ //No need to create the concrete instance. Autofac takes care of it. - this is not true if you use this constructor. \$\endgroup\$ – t3chb0t Jun 29 '18 at 9:04
  • \$\begingroup\$ @t3chb0t - This is just a comment for my own. I know the actual usage. Although I should remove it to avoid confusion. \$\endgroup\$ – iSahilSharma Jun 29 '18 at 9:05
  • \$\begingroup\$ The default constructor is pointless anyway. What do you need it for in the first place? Have you removed any other parts of the code? \$\endgroup\$ – t3chb0t Jun 29 '18 at 9:07
  • \$\begingroup\$ Default constructor is before I implemented Autofac. This is the complete code. \$\endgroup\$ – iSahilSharma Jun 29 '18 at 9:09
12
\$\begingroup\$
throw ex;

This is a fatal mistake. It'll create a new stack-trace and you won't be able to tell where the actual exception occured. If you want to rethrow it then use just

throw;

without ex.

\$\endgroup\$
5
\$\begingroup\$

Typically in an ASP.NET app (and in fact in any app), you should only catch and handle the base System.Exception in a single place as close to the application entry point (or in this case request entry point) as possible. This means you don't have to duplicate exception handling code in every action method, and helps ensure you always return a consistent error response that is the same shape.

In .NET Core and OWIN based APIs this is typically done using a Middlware. With traditional ASP.NET APIs you can use an exception filter. The default MVC API template in VS2017 adds a System.Web.Mvc.FilterAttribute for you:

public class FilterConfig
{
    public static void RegisterGlobalFilters(GlobalFilterCollection filters)
    {
        filters.Add(new HandleErrorAttribute());
    }
}

You can replace this with your own implementation that for example logs the error using your chosen logging framework and forms the error response according to your API contract. Your controllers then do not need to worry about exception handling:

public class EmployeeController : ApiController
{
    private readonly IEmployeeService _employeeService;

    public EmployeeController(IEmployeeService employeeService)
    {
        _employeeService = employeeService;
    }

    [HttpPost, Route("add")]
    public IHttpActionResult Add(AddEmployeeRequest request)
    {
        var response = _employeeService.AddEmployee(request);

        if (!response.OperationResult)
        {
            // Can return content that matches what your error filter returns.
            return Content(HttpStatusCode.InternalServerError, new
            {
                Code = ErrorCodes.E002,
                Message = "Failed to add customer."
            });

            // Or could throw a custom exception that will be picked up in your error filter.
            throw new CustomerException("Failed to add customer.", ErrorCodes.E002);
        }

        return Ok(response);
    }
}
\$\endgroup\$
  • \$\begingroup\$ This is the kind of answer I was hoping to see one day ;-) \$\endgroup\$ – t3chb0t Jul 7 '18 at 14:06
  • \$\begingroup\$ It actually makes a lot of sense to implement a global exception filter instead of try catch everywhere. \$\endgroup\$ – iSahilSharma Jul 7 '18 at 14:21
3
\$\begingroup\$

The linked post adds a neat summary:

So, to sum up:

  • Don’t catch fatal exceptions; nothing you can do about them anyway, and trying to generally makes it worse.
  • Fix your code so that it never triggers a boneheaded exception – an "index out of range" exception should never happen in production code.
  • Avoid vexing exceptions whenever possible by calling the “Try” versions of those vexing methods that throw in non-exceptional circumstances. If you cannot avoid calling a vexing method, catch its vexing exceptions.
  • 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.

But then you conclude:

Yesterday someone guided me through this article and I'm wondering whether I should avoid try-catch completely ...

That's not in line with the summary.

or rarely use.

Depends on what you use with "rarely". If you mean to only use them when appropriate and avoid using them when not appropriate, then it's inherently good to do so (by definition of the word "appropriate").


Ask yourself why we avoid exceptions.

  • Throw/catch logic is expensive, performance-wise.
  • It can cause developers to use throw/catch logic as the intended flow of the application.
  • When implemented too broadly, it can swallow exceptions that you needed to be aware of.

Which brings me to your code:

try
{
     //...
}
catch (Exception ex)
{
#if (DEBUG)
    Logger.Instance.Error(ex);
    throw ex;

#endif
    //Logger.Instance.Error(ex);
    //return Content(HttpStatusCode.InternalServerError, ErrorCodes.E001);
}

I infer that you're trying to catch boneheaded exceptions (as per the linked post's definition). You shouldn't usually be catching these. They should blow up in your face.

However, there is a bit lacking in your implementation.

  • In debug, you log the exception and rethrow it. You're not handling it, you're basically intercepting an exception for logging purposes.
    • As t3chb0t mentioned, you'd be better off doing throw; instead of throw ex; so you conserve the stack trace, but that's a minor fix.
  • In release, you catch the exception but you don't throw it again. You're effectively swallowing the exception and not alerting anyone than a problem was encountered.

For debug, the performance argument of throw/catch logic is moot. Debug builds are not optimized for performance, they are optimized for easy debugging. You're also not using it for logical flow, since you're only interested in more verbose logging. It is implemented rather broadly, but that seems reasonable for debugging purposes.
All in all, I'm not opposed to your approach that is specific to debug builds.

Your approach in release is not good for a few reasons:

  • Don't swallow or hide exception. Instead, you should handle them gracefully; i.e. don't break the flow but rather intentionally pass an error message.
    • For the error messages, you should "dumb them down" for the end user. Instead of a nullreference exception message, simply report that "An error occurred on the server.
    • You could for example choose to show the actual exception message in debug mode (because you assume a developer is using the application then).
  • You're catching an exception for no reason. Catching costs performance, yet you gain nothing from doing so.

A more graceful solution would be:

try
{
    //...
}
catch (Exception ex)
{
#if (DEBUG)
    Logger.Instance.Error(ex);
    throw;
#endif

    throw new Exception("An error occurred on the server.");
}

This assumes that the frontend is built in a way that it gracefully displays the error message to the user.

This handles both debug (detailed logging and message) and release (simplified message to the end user).

Note that you could log the real exception and give the user a reference ID that they can use if they create a support ticket:

catch (Exception ex)
{
    int errorReferenceNumber = Logger.Instance.Error(ex);

    #if (DEBUG)        
        throw;
    #endif

    throw new Exception(
            $"An error with reference {errorReferenceNumber} occurred on the server. Contact the helpdesk for help.");
}

This way, a release build still logs the error, hides it from the user, but still gives them a reference number (which does not reveal information about the error to the user) so that the user can give it to a developer who can examine the exception.

\$\endgroup\$
  • 1
    \$\begingroup\$ According to this question (asked by me some time ago) - the exception message is not intended to be read by users. I also think there is a better way of handling exceptions in asp.net. the core has its middleware, the old one probably something similar so catching and throwing them in each and every action is a lot of boilerplate code. \$\endgroup\$ – t3chb0t Jun 29 '18 at 11:37
  • \$\begingroup\$ @t3chb0t I'm not quite following your comment. I specifically show code that hides the exception message from end users. It does seem fair to assumes that during debug, the "user" is actually the developer, hence not hiding the exception message. \$\endgroup\$ – Flater Jun 29 '18 at 11:52
  • 1
    \$\begingroup\$ You've just replaced it with another message, more generic one but still an exception message. \$\endgroup\$ – t3chb0t Jun 29 '18 at 11:53
  • \$\begingroup\$ @t3chb0t: It is still the message property of an exception, but that's not what was meant when we say "not showing an exception message to the user". You're applying a literal reading (using the Exception.Message property) for a much more figurative rule (avoiding blindly returning unfiltered message content, i.e. the value of the string). Note that in the linked answer you posted, the focus is on the content of the message, not the type of the object in which the message is contained. \$\endgroup\$ – Flater Jun 29 '18 at 11:58
  • \$\begingroup\$ @t3chb0t: I get your point but it's simply not what my answer focuses on. There are countless ways to gracefully handle an exception, including setting up a page redirect in case of exceptions. \$\endgroup\$ – Flater Jun 30 '18 at 21:35

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.