2009

Volume 24 Number 0

Extreme ASP.NET Makeover - Separation of Concerns

By Kyle Baley, Donald Belcham, James Kovacs | 2009

Welcome back for the sixth installment of Extreme ASP.NET Makeover. In previous articles, we focused on client-side browser technologies:  XHTML, CSS, JavaScript, jQuery, and jQuery UI. The next few articles in the series will focus on the server-side code. How can we modify our C# code to improve the maintainability, extensibility, and flexibility of our codebase? This article, Part 6, will focus specifically on separation of concerns.

Gotta Keep ‘Em Separated

Separation of concerns is a concept that, when applied to software development, deals with creating distance between dissimilar aspects of your code. This may seem like a complicated statement, but we all have dealt with it in the past, even if we haven’t known it. You’ve probably heard that you shouldn’t have your data access code in your Web page. That is separation of concerns. The Web page shouldn’t directly know how to deal with database concerns like connection strings, command objects, and so on.

Why do we care? Well, separation of concerns is one of the concepts that allows us to work on a specific aspect of an application without having significant impact on other parts of the application. Think of it this way: Should I be able to work on the engine in my car without having to deal with the wheels? In our software, we shouldn’t have to deal with the database infrastructure when we’re working in aspx.cs (or aspx.vb) files.

If we aren’t paying attention to separation of concerns, we would start seeing some common issues while working on our codebases. We’ve all been in a situation where you try to change what appears to be a little thing and end up having to dive deep into the bowels of your application’s infrastructure. To take a quick example, say you wanted to change how you determine what the user is authorized to do on a page. In ScrewTurn, permissions are determined through a call to the AuthChecker singleton class. (We’ll discuss how to cure “singletonitis” in an upcoming article. For now, we’ll focus on separation of concerns.) To determine if the user has permission to download an attachment, for example, we make the following call:

AuthChecker.Instance.CheckActionForPage(currentPage, 
Actions.ForPages.DownloadAttachments,                                                                    
currentUsername, currentGroups)

If we look at this one line of code on its own, nothing immediately appears to be a problem. But let’s imagine that this call exists in many, or all, of the Web pages in the application. What we see then is that changing how you determine a user’s ability to download attachments will require you to modify each and every one of those Web pages. That is, there is a ripple effect of one change imposing the need to modify nonrelated code.

There is another issue at play here. Consider the amount of intimate detail about authorization that exists in the application. A good example is the Page_Load method in the Default.aspx.cs file.  In Page_Load, there are calls for security information for a given user, including the following:

  • Can the user view a page?
  • Can the user download attachments on a page?
  • Can the user set permissions?
  • Can the user administer a page?
  • Can the user view discussions on a page?
  • Can the user post to discussions on a page?
  • Can the user manage discussions on a page?
  • Can the user edit a page?

This is a lot of authorization for the Page_Load method to have to know about. Of course, the Page_Load method does need to retrieve and use this information. But our question is this: Does it need to know how to retrieve all of those pieces of information? And just as important, does it need to be done for each individual task that could be performed? Asked a different way, does Page_Load need to have a full understanding of how security is being handled? No, it doesn’t. Can it simply ask some other class to have all security information given to it? Yes, it can! Page_Load simply needs to get the permissions, and it shouldn’t know or care about the mechanics of how this is accomplished.

Warning: Code Maintenance Ahead

The ultimate goal when applying separation of concerns is maintainability. This being a brownfield application, ScrewTurn Wiki likely has areas that are hard to work with because classes are doing too much and have become fragile. The slightest change to one class has ripple effects throughout the code. By separating concerns, we can break apart the code into its individual pieces so that when you look at a class, you aren’t distracted by extra code that is not part of its responsibility.

This is a good segue into one of the ways you can start separating concerns in your classes. Like many things in our craft, knowing when separation of concerns has been broken is not easy to see. There is one simple thing you can ask yourself when you look at any piece of code:

                Does this code belong in this class?

Or put another way:

                Is this part of the class’s immediate responsibility?

The idea of responsibility is an important one. When we start thinking of separation of concerns and of classes having responsibility, we gravitate toward the Single Responsibility Principle:

                A class should have only one reason to change. – Robert C. Martin in Agile Software Development: Principles, Patterns, and Practices

Earlier, when we were discussing the contents of the Page_Load method in the Default.aspx.cs file, we proposed asking the question, “Does it need to know how to retrieve all of those pieces of information individually?” In our case, the answer should be no. The Page_Load method should know only that it needs to retrieve authorization information, not the gory details of how that information is retrieved.  Separating those two needs is at the root of determining separation of concerns.

