Head.SmackOnTable();

Often, when working with someone else’s code, it works, but it isnt easy to maintain and read. Basically its ugly, and sometimes its like an ugly bag of snakes. Take this example:

Note: This code has been changed to protect the guilty.

[HandleError]
 public ActionResult LogOn(string username, string password, bool rememberMe)
 {
 string failureText = _websiteService.Login(username, password, rememberMe);
 if (!string.IsNullOrEmpty(failureText))
 {
 TempData["LoginFailure"] = failureText;
 return RedirectToAction("CheckoutAddress");
 }
 else
 Response.Redirect(Url.Action("CheckoutAddress", "Orders"));

 return View();
 }

Now, to me, this rings alarm bells, for multiple reasons. First off, you have failureText which claims to login, but actually gets you if it failed or not. Next thing you have is the “If there is no failure text, then it has worked”. Thirdly, you have the defunct return view, even though it shouldnt really be there.

How should this code be structured I hear you cry? Well, something like this:

[HandleError]
public ActionResult LogOn(string username, string password, bool rememberMe)
{
 if (AreCredentialsValid(username, password) && IsUserAllowedToLogin(username))
 {
 SetAuthenticationCookie(username,password);
 return RedirectToAction("CheckoutAddress");
 }
 else
 {
 ShowInvalidCredentialsPanel();
 return RedirectToAction("CheckoutAddress");
 }
}

Much more easier to read, much easier to maintain and modify, and no having to look for magic strings and just assuming someone can login because there wasnt an error.