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:
1
[Authorize]<span class="kwrd">public</span> ActionResult ChangePassword(<span class="kwrd">string</span> currentPassword, <span class="kwrd">string</span> newPassword, <span class="kwrd">string</span> confirmPassword){ViewData[<span class="str">"Title"</span>] = <span class="str">"Change Password"</span>;ViewData[<span class="str">"PasswordLength"</span>] = Provider.MinRequiredPasswordLength;<span class="rem">// Non-POST requests should just display the ChangePassword form </span><span class="kwrd">if</span> (Request.HttpMethod != <span class="str">"POST"</span>){<span class="kwrd">return</span> View();}<span class="rem">// Basic parameter validation</span>List<<span class="kwrd">string</span>> errors = <span class="kwrd">new</span> List<<span class="kwrd">string</span>>();<span class="kwrd">if</span> (String.IsNullOrEmpty(currentPassword)){errors.Add(<span class="str">"You must specify a current password."</span>);}<span class="kwrd">if</span> (newPassword == <span class="kwrd">null</span> || newPassword.Length < Provider.MinRequiredPasswordLength){errors.Add(String.Format(CultureInfo.InvariantCulture, <span class="str">"You must specify a new password of {0} or more characters."</span>, Provider.MinRequiredPasswordLength));}<span class="kwrd">if</span> (!String.Equals(newPassword, confirmPassword, StringComparison.Ordinal)){errors.Add(<span class="str">"The new password and confirmation password do not match."</span>);}<span class="kwrd">if</span> (errors.Count == 0){<span class="rem">// Attempt to change password</span>MembershipUser currentUser = Provider.GetUser(User.Identity.Name, <span class="kwrd">true</span> <span class="rem">/* userIsOnline */</span>);<span class="kwrd">bool</span> changeSuccessful = <span class="kwrd">false</span>;<span class="kwrd">try</span>{changeSuccessful = currentUser.ChangePassword(currentPassword, newPassword);}<span class="kwrd">catch</span>{<span class="rem">// An exception is thrown if the new password does not meet the provider's requirements</span>}<span class="kwrd">if</span> (changeSuccessful){<span class="kwrd">return</span> RedirectToAction(<span class="str">"ChangePasswordSuccess"</span>);}<span class="kwrd">else</span>{errors.Add(<span class="str">"The current password is incorrect or the new password is invalid."</span>);}}<span class="rem">// If we got this far, something failed, redisplay form</span>ViewData[<span class="str">"errors"</span>] = errors;<span class="kwrd">return</span> 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.
1
[Authorize]<span class="kwrd">public</span> ActionResult ChangePassword(<span class="kwrd">string</span> currentPassword, <span class="kwrd">string</span> newPassword, <span class="kwrd">string</span> confirmPassword){ViewData[<span class="str">"Title"</span>] = <span class="str">"Change Password"</span>;ViewData[<span class="str">"PasswordLength"</span>] = Provider.MinRequiredPasswordLength;<span class="rem">// Non-POST requests should just display the ChangePassword form </span><span class="kwrd">if</span> (Request.HttpMethod != <span class="str">"POST"</span>){<span class="kwrd">return</span> View();}var errors = <span class="kwrd">new</span> List<<span class="kwrd">string</span>>();ValidatePassword(currentPassword, newPassword, confirmPassword, errors);<span class="kwrd">if</span> (errors.Count == 0){<span class="kwrd">const</span> <span class="kwrd">bool</span> isOnline = <span class="kwrd">true</span>;var currentUser = Provider.GetUser(User.Identity.Name, isOnline);<span class="kwrd">if</span>( PerformPasswordChange(currentUser, currentPassword, newPassword)){<span class="kwrd">return</span> RedirectToAction(<span class="str">"ChangePasswordSuccess"</span>);}errors.Add(<span class="str">"The current password is incorrect or the new password is invalid."</span>); }<span class="rem">// If we got this far, something failed, redisplay form</span>ViewData[<span class="str">"errors"</span>] = errors;<span class="kwrd">return</span> View();}<span class="kwrd">private</span> <span class="kwrd">bool</span> PerformPasswordChange(MembershipUser user, <span class="kwrd">string</span> currentPassword, <span class="kwrd">string</span> newPassword){<span class="kwrd">try</span>{user.ChangePassword(currentPassword, newPassword);<span class="kwrd">return</span> <span class="kwrd">true</span>;}<span class="kwrd">catch</span>{<span class="kwrd">return</span> <span class="kwrd">false</span>;}}<span class="kwrd">private</span> <span class="kwrd">void</span> ValidatePassword(<span class="kwrd">string</span> currentPassword, <span class="kwrd">string</span> newPassword, <span class="kwrd">string</span> confirmPassword, ICollection<<span class="kwrd">string</span>> errors){<span class="kwrd">if</span> (String.IsNullOrEmpty(currentPassword)){errors.Add(<span class="str">"You must specify a current password."</span>);}<span class="kwrd">if</span> (newPassword == <span class="kwrd">null</span> || newPassword.Length < Provider.MinRequiredPasswordLength){errors.Add(String.Format(CultureInfo.InvariantCulture,<span class="str">"You must specify a new password of {0} or more characters."</span>,Provider.MinRequiredPasswordLength));}<span class="kwrd">if</span> (!String.Equals(newPassword, confirmPassword, StringComparison.Ordinal)){errors.Add(<span class="str">"The new password and confirmation password do not match."</span>);}}
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.