There’s yet another good reason to try to separate concerns in your application. In a previous article, we mentioned user interface testing with WatiN. If you’ve tried setting up UI testing, you’ll quickly discover that it can become cumbersome to create this safety net. The tests are slow and often need to be reworked as the UI changes. By moving code out of the ASPX pages and into separate classes, we open up the testing arena and are able to write true unit tests, ones that test only a single class in isolation, without having to fire up a browser instance, Web server, and database. These tests are faster, less fragile, and can lead us to a place where the code in the ASPX pages is limited to interaction between the various UI widgets.

From Principles to Practice

Enough talk. Let’s take this and apply it to the ScrewTurn Wiki codebase. First, we’ll start with the Default.aspx page’s code behind file. To make things easy, we will start at the top and look at the contents of the Page_Load. This is a busy method and not uncommon for a brownfield application. In this method alone, we can see several responsibilities:

  • Redirect the page if necessary
  • Determine the user’s rights for the page
  • Set up various UI widgets
  • Display the contents of the requested wiki page

 

When looking at the code, the first thing that our attention was drawn to was the repetitive nature of the authorization code. As you can see in the code in Figure 1, there are a large number of calls to retrieve single permission values.

Figure 1 Default.aspx Page_Load Code

string currentUsername = SessionFacade.GetCurrentUsername();
string[] currentGroups = SessionFacade.GetCurrentGroupNames();
 
bool canView = AuthChecker.Instance.CheckActionForPage
(currentPage, Actions.ForPages.ReadPage, currentUsername, currentGroups);
bool canEdit = false;
bool canEditWithApproval = false;
Pages.CanEditPage
(currentPage, currentUsername, currentGroups, out canEdit, out canEditWithApproval);
if(canEditWithApproval && canEdit) canEditWithApproval = false;
bool canDownloadAttachments = AuthChecker.Instance.CheckActionForPage
(currentPage, Actions.ForPages.DownloadAttachments, currentUsername, currentGroups);
bool canSetPerms = AuthChecker.Instance.CheckActionForGlobals
(Actions.ForGlobals.ManagePermissions, currentUsername, currentGroups);
bool canAdmin = AuthChecker.Instance.CheckActionForPage
(currentPage, Actions.ForPages.ManagePage, currentUsername, currentGroups);
bool canViewDiscussion = AuthChecker.Instance.CheckActionForPage
(currentPage, Actions.ForPages.ReadDiscussion, currentUsername, currentGroups);
bool canPostDiscussion = AuthChecker.Instance.CheckActionForPage
(currentPage, Actions.ForPages.PostDiscussion, currentUsername, currentGroups);
bool canManageDiscussion = AuthChecker.Instance.CheckActionForPage
(currentPage, Actions.ForPages.ManageDiscussion, currentUsername, currentGroups);

Now, we certainly can’t look at this code and simply say, “Get rid of all that stuff”, because it is necessary for the page to properly function. But how permissions are retrieved, whether from the AuthChecker or Pages or even hard-coding, is the responsibility of some other class, not this one's responsibility. All Page_Load needs are the final values.

Since we have decided that the Page_Load no longer needs to have intimate knowledge of these details, we can refactor them out. Our first refactoring step is to create a class that is responsible solely for providing the authorization information to the Default.aspx.cs file. We’ll call this class AuthorizationServices. Instead of diving straight into the contents of AuthorizationServices, let’s look at how we want to use it in the Default.aspx.cs Page_Load method. What we want to do is eliminate all of the individual calls to the AuthChecker class and replace them with a single call that returns all of the same authorization information, but in a consolidated fashion. That is, we are looking for something like this:

AuthorizationServices authorizationServices = new AuthorizationServices();
CurrentCapabilitiesStatus currentUser = authorizationServices.RetrieveCurrentStatusFor(CurrentPage);

That single call to AuthorizationServices is what we want to determine what levels of permission are available for the current user. The results of making that method call are returned in a CurrentCapabilitiesStatus object. That object is quite simple and has only properties containing the different authorization values.

Now that we know what we want the usage of the code to look like, we can move on to creating those classes and filling them out. Since our refactoring step is to remove the detailed information about the authorization concern out of the Page_Load method, let’s start there. In this case, the simplest refactoring task is to cut and paste the existing AuthChecker calls in the Page_Load code into the RetrieveCurrentStatusFor method. To make the AuthChecker code compile and function, we will need to move some other code as well. This will include:

string currentUsername = SessionFacade.GetCurrentUsername();
string[] currentGroups = SessionFacade.GetCurrentGroupNames();
 
