Scott Hanselman

And so, Dear Reader, I present to you twenty-first in a infinite number of posts of "The Weekly Source Code." I'm doubling up this week, but the ASP.NET MVC Source was released today and I wanted to share more thoughts. I would also encourage you to check out TWSC 17 on Community ASP.NET MVC code.

Read the Comments

When you're reading source, look for words like "TODO," "HACK," "REVIEW," etc, to find parts of the code that the writers are concerned about.

In the SelectBuilder.cs, there's a comment that says:

// TODO: Should these be HTML encoded or HTML attribute encoded? Need to review all helper methods that call this.
string thisText = HttpUtility.HtmlEncode(listData[key].ToString());
string thisValue = HttpUtility.HtmlEncode(key.ToString());

This is an interesting question. He's asking if they should use System.Web.HttpUtility.HtmlAttributeEncode or HtmlEncode. HTML Attribute Encoding encodes <, " and &.

In ViewUserControl.cs we see these:

public virtual void RenderView(ViewContext viewContext) {
       // TODO: Remove this hack. Without it, the browser appears to always load cached output
       viewContext.HttpContext.Response.Cache.SetExpires(DateTime.Now);
       ViewUserControlContainerPage containerPage = new ViewUserControlContainerPage(this);
       containerPage.RenderView(viewContext);
}

This is a tough one also. Chasing caching issues is a huge hassle and consumed at least 10% of my time when I was writing banking software. Even now it feels like there are subtle (and not-so-subtle) differences between IE and Firefox. Seems like Firefox really caches aggressively.

There's a few marked "REVIEW" like:

    // REVIEW: Should we make this public?
    internal interface IBuildManager {
        object CreateInstanceFromVirtualPath(string virtualPath, Type requiredBaseType);
        ICollection GetReferencedAssemblies();
    }

And this one, which is kind of funny. The property IsReusable in an HttpHandler indicates whether or not an instance has state and as such, should not be reused by ASP.NET property. If you write an HttpHandler and it has no state, just a ProcessRequest, you can "reuse" it which should result in a small perf gain.

protected virtual bool IsReusable {
   get {
     // REVIEW: What's this?
     return false;
   }
}

Here's one about overloads:

//REVIEW: Should we have an overload that takes Uri?
[SuppressMessage("Microsoft.Design", "CA1055:UriReturnValuesShouldNotBeStrings",
            Justification = "As the return value will used only for rendering, string return value is more appropriate.")]
[SuppressMessage("Microsoft.Design", "CA1054:UriParametersShouldNotBeStrings",
            Justification = "Needs to take same parameters as HttpUtility.UrlEncode()")]
[SuppressMessage("Microsoft.Performance", "CA1822:MarkMembersAsStatic",
            Justification = "For consistency, all helpers are instance methods.")]
  public string Encode(string url) {
            return HttpUtility.UrlEncode(url);
}

We've all written comments like these. The trick is to make sure you've included all your key words in Visual Studio so that all your comments will show up in the Task List and can be dealt with before you ship.

Check out SuppressMessage

Microsoft uses CodeAnalysis a lot and you should too. However, sometimes CodeAnalysis offers suggestions that are wrong or not really appropriate and you'll want to suppress those.

[System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Design", "CA1024:UsePropertiesWhereAppropriate",
           Justification = "There is already a ViewData property and it has a slightly different meaning.")]
        protected internal virtual void SetViewData(object viewData) {
            _viewData = viewData;
        }

Looking for references to SuppressMessage is a good way to find out where unwavering "purity" analytics fall down and pragmatism should win the day. That said, it never hurts to reevaluate these occasionally as opportunities for refactoring.

The most interesting aspect is the Justification attribute which is actual prose written by the developers. For example, this is the contents of GlobalSuppressions.cs:

[assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Design", "CA1033:InterfaceMethodsShouldBeCallableByChildTypes", Scope = "member", Target = "System.Web.Mvc.TempDataDictionary.#System.Collections.Generic.ICollection`1<system.collections.generic.keyvaluepair  `2>)",
    Justification = "There are no defined scenarios for wanting to derive from this class, but we don't want to prevent it either.")]
[assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Design", "CA1033:InterfaceMethodsShouldBeCallableByChildTypes", Scope = "member", Target = "System.Web.Mvc.TempDataDictionary.#System.Collections.Generic.ICollection`1<system.collections.generic.keyvaluepair  `2>[],System.Int32)",
    Justification = "There are no defined scenarios for wanting to derive from this class, but we don't want to prevent it either.")]
[assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Design", "CA1033:InterfaceMethodsShouldBeCallableByChildTypes", Scope = "member", Target = "System.Web.Mvc.TempDataDictionary.#System.Collections.Generic.ICollection`1<system.collections.generic.keyvaluepair `2>>.IsReadOnly",
    Justification = "There are no defined scenarios for wanting to derive from this class, but we don't want to prevent it either.")]

Here's a good example of a justification:

[System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Design", "CA1054:UriParametersShouldNotBeStrings", MessageId = "2#", Justification = "The return value is not a regular URL since it may contain ~/ ASP.NET-specific characters")]
public static string SubmitImage(this HtmlHelper helper, string htmlName, string imageRelativeUrl) {
      return SubmitImage(helper, htmlName, imageRelativeUrl, null);
}

Code analysis is warning that there's a string parameter with the name "Url", but the justification is valid: "The value is not a regular URL since it may contain ~/ ASP.NET-specific characters"

Look to Utils

As I've said before, whenever I start reading code, I look for things marked "Util." These tell us a few things. Things named Util show the "underbelly" of code and point out where things could either be better factored, either in the thing your reading, or in the larger Framework whatever your reading lives in.

In ASP.NET MVC's project there's a Util folder and a Pair.cs file, so let's check it out.

//------------------------------------------------------------------------------
//     Copyright (c) Microsoft Corporation.  All rights reserved.
//------------------------------------------------------------------------------

namespace System.Web.Util {
    using System;

    // Generic Pair class.  Overrides Equals() and GetHashCode(), so it can be used as a dictionary key.
    internal sealed class Pair {
        private readonly TFirst _first;
        private readonly TSecond _second;

        public Pair(TFirst first, TSecond second) {
            _first = first;
            _second = second;
        }

        public TFirst First {
            get {
                return _first;
            }
        }

        public TSecond Second {
            get {
                return _second;
            }
        }

        public override bool Equals(object obj) {
            if (obj == this) {
                return true;
            }

            Pair other = obj as Pair;
            return (other != null) &&
                (((other._first == null) && (_first == null)) ||
                    ((other._first != null) && other._first.Equals(_first))) &&
                (((other._second == null) && (_second == null)) ||
                    ((other._second != null) && other._second.Equals(_second)));
        }

        public override int GetHashCode() {
            int a = (_first == null) ? 0 : _first.GetHashCode();
            int b = (_second == null) ? 0 : _second.GetHashCode();
            return CombineHashCodes(a, b);
        }

        // Copied from ndp\fx\src\xsp\System\Web\Util\HashCodeCombiner.cs
        private static int CombineHashCodes(int h1, int h2) {
            return ((h1 << 5) + h1) ^ h2;
        }
    }
}

This is a simple but clever class that uses generics to make a Pair of any two types. The interesting part is the CombineHashCodes method that takes the hash codes from each object and combines them in a way that makes that pair's hashcode unique enough for use in a Hashtable later.

The Pair class is used to create a combined object inside the TempDataDictionary class like this:

private Pair<Dictionary<string , object>, HashSet<string>> _sessionData;

...where the Key is the actual TempData storage dictionary, and the value is the list of keys that were modified during one request so that they might survive to the next.

There's lot more to learn from reading this code, and it's going to be fun to watch it grow, change and improve!

About Scott

Scott Hanselman is a former professor, former Chief Architect in finance, now speaker, consultant, father, diabetic, and Microsoft employee. He is a failed stand-up comic, a cornrower, and a book author.

About   Newsletter

Hosting By