Improving LINQ Code Smell with Explicit and Implicit Conversion Operators
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.
About Newsletter
