The Dark Side of LINQ
.NET August 5th, 2008
I’ve been having mixed feeling for quite some time now regarding LINQ.
Sure it can make working with data sources a lot easier and it can definately save a lot of code…
But, what happens with the following C# foreach statement
List<KeyValuePair<string, string>> resultList = new List<KeyValuePair<string, string>>(); string[] paramsArray = parameters.Split(new char[] { '&' }, StringSplitOptions.RemoveEmptyEntries); foreach (string p in paramsArray) { int index = p.IndexOf('='); if (index > 0) { string key = p.Substring(0, index); string value = p.Substring(index + 1); resultList.Add(new KeyValuePair<string, string>(key, value)); } } IEnumerable<KeyValuePair<string, string>> result = resultList.Distinct((p1, p2) => p1.Key == p2.Key);
Turns to this query:
var distinctPairs = (from keyValuePair in parameters.Split(new char[] { '&' }, StringSplitOptions.RemoveEmptyEntries) let index = keyValuePair.IndexOf('=') where index != -1 let key = keyValuePair.Substring(0, index) where !string.IsNullOrEmpty(key) let valueText = keyValuePair.Substring(index + 1) select new { Key = key, ValueText = valueText }) .Distinct( (p1, p2) => (p1.Key == p2.Key) ) .ToArray();
I don’t know about you but I find the first version a lot more approachable, readable and quicker to understand. The same code in LINQ is not shorter and looks simply looks Evil.
LINQ is like the force… It can be used to wonderful code that is simple and functional, but it also has the potential of producing cryptic code that’s hard to maintain.
Use it wisely and don’t be tempted for its dark side…
Similar Posts:
- [Code snippet] Obtaining Active Directory User Information Using System.DirectoryServices
- [Code Snippet] SyncStringDictionary - A synchronized StringDictionary
- LINQ\C# Learning Guide
- Introduction to LINQ
- Getting the Full Name of the Current User
Tags: LINQ
Is there any reason why ‘index > 0′ in the first snippet turned into ‘index != -1′ in the second one?
Actually, I do not think the LINQ version is particularly worse and find them about equally readable. (Except I’d write an overload for Distinct and write Distinct(p => p.Key) instead in both cases).
its (index > 0) because if index ==0 then the key is empty and the code skips these values (in LINQ we check where !string.IsNullOrEmpty(key))
Perhaps now you see why there is no \’direct\’ Linq support for \’for each\’ processing..
Remember it\’s declarative, not imperative or you are likely solving the wrong problem (perhaps)..
Here is how I recommend this type of problem be solved using Linq only where it adds value and the far more expressive (in this case) use of extension methods.
As always first the test to prove this works:
[TestFixture]
public class Class2
{
[Test]
public void InvokedMethod()
{
var parameters = \”firstname=d&lastname=c\”;
var splitString = parameters.ToSplitQueryString();
var result = splitString.ToQueryStringURLSequence().Distinct();
Assert.That(result.Count() == 2);
Assert.That(result.First().Value.Equals(\”d\”));
Assert.That(result.Last().Value.Equals(\”c\”));
}
}
OK now the extension method class:
public static class URLEncodingExtensions
{
private static readonly char[] _splitArray;
private const char _eq = \’=\’;
private const char _amp = \’&\’;
static URLEncodingExtensions() {
_splitArray = new[] { _amp };
}
///
/// Split a query string into a formatted array
///
/// The target.
///
public static IEnumerable ToSplitQueryString(this String target)
{
return target.Split(_splitArray, StringSplitOptions.RemoveEmptyEntries);
}
///
/// Leverages any input which is IEnumerable (string[] is one)
///
/// The target.
///
public static IEnumerable<KeyValuePair> ToQueryStringURLSequence(this IEnumerable target)
{
foreach (var p in target)
{
var index = p.IndexOf(_eq);
if (index == -1)
continue;
yield return ExtractKVP(p, index);
}
}
///
/// Refactored method for readability
/// KVP=KeyValuePair
///
/// The p.
/// The index.
///
private static KeyValuePair ExtractKVP(string p, int index) {
return new KeyValuePair(p.Substring(0, index), p.Substring(index + 1));
}
}
Not much more work but now reusable in this domain and a solid use. Perhaps you could call ToList() or ToArray() if you want to not use the Enumerable calls to first() and last() but whatever…
By focusing on \’how can I write the solution for others to then reuse\’ as the next mandate beyond simple \’TDD/Agile/Verifiable Regressions\’ your post in my opinion provides an opportunity to not ask \’why linq\’ but \’think differently\’.. No?
The second \’query\’ is not really a query at all. It\’s an attempt to force imperative logic where it was perhaps never intended. For myself a sure way to see this if when the LET statement is being used as it is in your second code snippet to force an algorithm to be defined instead of declaring the result you are after.
Kind Regards,
Damon Wilder Carr
http://blog.domaindotnet.com/
Hi Damon,
That’s definitely a solution I would favor over both code snippets in the post.
“Think differently” is definitely a lesson here.
We tend to take new language features and in all sort of cool way and sometimes we just forget the basic goal of producing maintainable code (reusable, testable, etc.) - especially when it comes to readability.
Just because LINQ is there and it works doesn’t mean its the right solution…
Regards,
Eran
“Use it wisely” - is a main point. LINQ isn’t a silver bullet. Sometimes it saves your time, sometimes it doesn’t. It is just a tool, not a religion you should follow.
[...] The Dark Side of LINQ (Eran Kampf) [...]
var result =
parameters.Split(’&’)
.Select(s => s.Split(’='))
.Where(a => a.Length == 2)
.ToDictionary(a => a[0], a => a[1]);
I find that easier to read than the comprehension query and foreach loop. The declarative syntax is often good for queries but has the tendancy to obscure algorithms. The lambda syntax looks like a processing pipeline to me, and I just need to read each step of the processing instead of following the iterative, nested logic of a loop. That being said, some solutions are better off inside a foreach.
Just realized that code doesn’t remove dup keys.
var result =
parameters
.Split(’&’)
.Select(s => s.Split(’='))
.Where(a => a.Length == 2)
.Select(a => new { Key = a[0], Value = a[1] })
.Distinct()
.ToDictionary(kv => kv.Key, kv => kv.Value);
Beauty is in the eye of the beholder.
Any language feature can be misused. An even worse example is the LINQ ray tracer, cool, but not really very readable:
http://tirania.org/blog/archive/2007/Nov-16.html
In short, LINQ did not replace common sense.
Beauty is in the eye of the people trying to maintain your code after you\’ve moved on the greener pastures.. (grin)..
This is not about anyone who posted but I often find myself saying:
1) Most software engineers are either working a trade or continuing their mastery of a craft.
2) Although many details could describe the characteristics with many acronyms and buzzwords, it often comes down to this:
Craftsmen now more then ever have internal dialogs with real or \’hypothetical\’ consumers of their work around \’usability of the API\’ and how to make it a joy to leverage.
Why has code-reuse failed? Many reasons beyond this.. But most write code for the short-term here and now rather then act as craftsmen offering a \’potential\’ of quality, usability, documentation, verifiability and \’trustworthiness\’ for others to consider their work.
When push comes to shove any slight notion of these ideas are thrown out in favor of the short-term deadline, fine for trivial non-strategic work but a deal-breaker for scalability of teams/organization capacity in software.
Perhaps just as importantly, there is another concurrent thread of deep concern around the ability to verify with trivial effort over time and dramatic change the work produced. Today this is almost always mock driven regressions on a C.I. build server in real-time. There is no reason not to do this, as it has no impact on how people work. It\’s an island of critical knowledge for those who want it. The only change is if you break the build you\’ll hear about it (not a bad thing, but I routinely get push-back against transparency items such as this).
Thoughts?
Is the linq approach something that linq was even intended for? I know that linq can split and parse stuff, but my question is why would you want to use linq for that?
Typically I only use linq to select and find stuff - I almost never use it for \”post-processing\” (for lack of a better term).
[...] Querying with LINQ to Entities vs ObjectQuery in EF The Dark Side of LINQ Using Stored Procedures In [...]
I once glanced over the PLINQ examples provided with the Microsoft Parallel Extensions package. It almost gave me a heart attack. One example they wrote a ray tracer in LINQ. It’s 150 lines of pure LINQ (pure evil if you ask me).
This code is directly from the example (Sorry about this):
Ryan,
It’s hard to put LINQ to the “context it was intended for” apparently its hard to define one.
It can be used for querying the DB, for parsing, for parallel processing etc. and you can’t really say that any of these is right or wrong.
The problem I see is how do we make sure we take advantage of new platform’s advantages without gradually degrading from C# readable code into cryptic SQL-like statements…
It should take some discipline not to be tempted to abuse it…
Eran.