13
\$\begingroup\$

I'm pretty new to MVC. Below is the code that I have added to an existing MVC 5 application. The code is using log4net to log any run-time error/exception.

NOTE: I could override OnException() in MVC filter "HandleErrorAttribute" but I preferred to have this functionality in my custom Controller base class.

Can anyone please review and provide feedback?

BaseController.cs

public class BaseController: Controller
{
    private log4net.ILog logger;

    protected override void OnException(ExceptionContext filterContext)
    {
        //Log error
        logger = log4net.LogManager.GetLogger(filterContext.Controller.ToString());
        logger.Error(filterContext.Exception.Message, filterContext.Exception);

        //If the request is AJAX return JSON else redirect user to Error view.
        if (filterContext.HttpContext.Request.Headers["X-Requested-With"] == "XMLHttpRequest")
        {
            //Return JSON
            filterContext.Result = new JsonResult
            {
                    JsonRequestBehavior = JsonRequestBehavior.AllowGet,
                    Data = new { error = true, message = "Sorry, an error occurred while processing your request." }
            };
        }
        else
        {
            //Redirect user to error page
            filterContext.ExceptionHandled = true;
            filterContext.Result = this.RedirectToAction("Index", "Error");
        }
        base.OnException(filterContext);
    }
}

HomeController.cs

public class HomeController : BaseController
{
    ...
}

Global.asax.cs

public class MvcApplication : System.Web.HttpApplication
{
    private static readonly ILog log = LogManager.GetLogger(typeof(MvcApplication));

    protected void Application_Error(object sender, EventArgs e)
    {
        //Log exception
        Exception exception = Server.GetLastError();
        log.Error(exception);

        //Clear error from response stream
        Response.Clear();
        Server.ClearError();

        //Redirect user
        Context.Server.TransferRequest("~/Error/");
    }
    ...
}

For displaying errors, I have a basic ErrorController.cs with just Index() Action along with its associated Error/Index.cshtml.

Some blogs suggests different error pages for different type of errors (such as 404 error). Do I need to handle them separately?

Please share if you have any ideas to improve upon my code in terms of usability and logging details.

\$\endgroup\$
12
\$\begingroup\$

The OnException() methods does 2 things.

  1. It logs the exception
  2. It creates an return value

So let us refactor these two points to two separate methods.

private void LogException(ExceptionContext exceptionContext)
{
    logger = log4net.LogManager.GetLogger(exceptionContext.Controller.ToString());
    logger.Error(exceptionContext.Exception.Message, exceptionContext.Exception);
}

private const String XMLHttpRequest = "XMLHttpRequest";
private const String XRequestedWithHeadername= "X-Requested-With";
private const String JSONErrorMessage = "Sorry, an error occurred while processing your request.";
private void CreateExceptionContextResult(ExceptionContext exceptionContext)
{
    if (exceptionContext.HttpContext.Request.Headers[XRequestedWithHeadername] == XMLHttpRequest )
    {
        //Return JSON
        exceptionContext.Result = new JsonResult
        {
                JsonRequestBehavior = JsonRequestBehavior.AllowGet,
                Data = new { error = true, message = JSONErrorMessage }
        };
    }
    else
    {
        //Redirect user to error page
        exceptionContext.ExceptionHandled = true;
        exceptionContext.Result = this.RedirectToAction("Index", "Error");
    }
}

If we want to handle the different error types, we should create a new class which only keeps the values for actionName and controllerName.

public class ActionControllerName
{
     public String ActionName { get; private set; }
     public String ControllerName { get; private set; }
     public ActionControllerName(String actionName, String controllerName)
     {
         ActionName = actionName;
         ControllerName = controllerName;
     }
}  

Now we add a Dictionary and a method to fill this dictionary, which should be called in the constructor, or injected by an property.

private const int DefaultEntryKey = -1;
private Dictionary<int,ActionControllerName> redirectDictionary = new Dictionary<int,ActionControllerName>();
private void FillRedirectDictionary()
{
    redirectDictionary.Add(DefaultEntryKey , new ActionControllerName("Index", "Error"));
    // more to add
}  

Next we implement a method to get a RedirectToRouteResult passing the exception as parameter

private RedirectToRouteResult GetRedirect(Exception exception)
{
    int errorCode = DefaultEntryKey;
    HttpException httpException = exception as HttpException;
    if (httpException != null) 
    {
        errorCode = httpException.ErrorCode;
        if (!redirectDictionary.ContainsKey(errorCode))
        {
            errorCode = DefaultEntryKey;
        }
    }
    ActionControllerName acn = redirectDictionary[errorCode];
    return this.RedirectToAction(acn.ActionName , acn.ControllerName);
}

We go back to this else part

else
{
    //Redirect user to error page
    exceptionContext.ExceptionHandled = true;
    exceptionContext.Result = this.RedirectToAction("Index", "Error");
}  

and change it to

else
{
    //Redirect user to error page
    exceptionContext.ExceptionHandled = true;
    exceptionContext.Result = GetRedirect(exceptionContext.Exception);
}
\$\endgroup\$
  • \$\begingroup\$ @heslacher this creates a major problem that I'm having to deal with now: it just binds the original url to the error redirect url forever even after you get rid of the exception : stackoverflow.com/questions/40143304/… \$\endgroup\$ – Riz Oct 21 '16 at 18:44

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.