current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 10 down vote favorite
6

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.

share|improve this question

1 Answer 1

up vote 5 down vote

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);
}
share|improve this answer
    
Awesome post. Thank you –  Anil Singh Mar 12 at 10:19

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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