CodeSOD: Magical Thinking

I have a very casual interest in magic tricks. I certainly don't have the patience to actually learn to perform any well, but I enjoy watching them and piecing together how they're done. What really captures my interest is that actually performing a trick is about following a well-defined path, and by following that path you can achieve incredibly impressive effects. But if you just see the effect without understanding the path, and try and replicate it yourself, it's probably not going to work.

In programming, there are certain things I consider "frickin' magic". They can create impressive effects, but the actual mechanism is concealed behind enough layers of abstraction that, if you don't stick to the carefully defined path, you can make some mistakes.

Object-Relational Mappers and .NET's LINQ features leap to mind as peak examples of this. We have a lot of LINQ related WTFs on the site.

Today's is a simple one, which highlights how, when one attempts to replicate the magic without understanding it, problems arise.

var vendor = connection.Query<Vendor>(get, new { id = id });var vendorlist = vendor.ToList();vendor.First().Contacts = this.GetContactById(vendorlist[0].Id);vendor.First().Products = this.GetVendorProducts(id);return vendor.Any() ? vendor.First() : null;

Here, we're using Dapper as our ORM. The goal here is to get one vendor, based on ID. Since we know ID is unique, since we know this should only ever return one value, at most, there's a method already extant for doing that. QueryFirstOrDefault will run a query, returning either the first item in the result set or null.

But we don't do that. We just fetch a result set that will, at most, contain one item, and then convert it into a list.

Which brings us to this line:

vendor.First().Contacts = this.GetContactById(vendorlist[0].Id);

What bothers me about this is that we have two totally different ways of fetching the first item in a list- by calling .First() or by indexing [0]. I don't really care which one you do, but please be consistent, at least for the duration of a single line.

Also, it's almost certainly true that GetContactById and GetVendorProducts are running additional queries, because while you can definitely use your ORM to fetch related entities all in one query and thus improve performance, you know this code isn't doing that.

But all this brings us to the cherry on top, because this line, right here, highlights a lot of misunderstandings about the magic:

return vendor.Any() ? vendor.First() : null;

.Any() returns true if there are any elements. If there are, we can return vendor.First(), because if there aren't calling vendor.First() would raise an exception. So it's good that we do a null check, but it's a pity we didn't do it before we did the previous two lines.

"Better late than never" isn't that much better.

[Advertisement] Otter - Provision your servers automatically without ever needing to log-in to a command prompt. Get started today!