HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Storing an integer counter in indexer

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
storingintegerindexercounter

Problem

I've been implementing a custom internal server error page in ASP.Net MVC which will check if the current user is either an administrator or accessing the page from localhost, and if so, show them a whole bunch of details about the error to debug it with, otherwise just send them to a basic HTML error page.

So far, it works great, but one problem I had was that if there is an error in a partial view on the page, the system gets stuck in a loop trying to report the error.

To avoid this, I'm storing a temporary counter of how many times the current action has requested the error page in TempData, but I find the amount of lines and style of the code to get, set and check this variable a bit verbose:

using System.Web.Mvc;

namespace Test
{
    public class ErrorController : Controller
    {
        [ActionName("500")]
        public ActionResult InternalServerError(string aspxerrorpath = null)
        {
            int detectRedirectLoop = (TempData.Peek("redirectLoop") as int?) ?? 0;
            TempData["redirectLoop"] = detectRedirectLoop + 1;
            if((int) TempData.Peek("redirectLoop") <= 1)
            {
                // Check if user is admin or running locally and display error if so
            }
            return Redirect("/GeneralError.htm");
        }
    }
}


Is there a better/prettier/shorter way of doing this?

Solution

I suggest the following:

public class ErrorController : Controller
{
    private const string RedirectLoopCounterName = "RedirectLoopCounter";
    private const int MaxRedirectLoopCount = 1;

    private int RedirectLoopCounter
    {
        get { return ((int?)TempData.Peek(RedirectLoopCounterName)) ?? 0; }
        set { TempData[RedirectLoopCounterName] = value; }
    }

    private int IncreaseRedirectLoopCounter() 
    {
        return ++RedirectLoopCounter;
    }

    [ActionName("500")]
    public ActionResult InternalServerError(string aspxerrorpath = null)
    {       
        var isRedirectLoop = IncreaseRedirectLoopCounter() > MaxRedirectLoopCount;
        if(isRedirectLoop)
        {
            return Redirect("/GeneralError.htm");
        }

        // Check if user is admin or running locally and display error if so
    }
}


  • Create a constant for the counter's name as already mentioned by @BCdotWEB



  • Create a property for getting and setting its value



  • Create a method for actually increasing the counters value



  • Additionaly you could replace the 1 by a constant



  • Finally you can replace the condition by a helper variable to document it better



  • I would also invert the if

Code Snippets

public class ErrorController : Controller
{
    private const string RedirectLoopCounterName = "RedirectLoopCounter";
    private const int MaxRedirectLoopCount = 1;

    private int RedirectLoopCounter
    {
        get { return ((int?)TempData.Peek(RedirectLoopCounterName)) ?? 0; }
        set { TempData[RedirectLoopCounterName] = value; }
    }

    private int IncreaseRedirectLoopCounter() 
    {
        return ++RedirectLoopCounter;
    }

    [ActionName("500")]
    public ActionResult InternalServerError(string aspxerrorpath = null)
    {       
        var isRedirectLoop = IncreaseRedirectLoopCounter() > MaxRedirectLoopCount;
        if(isRedirectLoop)
        {
            return Redirect("/GeneralError.htm");
        }

        // Check if user is admin or running locally and display error if so
    }
}

Context

StackExchange Code Review Q#102510, answer score: 7

Revisions (0)

No revisions yet.