May 01, 2009

Extract Method is your friend

Ok, so I’ve been writing about how I try to write code that reads more like a narrative, and how I enjoy reading code that expresses intent. I thought it was about time I give a small example. I came across the perfect opportunity while refactoring the BlogEngine.NET login.aspx.cs for my ‘BlogEngine Experiment’.

Here’s the original code (minus the documentation comments):
protected void Page_Load(object sender, EventArgs e)

{

if (Request.QueryString.ToString() == "logoff")

{

FormsAuthentication.SignOut();

if (Request.UrlReferrer != null && Request.UrlReferrer != Request.Url)

{

Response.Redirect(Request.UrlReferrer.ToString(), true);

}

else

{

Response.Redirect("login.aspx");

}

}

 

if (Page.User.Identity.IsAuthenticated)

{

changepassword1.Visible = true;

changepassword1.ContinueButtonClick += new EventHandler(changepassword1_ContinueButtonClick);

lsLogout.Visible = true;

Login1.Visible = false;

Page.Title = Resources.labels.changePassword;

}

else

{

Login1.LoggedIn += new EventHandler(Login1_LoggedIn);

Login1.FindControl("username").Focus();

}

}

 

void changepassword1_ContinueButtonClick(object sender, EventArgs e)

{

Response.Redirect(BlogEngine.Core.Utils.RelativeWebRoot, true);

}

 

void Login1_LoggedIn(object sender, EventArgs e)

{

if (!Roles.IsUserInRole(Login1.UserName, BlogEngine.Core.BlogSettings.Instance.AdministratorRole))

Response.Redirect(BlogEngine.Core.Utils.RelativeWebRoot, true);

}

Nothing shocking there, and it seems easy enough to understand.

On the other hand, there’s a lot of implementation detail in the Page_Load event. If you want to understand what happens when the page loads, you have to spend at least a second or two mentally parsing through each line, running through the code as if you’re the runtime, trying to keep it all in memory. In a small method like this, it’s not hard to do. But trying to do the same in a much larger method can get very difficult, if not impossible for mere mortals like me.

Additionally, I can’t show this code to a non-programming domain expert and get a quick reality check on the flow of things. They’ll get lost in the details. However, with a few small tweaks, the exact same functionality can be accomplished in a more expressive way.

The first step, as this post’s title implies, is to extract some of the implementation details into their own methods. Name the methods something expressing the intent, rather than the details. If done carefully, the code in the Page_Load() event can be much more easily read, and the intent understood with less effort on the part of the reader.

Here’s my slightly refactored example:
protected void Page_Load(object sender, EventArgs e)

{

if (UserIsLoggingOff())

{

FormsAuthentication.SignOut();

RedirectToProperPage();

}

 

if (UserIsAuthenticated())

{

DisplayChangePasswordControls();

}

else

{

DisplayLoginControls();

}

}

 

private bool UserIsLoggingOff()

{

return Request.QueryString.ToString() == "logoff";

}

 

private bool UserIsAuthenticated()

{

return Page.User.Identity.IsAuthenticated;

}

 

private void RedirectToProperPage()

{

if (Request.UrlReferrer != null && Request.UrlReferrer != Request.Url)

{

Response.Redirect(Request.UrlReferrer.ToString(), true);

}

else

{

Response.Redirect("login.aspx");

}

}

 

private void DisplayChangePasswordControls()

{

changepassword1.Visible = true;

changepassword1.ContinueButtonClick += new EventHandler(changepassword1_ContinueButtonClick);

lsLogout.Visible = true;

Login1.Visible = false;

Page.Title = Resources.labels.changePassword;

}

 

private void DisplayLoginControls()

{

Login1.LoggedIn += new EventHandler(Login1_LoggedIn);

Login1.FindControl("username").Focus();

}

 

void changepassword1_ContinueButtonClick(object sender, EventArgs e)

{

Response.Redirect(RoutingAgent.RelativeWebRoot, true);

}

 

void Login1_LoggedIn(object sender, EventArgs e)

{

if (!Roles.IsUserInRole(Login1.UserName, BlogSettings.Instance.AdministratorRole))

{

Response.Redirect(RoutingAgent.RelativeWebRoot, true);

}

}

The first thing you’ll notice (I hope) is how much simpler Page_Load() is to understand. The implementation details for each logical step have been extracted and what was left behind is more expressive of intent. You can almost read the code as a series of short sentences. I can show this to a domain expert, and they’ll be able to follow along and give feedback pretty easily.

The cost, of course, is the increase in the raw number of methods. Some might see that as a bad thing. I don’t. Each method is but a few lines long, easily understood in isolation and very limited in scope. If you need to see the implementation details of a particular step in the overall algorithm, it’s trivial to navigate directly to the method and inspect it, without having to wade past all of the other, possibly distracting, details.

I realize this is an almost trivial example, but multiply this type of refactoring a few hundred times. You’ll quickly see how much easier a codebase is to read and understand when the code itself is cleaner and more expressive.