trawlr / Those pesky indexers!

Password

Remember me:

Those pesky string indexers.

I've worked on projects in the past where Microsoft has given developers more than enough rope to hang themselves, the rest of the team and any chance at easily maintaining the application. One common piece of rope is when they give developers a generic dictionary object to cram information into. ViewState, SessionState and recently in ASP.NET MVC, ViewData.

Giving developers these bucket-like objects is simply inviting people to make stuff up as they go. They get used as catch-alls, with people making up their own key-names ad hoc and rarely checking to see if someone else has used it. I've worked on several applications before where multiple developers decide to store the same value in something like SessionState with key names that are slightly different (userID vs userId, for example). This makes things hard to maintain as bugs creep up in a system and developers struggle to track them down. In the end, it is the fault of lazy or inobservant developers for doing it, but I lay some of the blame at a framework that makes it all too easy to do.

If things were strongly typed, it'd be easier. You'd be able to track code issues by examining uses of your interface. New "keys" would be introduced as properties to your interface, informing consumers as to what properties are already in use in the system, promoting better self-documentation. The tendancy of developers to use these objects as a catch-all for whatever piece of information they find convenient to cram inside would drop dramatically; It's harder to justify extending an interface for something that will be used once in an application than it is just to tuck it into one of these objects.

What I'm suggesting here isn't revolutionary at all but I'm constantly surprised at the number of shops that let this sort of behaviour go on.

Recently, I've been working a lot with the ASP.NET MVC framework. I love it, it's a very cool tool. But again you have a chance here to hang yourself if you're not careful by interaction with the form values and the viewdatadictionary. This has come up a handful of times with the team I'm working with, where typos have caused bugs to creep up, or people aren't aware of what information is being put into the viewdatadictionary simply because it's not very self-documenting.

Here's an example of how I'm handling this. It could be better, but it works for now.

public ViewResult AddUser()
{
IAddUserView view = new AddUserView(this.ViewData, this.FormData);
var model = new UserModel(this.UserRepository);

       User user = model.CreateUser(view.UserName);

       user.IsAuthorized = view.IsAuthorized;
       user.Email = view.EmailAddress;
       user.FirstName = view.FirstName;
       user.LastName = view.LastName;

       model.Save(user);

       return View("UserList");
}

public class AddUserView : ViewWrapper, IAddUserView
{
      public AddUserView(ViewDataDictionary viewData, NameValueCollection formData) : base("AddUser", viewData, formData) { }

      public string UserName
      {
           get { return GetData("UserName"); }
           set { this.ViewData["UserName"] = value; }
      }

      public string FirstName
      {
          get { return GetData("FirstName"); }
          set { this.ViewData["FirstName"] = value; }
      }      public string LastName
      {
          get { return GetData("LastName"); }
          set { this.ViewData["LastName"] = value; }
      }

      public string EmailAddress
      {
          get { return GetData("EmailAddress"); }
          set { this.ViewData["EmailAddress"] = value; }
      }

      public bool IsAuthorized
      {
         get
         {
              bool? data = GetData<string, bool?>("IsAuthorized", formValue => formValue == "on", viewValue => viewValue ?? false);
              return (bool)data;
         }
         set { this.viewData["IsAuthorized"] = value; }
      }
}public class ViewWrapper
{

      private readonly string viewName;
      protected readonly ViewDataDictionary viewData;
      private readonly FormDataIndexer formData;

      public ViewWrapper(string viewName, ViewDataDictionary viewData, NameValueCollection formData)
      {
             this.viewName = viewName;
             this.viewData = viewData;
             this.formData = new FormDataIndexer(formData);
      }

      public static implicit operator ViewResult(ViewWrapper wrapper)
      {
            ViewResult result = new ViewResult();

             result.ViewName = wrapper.ViewName;
             result.ViewData = wrapper.ViewData;

             return result;
       }

       public string ViewName
       {
            get { return viewName; }
       }

       public ViewDataDictionary ViewData
       {
           get { return viewData; }
       }

       protected string GetData(string key)
       {
          return GetData<object, string>(key, formValue => formValue == null ? "" : formValue.ToString(), viewValue => viewValue);
       }

       protected TResult GetData<TInput, TResult>(string key, Func<TInput, TResult> formEvaluation)
       {
            if (formData.ContainsKey(key)) return formEvaluation.Invoke((TInput)formData[key]);

            return (TResult)viewData[key];
      }

      protected TResult GetData<TInput, TResult>(string key, Func<TInput, TResult> formEvaluation, Func<TResult, TResult> viewEvaluation)
      {
           if (formData.ContainsKey(key)) return formEvaluation.Invoke((TInput)formData[key]);

           return viewEvaluation.Invoke((TResult)viewData[key]);
      }
}

internal interface IIndexer
{
object this[string index] { get; }
bool ContainsKey(string key);
}

internal class FormDataIndexer : IIndexer
{
     private readonly NameValueCollection formData;

     public FormDataIndexer(NameValueCollection formData)
     {
          this.formData = formData;
     }

     public object this[string index]
     {
          get { return this.formData[index]; }
     }

     public bool ContainsKey(string key)
     {
          foreach(string collectionKey in this.formData.AllKeys)
          {
               if (collectionKey == key) return true;
          }

          return false;
     }
}

This example may be a little over the top, but I really do wish development shops would see more value in coding against interfaces instead of writing hard-to-maintain code against bucket objects like the above, or things like SessionState or ViewState. It doesn't take much effort to make a SessionWrapper and expose things as properties, and it would make life a lot easier for people trying to track down bugs.

Of course, just because you build it doesn't mean they can't ignore it and use the buckets anyway... but if that happens, then it's a different problem all together.

Those pesky indexers!

comments