Note: this post is republished from my old blog, which I need to import into this new blog.

I recently worked on an ASP.NET project where the original developer had made quite a lot of use of the session. There was lots of code like this:

if not Session("userid") is nothing then ...

if not Session("userid") = "" then ...

if Session("customertype") = 2 then
discountedPrice = price - (price * Session("discount")/100)
end if

The project was reasonably small and I wasn't hired to clean code. However, there's a number of problems with this code that make life difficult, so I wanted to improve things a little.

Don't Repeat Yourself
Firstly, we've got some business logic repeated all over the site. The developer is using the

if Session("userid") = "" then ...

to check to see if the user is logged in. He's also used different variations of this rule on different pages, such as

if Session("userid") is Nothing then ...

This is messy, inconsistant, and in violation of the great Don't Repeat Yourself (DRY) principle. A better way is this:

if not SessionHelper.IsUserLoggedIn then ....

To make this work you need to make a class called SessionHelper that contains static methods for all the logic you need.

'''

''' Creates a handy wrapper for accessing session variables in a clear way.
'''

Public Class SessionHelper
public shared readonly property get IsLoggedIn as Boolean
get
if Session("userid") is nothing
return false
else if Session("userid") = String.Empty
return false
end if
end get
end property
End Class

I think that it's much clearer, and if we want to change our "IsLoggedIn" rule, we only have to do it in one place.

You may be wandering why the developer didn't use .NETs Forms Authentication for this application, rather than the session. I'm not quite sure why to be honest!

Purpose of Code is Not Clear

Consider this line:

if Session("customertype") = 2 then ...

We've got a couple of problems here. The first is a magic number - 2. Any new developer to the project will have to dig around to find out what "2" means, which wastes his or her time. A cleaner way is this:

Dim TRADECUSTOMER as long = 2
if Session("customertype") = TRADECUSTOMER then ...

You could even define it as a const somewhere so that it's available to all pages and code-behinds.

Note that in the Refactoring book (Martin Fowler), this refactoring is called "Introduce Explaining Variable". In case you've not come across refactorings, they're basically little rules you can use to clean up smelly code. See www.refactoring.com

An further improvement to that might be this

if Session("customertype") = CustomerTypes.TradeCustomer then ...

Here an Enum is used to represent customer types:

Public Enum CustomerTypes
UnknownCustomer = 0
RetailCustomer = 1
TradeCustomer = 2
End Enum

Using the SessionHelper, we could do one final improvement which is this.

if SessionHelper.CustomerType = CustomerTypes.TradeCustomer then ...

which is clearer still.

You may have guessed that I subscribe to the belief that code is something that should usually be expressive and clear. I've not come across many projects where the code doesn't require modifying and changing over time, therefore clarity can drastically affect how easy it is to change.

Think of it this way, every time you write some code you can chose how much time and money it should cost to maintain. If the block of code is going to be thrown away soon, then it may make sense not to invest too much time making it maintainable. However, if you anticipate returning to a block of code a lot then it would save you much time to make it highly maintainable. Also, once you practice writing cleaner code it tends to come more naturally (that said, I still make the odd dogs dinner despite best intentions!)

Some other thoughts
One thing I considered when working on this project was weather to stop storing so much stuff in the session. The session is great because it's easy to access, but there are a couple of downfalls.

The longer some data is in the session, the more stale it gets. If some back office staff update the customer record whilst the customer is browsing the site, the customer won't see these changes until he next logs in. Not the end of the world, but I'd be inclined to work around these problems by default.

One such work around would be to only load the customer data from the database when it's needed. It means more database hits, but that's better than some of the hazards that can arise by having stale data sat around. The principle of caching also states that you should cached data to get cheap access to data that requires frequent access. If we needed to show a username on every page, then caching it in the session may make more sense than pulling it out of the database on every page.