CodeSOD: A Managed Session

Some time ago, Roald started an internship on a ASP .Net application. It didn't take long to find some "special" code.

 public string RetrieveSessionString(string sessionName){try{return Session[sessionName].ToString();}catch (NullReferenceException){return null;}}

The Session variable is a session object for this user session. Each request carries a token which allows us to pair a Session with a user, making a cross-request per-user global object. That is what it is- but it's weird that we call the parameter sessionName. Maybe that's just a bad parameter name- it might be better called sessionKey or something like that.

Of course, the real issue here is it's null handling. Calling ToString on a key that doesn't exist throws a NullReferenceException, so we handle it just to return a null, thus making future NullReferenceExceptions somebody else's problem. Arguably, an empty string would be a better behavior. Still, I hate it.

But Roald also found this function's evil twin:

public Dictionary<string, string> RetrieveSessionDictionary(string sessionName){try{return (Dictionary<string, string>)Session[sessionName];}catch (NullReferenceException){return null;}}

This is the same function, but instead of fetching a string, it fetches a dictionary of string/string pairs. It does the same null handling, but notably, doesn't do any error handling for situations where the cast fails.

And suddenly, this makes more sense. They're using the word "session" in two different contexts. There's the Session- a series of HTTP requests sharing the same token- and there's a user's session- settings which represent a unit of work. They're storing a dictionary representing a session in the Session object.

Which leaves this code feeling just... gross. It makes sense, and aside from the awful null handling, I understand why it works this way. It's just awkward and uncomfortable and annoying. I dislike it.

Also, functions which are name RetrieveBlahAsType are generally an antipattern. Either there should be some generics, or type conversions should be left to the caller- RetrieveSession(sessionName).ToString() is clearer with its intent than RetrieveSessionString(sessionName). Maybe that's just my hot take- I just hate it when functions return something converted away from its canonical representation; I can do that myself, thank you.

[Advertisement] ProGet's got you covered with security and access controls on your NuGet feeds. Learn more.