Scott Hanselman

Improving LINQ Code Smell with Explicit and Implicit Conversion Operators

August 31, '07 Comments [9] Posted in Learning .NET | LINQ | Microsoft | Programming | XML
Sponsored By

It's fairly easy to learn a language - programming language or human language - and get to the point where you can be understood. Your intent may not be crystal-clear, but at least it can be figured out but the computer/listener. However, it takes time and consultations with "native speakers" to get to be really fluent and start writing poetry.

ScottGu has a fine sample on how to make a Simple Web-Based RSS Reader with LINQ to XML. In it he's filling an ASP.NET ListView using RSS XML as the data source. He makes a query like this to populate a collection of posts.

var posts = from item in rssFeed.Descendants("item")
    select new
    {   
        Title = item.Element("title").Value,
        Published = DateTime.Parse(item.Element("pubDate").Value),
        NumComments = Convert.ToInt32(item.Element(slashNamespace + "comments").Value,
        Url = item.Element("link").Value,
        Tags = (from category in item.Elements("category")
                orderby category.Value
                select category.Value).ToList() 
}

Seems pretty straight forward. He's grabbing a number of things from the <item> tag in the RSS and putting them into an anonymous type. See how he's using "select new" rather than "select new Foo"? My code smell doesn't like the .Values, but I'm not that worried yet.

I added my own feed and ran his code and got a NullReferenceException on this line. Remember it's all one line, so that can be confusing. It was hard to tell which of these expressions was telling me something was null. I guessed though, that it was the line looking for <rss:comments> on the blog post. DasBlog, the engine I use, doesn't include a <comments> element when there's zero comments. Perhaps it's right, perhaps not, regardless, it doesn't include. So, for DasBlog, zero comments means no <comments> element. This code doesn't handle that because the call to item.Element(slashNamespace + "comments").Value is a NullReferenceException as item.Element(slashNamespace + "comments") is null.

So, how do I say "make NumComments = the value of <comments> unless it's null, then make it zero?" First, we tried this.

NumComments = Convert.ToInt32(item.Element(slashNamespace + "comments") != null ? item.Element(slashNamespace + "comments").Value : "0")

The use of ? :, as in <expression> ? true : false is very comment in early C#. Wordy, but it works. Of course, we're indexing twice into item.Element, once to see if it's null and again to get the value it if it's not.

This didn't smell right to me, but a few things smelled bad both here and before. I don't like seeing all the .Value properties. Also, the Convert.ToInt32 seemed dodgy. I kind of felt like I shouldn't have to do that, or that I should called XmlConvert, rather than Convert.

I also felt that I should be able to use the ?? operator, as in x = y ?? z, meaning make x = y if y's not null, else x = z. But that darned .Value property gets in the way.

A few newish C# things can make it cleaner (Thanks Anders).

Explicit vs. Implicit Conversions

Classes can use the explicit or implicit keywords to control how types are converted. You use implicit if you can guarantee that no data will be lost in the conversion and you use explicit if you can't guarantee that. Implicit conversions are like long foo = bar, where bar is an int. Ryan Olshan has some good examples like:

public static implicit operator int(MyClass myClass)
{
    return myClass.Value;
}

Then later, you'd do something like int x = someMyClassInstance and the conversion is implicit.

Back to the LINQ to XML RSS, example there's a whole pile (24, actually) of explicit conversion operators defined for the XElement class. Here's the one for int? (Nullable int):

public static explicit operator int?(XElement element)
{
    if (element == null)
    {
        return null;
    }
    return new int?(XmlConvert.ToInt32(element.Value));
}

See how it calls .Value for me? It does exactly what I want. Also, because it'll return null, now I can do this (changes in red):

var posts = from item in rssFeed.Descendants("item")
    select new 
    {   
        Title = (string)item.Element("title"),
        Published = (DateTime)item.Element("pubDate"),
        NumComments = (int?)item.Element(slashNamespace + "comments") ?? 0,
        Url = (string)item.Element("link"),
        Tags = (from category in item.Elements("category")
                orderby category.Value
                select category.Value).ToList()
    };

I changed a few calls to .Value to explicit string casts to emphasize the point since the explicit operator for string is just this. Same with DateTime where the call to Parse becomes a cast.

public static explicit operator string(XElement element)
{
    if (element == null)
    {
        return null;
    }
    return element.Value;
}

Now before you go off on how this can be used for evil. Yes, of course it can. However, there's a very unambiguous expectation presented with explicit and implicit. You only get to use them as a class designer if you can promise they will work as expected.

In this case, I think it makes the code much cleaner. Here's the before and after again:

BEFORE:

var posts = from item in rssFeed.Descendants("item")
    select new
    {   
        Title = item.Element("title").Value,
        Published = DateTime.Parse(item.Element("pubDate").Value),
        NumComments = Convert.ToInt32(item.Element(slashNamespace + "comments") != null ? item.Element(slashNamespace + "comments").Value : "0"),
        Url = item.Element("link").Value,
        Tags = (from category in item.Elements("category")
                orderby category.Value
                select category.Value).ToList()     
    };

AFTER:

var posts = from item in rssFeed.Descendants("item")
    select new 
    {   
        Title = (string)item.Element("title"),
        Published = (DateTime)item.Element("pubDate"),
        NumComments = (int?)item.Element(slashNamespace + "comments") ?? 0,
        Url = (string)item.Element("link"),
        Tags = (from category in item.Elements("category")
                orderby category.Value
                select category.Value).ToList()
    };

Now NumComments will handle missing <comments> elements and things look tidier. Very cool. Thanks ScottGu and Anders for their help, direct and indirect.

It's going to take a while but a new, more refined sense of smell is in my future, I think. One doesn't have to necessarily remember that ?? and conversion operators exist, but rather to have a better "sense" when reading/writing code that there ought to be a cleaner way and then go looking for it.

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 ORCS Web
Friday, August 31, 2007 1:32:21 PM UTC
Why not take advantage of extension methods to elimintate casts (kind of Ruby style)

for ex create a convert class: (this one comes from top of my head)


public static class XConvert {
public static string AsString(this XElement element) {
return element.Value;
}
public static int AsInt(this XElement element) {
return Convert.ToInt32(element.Value);
}
public static int? AsNullableInt(this XElement element) {
return (element.Value == null) ? null : Convert.ToInt32(element.Value);
}
public static DateTime AsDateTime(this XElement element) {
return Convert.ToDateTime(element.Value);
}
.....
}


and the querry should become something like :

var posts = from item in rssFeed.Descendants("item")
select new
{
Title = item.Element("title").AsString(),
Published = item.Element("pubDate").AsDateTime(),
NumComments = item.Element(slashNamespace + "comments").AsNullableInt(),
Url = item.Element("link").AsString(),
Tags = (from category in item.Elements("category")
orderby category.Value
select category.Value).ToList()
};


Sory if I made some mistakes, I have no C# 3.5 compiler available to test this.
Pop Catalin
Friday, August 31, 2007 2:16:02 PM UTC
NumComments = item.Element(slashNamespace + "comments").AsNullableInt(),

That will still throw a NullReferenceException if item.Element(slashNamespace + "comments") is null, so you haven't solved Scott's original problem.
Grant Wagner
Friday, August 31, 2007 2:40:02 PM UTC
Grant, you are right, but that can be fixed:

public static int? AsNullableInt(this XElement element) {
return (element == null || element.Value == null) ? null : Convert.ToInt32(element.Value);
}

Like I said my applologies for posting untested code snipet but I'm not gonna get near a C# 3.5 compiler till week-end so I can't test it. Anyway a bug like that one woudn't had passed the first the first unit test run.
Anyway I had this ideea since I first saw extension methods, because I never liked the way you had to work with the XML object model.
Pop Catalin
Friday, August 31, 2007 4:16:31 PM UTC
Very nice example. I think that there are a lot of things in 3.5 that will make it easier for developers to more closely express their intent without having to move to a dynamic language like Ruby. A great example of implicit and explicit casting. Thanks.
Friday, August 31, 2007 5:53:50 PM UTC
Microsoft making huge efforts to allow ppl write sql right in pages =)
some name
Saturday, September 01, 2007 2:10:52 PM UTC
This is very nice. ..
Tuesday, September 04, 2007 4:29:57 AM UTC
I am really starting to think the new C# is the new C++. ;) There are so many possible points of failure in that one statement....
Aaron
Wednesday, October 10, 2007 6:56:13 AM UTC
Pop Catalin,
I may be wrong here, I also haven't got a 3.5 compiler to try this but how can you call an extension method on item.Element(slashNamespace + "comments") when it is null?

I may be wrong here and am not sure, i'm just asking..

-Rob.
robert
Saturday, October 13, 2007 9:34:50 PM UTC
One thing C++ taught me was that casts are evil and must (well _should_) be avoided at all times. And here we have something that is safe but looks exactly like a cast. No, sorry, I'll stick to the Convert.* functions I think. :)

When will C# get the NOT NULL qualifier for declarations? I needs it.


PES
Comments are closed.

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