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:

    None Found

Tags:


19 Comments to “The Dark Side of LINQ”

  1. Alexey Romanov | August 6th, 2008 at 1:59 am

    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).

  2. ekampf | August 6th, 2008 at 3:59 am

    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))

  3. Damon Wilder Carr | August 6th, 2008 at 7:34 am

    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/

  4. ekampf | August 6th, 2008 at 10:50 am

    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

  5. Mike Borozdin | August 6th, 2008 at 12:56 pm

    “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.

  6. Dew Drop - August 6, 2008 | Alvin Ashcraft's Morning Dew | August 6th, 2008 at 3:20 pm

    [...] The Dark Side of LINQ (Eran Kampf) [...]

  7. Scott Allen | August 6th, 2008 at 5:32 pm

    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.

  8. Scott Allen | August 6th, 2008 at 5:34 pm

    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. :)

  9. http://coffeecokeandcode.blogspot.com/ | August 6th, 2008 at 6:27 pm

    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.

  10. http://blog.domaindotnet.com/ | August 6th, 2008 at 6:30 pm

    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?

  11. ryan baldwin | August 7th, 2008 at 2:15 am

    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).

  12. Links for August 6th 2008 | .Net | August 7th, 2008 at 5:19 pm

    [...] Querying with LINQ to Entities vs ObjectQuery in EF The Dark Side of LINQ Using Stored Procedures In [...]

  13. Ian Qvist | August 9th, 2008 at 3:11 am

    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):

    
                var pixelsQuery =
                    from y in Enumerable.Range(0, screenHeight)
                    let recenterY = -(y - (screenHeight / 2.0)) / (2.0 * screenHeight)
                    select from x in Enumerable.Range(0, screenWidth)
                           let recenterX = (x - (screenWidth / 2.0)) / (2.0 * screenWidth)
                           let point =
                               Vector.Norm(Vector.Plus(scene.Camera.Forward,
                                                       Vector.Plus(Vector.Times(recenterX, scene.Camera.Right),
                                                                   Vector.Times(recenterY, scene.Camera.Up))))
                           let ray = new Ray() { Start = scene.Camera.Pos, Dir = point }
                           let computeTraceRay = (Func<Func, Func>)
                            (f => traceRayArgs =>
                             (from isect in
                                  from thing in traceRayArgs.Scene.Things
                                  select thing.Intersect(traceRayArgs.Ray)
                              where isect != null
                              orderby isect.Dist
                              let d = isect.Ray.Dir
                              let pos = Vector.Plus(Vector.Times(isect.Dist, isect.Ray.Dir), isect.Ray.Start)
                              let normal = isect.Thing.Normal(pos)
                              let reflectDir = Vector.Minus(d, Vector.Times(2 * Vector.Dot(normal, d), normal))
                              let naturalColors =
                                  from light in traceRayArgs.Scene.Lights
                                  let ldis = Vector.Minus(light.Pos, pos)
                                  let livec = Vector.Norm(ldis)
                                  let testRay = new Ray() { Start = pos, Dir = livec }
                                  let testIsects = from inter in
                                                       from thing in traceRayArgs.Scene.Things
                                                       select thing.Intersect(testRay)
                                                   where inter != null
                                                   orderby inter.Dist
                                                   select inter
                                  let testIsect = testIsects.FirstOrDefault()
                                  let neatIsect = testIsect == null ? 0 : testIsect.Dist
                                  let isInShadow = !((neatIsect > Vector.Mag(ldis)) || (neatIsect == 0))
                                  where !isInShadow
                                  let illum = Vector.Dot(livec, normal)
                                  let lcolor = illum > 0 ? Color.Times(illum, light.Color) : Color.Make(0, 0, 0)
                                  let specular = Vector.Dot(livec, Vector.Norm(reflectDir))
                                  let scolor = specular > 0
                                                 ? Color.Times(Math.Pow(specular, isect.Thing.Surface.Roughness),
                                                               light.Color)
                                                 : Color.Make(0, 0, 0)
                                  select Color.Plus(Color.Times(isect.Thing.Surface.Diffuse(pos), lcolor),
                                                    Color.Times(isect.Thing.Surface.Specular(pos), scolor))
                              let reflectPos = Vector.Plus(pos, Vector.Times(.001, reflectDir))
                              let reflectColor = traceRayArgs.Depth >= MaxDepth
                                                  ? Color.Make(.5, .5, .5)
                                                  : Color.Times(isect.Thing.Surface.Reflect(reflectPos),
                                                                f(new TraceRayArgs(new Ray()
                                                                {
                                                                    Start = reflectPos,
                                                                    Dir = reflectDir
                                                                },
                                                                                   traceRayArgs.Scene,
                                                                                   traceRayArgs.Depth + 1)))
                              select naturalColors.Aggregate(reflectColor,
                                                             (color, natColor) => Color.Plus(color, natColor))
                             ).DefaultIfEmpty(Color.Background).First())
                           let traceRay = Y(computeTraceRay)
                           select new { X = x, Y = y, Color = traceRay(new TraceRayArgs(ray, scene, 0)) };
    
  14. ekampf | August 11th, 2008 at 12:10 pm

    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.

  15. A Year’s Worth of Popular Posts | DeveloperZen | January 1st, 2009 at 1:02 am

    [...] The Dark Side of LINQ [...]

  16. Echilon | February 27th, 2009 at 11:14 pm

    Syntactic sugar at it’s best!. Thanks for the example.

  17. Justin | September 8th, 2010 at 9:28 pm

    Your surrounding design is imperative slop, of course linq doesn’t fair well in this particular case.

  18. Eran Kampf | September 20th, 2010 at 1:56 am

    Justing, the code in this example was taken from production code…

    C# crazies think that change from foreach to LINQ is proper…

    All I’m saying is that there’s more chance you’ll end up writing hideous code when using LINQ than proper one…

  19. Amir | September 21st, 2010 at 3:02 pm

    Call me on skype amirjen or phone 216 5779342
    I might have an interesting project – Prof Poreh