Please stop with the excessive amount of 'var' usage

"Writing code isn't hard, reading code is."

Over the years a lot of tasty nice features have arrived in .NET and C# specifically. The addition of anonymous variables was a very welcome addition arriving at the same time as LINQ, to help create new "on the fly" objects from data being queried. As an example, if you only want parts of a table of Users selected, you'll be able to write something like

var userInfo = context.Set<User>().Select(u => new {u.UserId, u.UserName});

This is excellent! You now have userInfo as an enumerable anonymous object containing UserIds and UserNames, and in this EF example, it saves you from selecting all the data in the entire table. This is good, this is all working as intended.

Now, the way var works has to be generic enough to handle everything thrown at it, which means it will also work fine in cases where you don't use it to imply an anonymous type, as an example here, let's say I want all of the users, I'm allowed to write

var userInfo = context.Set<User>();

Despite userInfo now not being an anonymous object, it's actually a DbSet<User>. Regardless, this will let me iterate over the collection of users by looping through userInfo. As a comparison all of these following statements will give me this same option.

var u1 = context.Set<User>();
var u2 = context.Set<User>().Select(u => u);
var u3 = context.Set<User>().ToList();

All these vars can be used in a foreach loop in the exact same way, yet they are very different object types, something you either have to know or use a mouse-over to identify. (Bonus points if you're able to tell how many database-queries would actually be executed by these three lines alone, provided context is an EF-context.)

So, what's the big deal? It all works!

The issue here is that these are very simple examples of the issue I'm trying to explain. In the real world, there are infinitely more complex queries, especially when it comes to LINQ and other methods which may or may not have a descriptive method in terms of what it might be returning. Why would you intentionally obfuscate the return value of a method by replacing say IQueryable<IGrouping<int, User>> with a var? Sure, your intellisense in Visual Studio will let you know what you can and can't do - you can decipher the 10-line long LINQ query to try to work out what's going on - maybe you can even tell by the name of the variable, that is, if it hasn't gone through a refactor where the method changed, yet the variable remained the same. You really shouldn't have to do this, you should be able to just see it right there on the line what kind of object we're dealing with.

I've done a number of code reviews where a huge portion of it is skimming through code to look for crazy usages of certain objects, and with the addition and extensive usage of vars for everything, this work simply takes longer and gets more tedious. Instead of relying on what could've been written right there, I need to either guess/assume what a method is returning, or actually stop and check. If this is done in a version comparison as well, intellisense/tooltips might not be there to help either, making it even worse to deal with. The funniest part to me is watching an excessive amounts of comments being descriptive all over the code, yet, still obfuscating the types through using var.

One of the primary culprits of this trend is also what I hope can reverse it. Yes, I'm looking at you ReSharper.

A tool used by "most" professional developers on top of their Visual Studio IDE, their default behavior in terms of cleaning up the code is suggesting to change every variable declaration to var. In my mind, the person who made this decision has to be a jealous and vindictive JavaScript individual who saw his opportunity to attempt to inflict the same pain of unstructured declarations unto the C#-developer population that he deals with on a daily basis, or an old VB.NET-developer who longed for the days before VB-Strict was implemented solution-wide at his company.

In their article regarding this option, they cite "Improved Readability" as the reason why it's enabled, which, to me makes little sense. Yes, it's easier to read the word var than what could potentially a long type name, but it's not a matter of reading English here. We're reading code, which means getting the understanding of the code should take precedence over any syntactic sugar we might feel like adding.

Don't get me wrong, ReSharper is a very helpful tool for most developers, I just feel that they should've defaulted to being more descriptive. Note that their article also easily explains how to invert the process to remove all vars from your projects, so it's simple to make it help you towards a better and more descriptive tomorrow.

On the same note regarding ReSharper, I strongly suggest to not always go with their refactoring advice. The best example is for some of those cases when it says "This loop can be converted into a LINQ expression", you let ReSharper do its thing and end up with a LINQ expression which surely will awaken Cthulhu by the time any developer is able to work out what the in the actual hell is going on. Simple rule here should be, if you follow a ReSharper refactor suggestion and can't immediately work out what it does, or find the previous method to be a lot more readable, revert it. This will save you and your coworkers debugging time in the long run.

(I'm sure someone will bring up the fact that I've used var in the past in my articles, my "excuse" for that is simply that I was taking screenshots of my code, and it made everything easier to fit into the picture.)

Comments (1) -

  • You rang? Ph'nglui mglw'nafh Cthulhu R'lyeh wgah'nagl fhtagn!

Add comment

Loading