var canEdit = false;
var canEditWithApproval = false;
Pages.CanEditPage(currentPage, currentUsername, currentGroups, out canEdit, out canEditWithApproval);

Now that we have all of that code in the RetrieveCurrentStatusFor method, the final step is to create and populate the output object type of CurrentCapabilitiesStatus. As we mentioned before, this is a simple class that contains only properties in it. You may have noticed in an earlier code snippet that we assigned an instance of this object to a variable named “currentUser”. At the time, this may have seemed odd, but when we combine that with the property names, we see that the resulting code in Page_Load becomes more intention-revealing. Since we don’t know what the intended use of the properties are when we’re inside the CurrentCapabilitiesStatus class, let’s step back to the Page_Load method and look at how we intend to use them there.

Chances are, if you did a complete cut and paste from Page_Load to AuthorizationServices.RetrieveCurrentStatusFor, there will be noncompiling code in the Page_Load method now. This should be limited to the variables that used to be in the method, but are now being moved to the CurrentCapabilitiesStatus class as properties. These are the points of usage that we’re looking for. The first one that you will see is an "if" statement that checks to see if the current user cannot view the current page. Let’s change this to use our “currentUser” variable now.

if(!currentUser.CanView) {
    // body of method remains unchanged
}

This is where you see how using the name currentUser starts to come into play. When scanning this code, it should be clear exactly what we are doing in this method. We have decided to name the property “CanView”, since we are predominantly working with positive questions when using the CurrentCapabilitiesStatus variable type. If you were to go through the rest of the authorization information used, you would end up with properties for CanDownloadAttachments, CanSetPerms, CanAdmin, CanViewDiscussion, CanEditDiscussion, CanManageDiscussion, CanEdit, and CanEditWithApproval, all defined on the CurrentCapabilitiesStatus object.

Once you have those properties added to the CurrentCapabilitiesStatus object and being used in the Page_Load method where needed, we still have one remaining loose end to tie up. While we moved the authorization code from Page_Load to the RetrieveCurrentStatusFor method in AuthorizationServices, we didn’t actually create a CurrentCapabilitiesStatus object and populate it. Doing that property value assignment is the final step in this refactoring.

 

Let’s summarize what we’ve done. We removed code from Page_Load using cut and paste, and we created two new classes. It doesn’t seem like much as far as refactoring is concerned. But that’s because we aren’t actually finished with the task. In order to consider the work complete, we must now go to every page that made use of AuthChecker and do the same refactoring as we did here. It will be a lot of work, but the method we used to separate the authorization-specific code can be incrementally applied to other code behind files. Let’s take another look at the refactoring, but this time we’ll see how automated refactoring tools can assist with the changes.

 

The Big Picture

Once all of the uses of AuthChecker have been removed and AuthorizationServices is being used in its place, you will have made the codebase much simpler to work with. Consider now what is involved when we change how permissions are retrieved. Instead of having to find and change the many places that AuthChecker was originally being used (over 130, by our count), you can work only in the AuthorizationServices class. This single point of change shows that you are more likely to be separating the concerns in your codebase.

In the end, a few things have happened from when we first looked at the Default.aspx.cs Page_Load method. First, we have been able to clearly define an area in code for the authorization concern. When anything related to determining a user’s authorization arises, we know that it will be taken care of by the AuthorizationServices. No longer will we continually suffer the ripple effects of modifying or fixing the functionality related to this concern.

Second, we have reduced the number of lines in the Page_Load method, yet through the use of revealing variable and property names, we haven’t reduced the ability for developers to decipher the intentions of the code. This is quite important when doing this type of refactoring. You never want to leave code less understandable than when you found it. (And considering how quickly comments become obsolete, leaving a trail of them behind you to ease understanding is not an optimal approach.)  Instead, think about how you will use variables, properties, and methods in the code and name them so that they read and convey ideas easily.

Conclusion

The final achievement is that we have started a trend of reducing repetition in our codebase. This is a side effect of separating our concerns, but by implementing an isolated class whose sole responsibility is to determine user authorization for a given page, we are allowing ourselves to eliminate any repetitive code that is also doing this. A simple Find search for “AuthChecker” in the entire solution shows that many locations can benefit from using the new AuthorizationServices class.

Taking care of the authorization concern is just one of many refactoring tasks that are available in the Page_Load method of Default.aspx.cs. Others include the URL redirection, the URL builders, Content.GetPageContent, and Settings. If you look through the code provided with this article, you can see how we have refactored those concerns out of Page_Load so that it has one, and only one, reason to change: how we are displaying the contents of the page.