Scott Hanselman

The Weekly Source Code 21 - ASP.NET MVC Preview 2 Source Code

March 21, '08 Comments [9] Posted in ASP.NET | ASP.NET MVC | Learning .NET | Programming | Source Code
Sponsored By

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.

facebook twitter subscribe
About   Newsletter
Sponsored By
Hosting By
Dedicated Windows Server Hosting by SherWeb
Friday, 21 March 2008 21:20:30 UTC
I've written extensively about getting GetHashCode implementation right here and then getting things right with regard to better hash code values against integers here.
Friday, 21 March 2008 21:58:05 UTC
Generic pairs really should be in the basic framework, in a top-level namespace. I used KeyValuePairs for a while. If we could have tuples (or "named anonymous types") that would be even better, so the property names aren't just "First" and "Second." Ah well.

This post was a lot of fun though, thanks! I think I should browse through the comments in the general .NET Framework source some time; might be similarly fun.
Friday, 21 March 2008 22:19:44 UTC
It's too bad that you can't view task list comments for all files in a C# project. As I remember, you can in VB.NET projects. It would be cool to put the comments in a build report. Anyone know of any good tools for doing that?
Friday, 21 March 2008 22:37:55 UTC
I feel rather strange now. Just a few hours ago I was digging in the MVC source and the exact same things caught up my attention: the "What's this" comment, all those suppressions and even the Pair class. One more good thing is "These values seem arbitrary...".
Unknown
Saturday, 22 March 2008 01:10:48 UTC
ScottHa wrote:
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"
I'd go along with you, except for the fact that the string parameter being passed is not a URL at all: it's a relative path. URLs are very well defined and not the expectation of this method. I say Code Analysis is "more right"!

-MattL
Tuesday, 25 March 2008 22:36:35 UTC
This just shows how silly C# dev coding is becoming in comparison to properly generic counterparts such as Boost.

The difference is engineering principles between .NET and modern template metaprogramming is so high it isn't even worth discussing.

But I understand we all need to be productive, and dive into Ruby and DLR and Silverlight silliness..

Anders did a great job producing a toy language by adding more and more runtime hacks on top while getting the compiler to generate more and more bloat.

Not good, where this industry is going, remember that.
qtqwer
Wednesday, 26 March 2008 13:15:26 UTC
Hey Scott,

The class doesn't show as a generic class because the converter messed up when the C# was converted to HTML. If you check the source you'll see that the code reads internal sealed class Pair<tfirst tsecond ,> which doesn't render correctly in HTML.

Greetings,
Laurent
Thursday, 03 April 2008 02:24:39 UTC
Hi Scott,

I'm looking at MVC now and found it to be a really nice initiative from the ASP.NET team. But I still can't understand why the Controller class is inside the web namespace and why it is ASP.NET specific.

Isn't the Controller part of MVC supposed to be "view technology" agnostic? Isn't that the whole point of the MVC pattern?

When I used MVC in the past my main goal was to implement one controller logic and use that logic into multiple presentation technologies, like Winforms, Winforms inside .Net compact framework, WPF or ASP.NET. So the devs would code one thing and have it running on two different presentation views with the same presentation rules controlled by the view controller on the background.


The controller has its mind around ASP.NET, even the nomenclature.... Why RenderView instead of DisplayView or ShowView? Render sounds quite to ASPy :)


Just some thoughts... :) Great webcasts about MVC by the way!!!

Cheers,

Guil
Wednesday, 23 April 2008 07:15:05 UTC
Hi good morning
Comments are closed.

Disclaimer: The opinions expressed herein are my own personal opinions and do not represent my employer's view in any way.