Those pesky indexers!

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()
{
       I
AddUserView 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.

Related Articles:

Post Footer automatically generated by Add Post Footer Plugin for wordpress.

This entry was posted in Uncategorized. Bookmark the permalink. Follow any comments here with the RSS feed for this post.
  • http://www.celikict.com tasarım

    Well you are actually right that is the FAULT of lazy programmers(including me and 90% of them). But code seems interesting

  • http://panteravb.com Chris C

    That seems like alot of work but I get where you’re coming from. Have you looked at using the ModelBinder stuff? I wrote a stupid little automagical wiring thingy here: http://panteravb.com/blog/posts/2008/10/2/imodelbinder-with-autobinder.ashx, it’s nothing great but saves me a lot of typing.

  • http://jimmybogard.lostechies.com Jimmy Bogard

    Believe it or not, I recently saw a large codebase where every method parameter was a NameValueCollection.

  • http://pwnagepants.lostechies.com mbratton

    @Chris C:

    Yeah I’ve looked at Binders… they look cool. The point of what I have here was supposed to be that you use one instance (in this case AddUserView) to either pull values from the form or send values to viewdata.

    Also, this was before we moved everything to preview 5. ;)

    In terms of actual work, like most classes once it’s set up it’s a simple matter of leveraging the functionality. Plus it simplified things for the developers I’ve been mentoring on ASP.NET MVC … just create a view, inherit from this class and then all access is pretty much the same. Get and Set. I find that repeatable patterns help people when they’re first learning something.

    I’ve no doubt that this could be better implemented, or there are better approaches, though, but it worked for us. :D

  • http://Bryan.ReynoldsLive.com Bryan Reynolds

    Great article. I agree that the ModelBinder is a potential solution in some cases.

  • http://blog.fohjin.com Mark Nijhof

    I like what you have done, it may be a little bit more work to create but definitely easier to maintain. And no more confusion about what certain fields are called.

  • http://www.dotnettricks.com Fregas

    I have to agree with Chris C is that this looks like a lot of work. I think you’re doing it the hard way. I don’t use the dictionaries at all. MVC ViewData and Views have a strongly typed option using generics, so thats what I use. Rather than making an “AddUserView” that wraps the dictionary, I would bind my view directly to one or more model/business objects. So for example, if I have an array of product objects I want to display, I would simply declare my viewpage like such:

    public partial class AddUser: ViewPage

    Then in my controller, I add the user to its view like so:

    this.ViewData.Model = _userRepository.GetUser(id);
    return View();

    If there is more then one object I need to pass, I do something similar and make a very small wrapper class. So if I had to pass a User and an array of Photos for example, I might already have a User object and a Photo object in my domain, and wrap them like this:

    public class AddUserModel
    {
    public User User {get;set;}
    public Photo[] Photos {get;set;}
    }

    then change my view to do this:

    public partial class AddUser: ViewPage

    similar to what you’re doing, but less work because I’m using existing business objects rather than making a wrapper around all the ViewData dictionary stuff.

  • http://pwnagepants.lostechies.com/ mbratton

    @Fregas:

    One the goals to this was to have a single interface to interact with the view page. In the approach of using a ModelBinder and a typed viewpage, I have two different methods of interacting with the view (depending on if I’m retrieving information or sending information).

    On top of that, I have to write additional classes for the ModelBinder and if I need to use more than one object to send to the view, I’m writing wrapper objects that exist only because I’m wanting to interact with the model in a way that isn’t natively supported. At that point I feel like I’d be losing the advantage of what you’re prosposing here, as there isn’t too much difference conceptually between writing a wrapper for multiple objects and what I’ve posted above.

    Not saying my way is better, but I don’t really think the level of effort involved is greater than the proposed alternative. Additionally, all code dealing with it is located in one place.

  • Steve Sheldon

    I see what you are doing, and I have to think about this a bit.

    I hate referencing anything with a hardcoded string. Configuration settings, dictionary, whatever. In such cases I’ll use constants, or wrap it kind of like you did. I do this with configurationmanager calls all the time.

  • Reshef Mann

    On a monorail project I did I used the Castle dictionary adapter http://www.castleproject.org/components/dictionaryadapter/basics.html
    to handle this exact issue.