Issue a redirect, trying several string replacements for the domain

Question

How could I improve this code?

var to = "http://forum.";
if (!RedirectPermanent("http://www.", to))
 if (!RedirectPermanent("http://blog.", to))
 if (!RedirectPermanent("http://forum.", to))
 if (!RedirectPermanent("http://tracker.", to))
 if (!RedirectPermanent("http://wiki.", to))
 RedirectPermanent("http://", to);

This is the method:

private bool RedirectPermanent(string from,string to)
{
 if (Request.Url.AbsoluteUri.StartsWith(from))
 {
 var domain = Request.Url.GetLeftPart(UriPartial.Authority).Replace(from, to);
 Response.RedirectPermanent(string.Concat(domain, Request.RawUrl));
 return true;
 }
 return false;
}

Answer 1

Went for this:

var arr = new[] { "http://www.", "http://blog.", "http://forum.", "http://tracker.", "http://wiki.", "http://" };
var to = "http://forum.";
foreach (var from in arr)
{
 if (RedirectPermanent(from, to))
 break;
}

• Jeff Mercado: Slight criticism, use better variable names. Expand it to redirectSources, redirectTo and redirectFrom. And in general, don't use contextual keywords as variable names such as from, even though safe, you should treat it like any other keywords and avoid it all together.

Answer 2

Review

• That level of nesting hurts readability.
• There is a lack of re-usability for a common set of source urls to redirect.
• You have specified an ultimate fallback of http://. But what if we encounter https:// or ftp://?

Alternative

As suggested by others, you want to loop a collection of input values and break on first match. The way I would do it is to provide a utility method. Use a HashSet to avoid duplicates.

private bool RedirectPermanent(HashSet<string> redirectSources, string redirectTo)
{
 return redirectSources.Any(x => RedirectPermanent(x, redirectTo));
}

Answer 3

As a variation:

var arr = new[] { "http://www.", "http://blog.", "http://forum.", "http://tracker.", "http://wiki.", "http://" }; 
var to = "http://forum.";
int i = 0;
while ((!RedirectPermanent(from[i], to)) && i < arr.Length)
{
 ++i; // reversed from i++
 if (i > arr.length) { break; }
}

There are several reasons I prefer this over a foreach:

• foreach has a performance cost to execute. An internal state machine iterator has to be instantiated. The while with a count limiter essentially does the same thing. While this is a performance tweak, IMO, it's still a better practice.

• code is not restricted/limited by the locked state of the iterated collection or the state machine iterator.

• IMO, using foreach with a body of if-then-break smells and reads like slang rather than proper grammar. ;)

• Pieter Witvoet: This may have been different 8 years ago, but nowadays a foreach loop over an array produces IL bytecode that is quite similar to that for a for or while loop - in this case a foreach loop is even slightly faster. More importantly, it's less error-prone: your loop fails with an IndexOutOfRangeException if none of the redirects is successful.
• Pieter Witvoet: A foreach loop does use an enumerator for other collection types, and that might be a bit slower. On the other hand, it works with any type that has an appropriate GetEnumerator method, it's easy to get right, and the intent is clear. The restrictions you mention are not inherent to foreach, they're an intentional safety mechanism in the enumerators of most BCL collection types.
• IAbstract: Fair enough; I was only somewhat aware that optimizations had occurred in compiling foreach (and other loop/enumerator ops). As far as restrictions go, regardless of the purpose - safety, whatever - they are still restrictions and I agree with the need for safeguards. Added check for 'out of range'.
• Pieter Witvoet: Your new out-of-range check is also broken...