Controller is NOT the New Code-Behind

One of the things that I often say when touting the benefits of ASP.NET MVC is clean separation of concerns.  I talk about how the code-behind of WebForms is just begging you to put code there.  Samples online frequently open direct database connections and execute ad-hoc queries against the database from a button-click event.

Of course the same is possible with ASP.NET MVC.  Just taking a peak at the new Preview 4 installer, you’ll see a brand-spanking new AccountController, pre-built for managing logins/membership on your site.

Open AccountController.cs and you’ll find this method:

    [Authorize]
    public ActionResult ChangePassword(string currentPassword, string newPassword, string confirmPassword)
    {

      ViewData["Title"] = "Change Password";
      ViewData["PasswordLength"] = Provider.MinRequiredPasswordLength;

      // Non-POST requests should just display the ChangePassword form 
      if (Request.HttpMethod != "POST")
      {
        return View();
      }

      // Basic parameter validation
      List<string> errors = new List<string>();

      if (String.IsNullOrEmpty(currentPassword))
      {
        errors.Add("You must specify a current password.");
      }
      if (newPassword == null || newPassword.Length < Provider.MinRequiredPasswordLength)
      {
        errors.Add(String.Format(CultureInfo.InvariantCulture,
                 "You must specify a new password of {0} or more characters.",
                 Provider.MinRequiredPasswordLength));
      }
      if (!String.Equals(newPassword, confirmPassword, StringComparison.Ordinal))
      {
        errors.Add("The new password and confirmation password do not match.");
      }

      if (errors.Count == 0)
      {

        // Attempt to change password
        MembershipUser currentUser = Provider.GetUser(User.Identity.Name, true /* userIsOnline */);
        bool changeSuccessful = false;
        try
        {
          changeSuccessful = currentUser.ChangePassword(currentPassword, newPassword);
        }
        catch
        {
          // An exception is thrown if the new password does not meet the provider's requirements
        }

        if (changeSuccessful)
        {
          return RedirectToAction("ChangePasswordSuccess");
        }
        else
        {
          errors.Add("The current password is incorrect or the new password is invalid.");
        }
      }

      // If we got this far, something failed, redisplay form
      ViewData["errors"] = errors;
      return View();
    }

That’s nearly 70 lines in a single method.  Even with the comments it’s difficult to see what it’s doing at a glance.

This serves as a lesson to those of us that sell TDD as a design practice.  Sure this method is testable, but it seems as though the author did not take a few minutes to refactor code.  Normally I wouldn’t pick on someone for writing this, but when this is installed as a project template, it serves as an example for others to write code just like this.

Let’s see how we might clean this code up to make it more readable.

Here’s a good first attempt.  This took all of about 5 minutes to accomplish.

    [Authorize]
    public ActionResult ChangePassword(string currentPassword, string newPassword, string confirmPassword)
    {
        ViewData["Title"] = "Change Password";
        ViewData["PasswordLength"] = Provider.MinRequiredPasswordLength;        // Non-POST requests should just display the ChangePassword form 
        if (Request.HttpMethod != "POST")
        {
            return View();
        }
            
        var errors = new List<string>();
        ValidatePassword(currentPassword, newPassword, confirmPassword, errors);
        if (errors.Count == 0)
        {                
            const bool isOnline = true;
            var currentUser = Provider.GetUser(User.Identity.Name, isOnline);
                
            if( PerformPasswordChange(currentUser, currentPassword, newPassword))
            {
                return RedirectToAction("ChangePasswordSuccess");
            }
                
            errors.Add("The current password is incorrect or the new password is invalid.");               
        }

        // If we got this far, something failed, redisplay form
        ViewData["errors"] = errors;
        return View();
    }

    private bool PerformPasswordChange(MembershipUser user, string currentPassword, string newPassword)
    {
        try
        {
            user.ChangePassword(currentPassword, newPassword);
            return true;
        }
        catch
        {
            return false;
        }
    }

    private void ValidatePassword(string currentPassword, string newPassword, string confirmPassword, ICollection<string> errors)
    {
        if (String.IsNullOrEmpty(currentPassword))
        {
            errors.Add("You must specify a current password.");
        }

        if (newPassword == null || newPassword.Length < Provider.MinRequiredPasswordLength)
        {
            errors.Add(String.Format(CultureInfo.InvariantCulture,
                  "You must specify a new password of {0} or more characters.",
                  Provider.MinRequiredPasswordLength));
        }

        if (!String.Equals(newPassword, confirmPassword, StringComparison.Ordinal))
        {
            errors.Add("The new password and confirmation password do not match.");
        }
    }

This code (while still completely in the controller class) is broken apart into distinct methods.  This reduces the line count on the action method to 26, which is much easier to swallow.  I’m sure you see even more opportunities for improvement here.  Some of this code probably belongs in another, completely separate class (one that is even more easily tested that this).

My point is, always remember to include time for refactoring.  Especially when the result of your work will stand for thousands to learn from.