6

I often find complex Linq queries that span over 10-15 lines in our code base.
I find such queries hard to understand and impossible to debug.
I've tried to split the query to smaller parts, but it's hard to do since usually inner parts depend on the outer scope variables. Think of nested loops in imperative code.
Is this an indication of my inproficiency with Linq? Are complex Linq queries justified?
Would imperative solution be better in such cases?
Below is a sample query...

 var xItems = (
            from responseData in filteredItems
            let parentLevelItems = responseData.Items.Where(adi => ids.Contains(adi.Id))
            select new
            {
                secondLevelData = 
                (from c in secondLevelLabels
                    join secondLevel in responseData.children on c
                    equals string.IsNullOrEmpty(secondLevel.Label) ? 
                                           Constants.BlankData : secondLevel.Label into coun
                    from cn in coun.DefaultIfEmpty()
                    select new
                    {
                        secondLevelLabel = c,
                        parentLevelItems = parentLevelItems,
                        dataItems = cn == null ? response : 
                                                 cn.Items.Where(adi => ids.Contains(adi.Id))
                    })
            });
  • Long and complex aren't really the same thing. A long linq statement is more like several loops in a row or a loop with several statements inside it. Whereas it's possible to make a linq statement devillishly complex in only a line or two – Ben Aaronson Jun 4 '15 at 23:44
  • @BenAaronson While you do have a point there, I find such cases to be rare. At least, in my practice. My question is about a complexity caused by multitude of operations on the result set(s) – DanielS Jun 5 '15 at 0:36
  • 1
    Not sure, but might be useful to have an example in the question – Ben Aaronson Jun 5 '15 at 0:39
8

Let's rewrite that without any LINQ or LINQ extensions:

var xItems = new List<object>();
foreach(var responseData in filteredItems)
{
    var parentLevelItems = new List<Item>();
    foreach(var adi in responseData.Items)
    {
        if(ids.Contains(adi.Id))
        {
            parentLevelItems.Add(adi);
        }
    }

    var coun = new Dictionary<Label, List<Data>>();
    foreach(var c in secondLevelLabels)
    {
        foreach(var secondLevel in responseData.Children)
        {

            if(c == string.IsNullOrEmpty(secondLevel.Label) ? 
                                           Constants.BlankData : secondLevel.Label)
            {
                if(!coun.ContainsKey(c))
                {
                   coun[c] = new List<Data>();
                }
                count[c].Add(secondLevel);
            }
        }
        var secondLevelData = new List<object>();
        foreach(var cn in coun.DefaultIfEmpty())
        {
            secondLevelData.Add(new
            {
                secondLevelLabel = c,
                parentLevelItems = parentLevelItems,
                dataItems = cn == null ? response : 
                                                 cn.Values.Where(adi => ids.Contains(adi.Id))
            });
        }
    }

    xItems.Add(new 
    {
        secondLevelData = secondLevelData;
    });
}

I took some liberties with making up type names, translating the group join, and I did in fact keep some simple LINQ extensions (Contains and DefaultIfEmpty) to avoid going too mad. Nonetheless, looking at the above, I think it's relatively clear that your root problem isn't the choice of LINQ over loops.


Actually, the right way to deal with this code is probably with the usual refactorings: rename and extract method.

Names range from terrible (cn,coun) to the pretty-bad (xItems, secondLevelItems). It's hard to say for sure without knowing the context of this code, but you should be able to find more expressive names for these. This is a lot of complex logic, it seems like what it applies to should be more specific than just an "item"!

As for extracting methods, you say:

I've tried to split the query to smaller parts, but it's hard to do since usually inner parts depend on the outer scope variables.

Well, this is where parameters come in handy! If loops are still more natural to you, you could always try translating from LINQ to loops, extracting out your methods, then translating the individual bits back to LINQ again.


More generally, you don't get any kind of automatic bonus points for using LINQ. There are two reasons it might be preferable:

  • It's more declarative than loop-writing, making your code more expressive by letting you write what you're trying to achieve rather than how you're doing it.
  • It may be easier to keep lazy evaluation.

The latter is very situation-specific, so let's focus on the former. That's really just a particular type of readability benefit. Usually it's enough, but I've definitely seen cases (often involving the Aggregate extension method) where the LINQ version is just plain harder to decipher than the loop version. In that case, go for the loops! It'd be crazy to pick the less readable option in the name of readability.

If you're worried that lack of experience with LINQ means your judgement may not be good on readability, there's really not too much you can do about that other than seek out the judgement of others and gain more experience. But if, as in your example, complexity is about the sheer length rather than too stuff much being packed into too few statements, there's a good chance that the choice of LINQ isn't ultimately what's causing the problem.

  • 1
    You've affirmed my suspicions about the code quality of this piece. And also pointed out a way to refactor it. Thank you for a detailed answer. – DanielS Jun 5 '15 at 11:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.