views:

276

answers:

7

Hi all,

In a project I'm currently working on, we've added a wrapper class for accessing the HttpSessionState object. The trouble is that the current solution means you have to write some code to wrap the functionality. I came up with the following solution

/// <typeparam name="TKey">Class used for generating key into session state storage.</typeparam>
/// <typeparam name="T">Type of object to store.</typeparam>
public static class SessionManager<TKey, T>
{
 static SessionManager()
 {
  _key = typeof(TKey).ToString();
 }

 private static readonly string _key;
 public static string Key
 {
  get { return _key; }
 }

     // Other functions ... (Set, IsSet, Remove, etc.)

}

Now you can create the desired storage by merely using

using StringStore= Test.SessionManager<System.Boolean, System.String>;
using StringStore2= Test.SessionManager<System.Version, System.String>;

StringStore.Set("I'm here");
StringStore2.Set("I'm also here");

The code works and is nice in that you can easily create the wrapper class (single using statement) and everything is static. The code is however abusing the type system a bit, so maybe it is a bit to obscure? Before I added it I wanted to get some feedback, so here's the question:

If you we're maintaining said system and encountered the code above, would you

  1. Hunt down and kill whoever checked the file in?
  2. Be a bit annoyed by the attempt to be clever, but let it slide?
  3. Think it was a nice way of avoiding boilerplate code?

Would you prefer to use a text generation tool] like T4?

Thanks for any replies,

Mads

A: 

Code's fine. Congratulate yourself. It's easy for clients to use, as long as the internals are tested, and the code has defined behavior, what's the problem?

tpdi
A: 

If the use of System.Boolean versus System.Version is merely to distinguish different types to get separate instances of _key into the system, my response would be somewhere between #1 and #2. At least comment it and create some dummy types (maybe just empty interfaces) to use instead of using arbitrary .NET types.

I say this as someone whose primary job has been code review for the past 6+ months. If you're working with a lot of other people who need to understand that code, it's going to be difficult to understand and maintain if you don't at least give the reader some clue.

BlueMonkMN
A: 

@BlueMonkMN

I would never use System.Version or System.Boolean ... these were simply examples. In the project I would use existing classes, that actually access the stored info.

A: 

@BlueMonkMN

Just a thought: Would it be more readable/maintainable if I used a marker interface for TKey? Ie. something like

public interface SessionManagerKey { }
That depends what the alternative is. Can you give an example of an actual class name that you might use for TKey if not an interface?
BlueMonkMN
+1  A: 

So, you're using generics to create a key for a dictionary. I'd say this is definitely not a good idea for a couple different reasons.

The first reason is that it violates the reasoning behind a dictionary key. A key should hold some significance to the value it holds. Why are you storing a string under System.Boolean? What does System.Boolean mean? Does it mean the string is either true or false? Confusion like this makes the code harder to support. I'm also suspicious that the key value is used in casting the string somewhere else in code. This is clearly a mis-mixing of dictionaries and generics.

The second reason is that the Session is shared within a user's session. So two completely different segments of code written by two different developers are accessing this shared location. What is to stop one developer from identifying System.Boolean as an appropriate key-type for data A in their code, and another using System.Boolean as a key-type for data B in their code? Now the first developer is expecting A when accessing this bucket, but gets B. A meaningful, unique key would prevent this from happening.

Will
A: 

From comment by BlueMonkMN

That depends what the alternative is. Can you give an example of an actual class name that you might use for TKey if not an interface?

@BlueMonkMN

The thought was something along the lines of

class User : SessionManagerKey {
     ...
}

In the end I decided against the above solution, as it was probably, as several of the comments mention, to obscure to be maintainable.

Thanks for the comments.

A: 

hunt down. and burn.

what is the goal of this code beside adding dependencies to types names?