Storing an integer counter in indexer

Question

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?

• BCdotWEB: One obvious problem: "redirectLoop" is repeated three times, and should obviously be a const.

Answer 1

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

• t3chb0t: oh, I was't sure about that but it shouldn't be a problem to change it ;-)
• t3chb0t: I removed the static keyword.

Answer 2

I have serious concerns about your use of TempData here: http://www.rachelappel.com/when-to-use-viewbag-viewdata-or-tempdata-in-asp.net-mvc-3-applications

My problem is it assumes your resource requests are single threaded (on a per-client basis), so if you have a client attempting to access more than one resource simultaneously I think your counter gets thrown off.

From my reading of that link I posted it wouldn't know if a subsequent request was in parallel or sequence to your initial 500.

• MrLore: Therefore, the only scenario where using TempData will reliably work is when you are redirecting that is happening in this case, as all uncaught exceptions redirect the user to error/500 (the webserver does this), and if the exception is also thrown on (part of) the error page, the user gets redirected again to error/500. Before I added the loop counter, this problem would cause a this page is not redirecting properly error in the browser after 30 or so redirects to the same page. +1 for giving me something to test though.