Update grid from source hierarchy

Question

In this question I answered with this Linq. While it does what I was looking for, I am not sure how easy the linq queries are for others to follow. So I am looking for feedback on formating, what comments would be helpful and other alternative approaches to moving through the children records.

    private void UpdateGridFromSourceHierarchy(int depth, IEnumerable<SourceFile> list)
    {

        //Get the initial set of sourcefiles
        var sourceFiles = list
            .SelectMany(current => current.getInvocations()
                                .Select(invocation => new {current = (SourceFile) null, invocation}))
            .ToList() //This ensures that getInvocations is only called once for each sourcefile
            .GroupBy(x => x.current, x => x.invocation);

        for (var currentDepth = 0; currentDepth <= depth; currentDepth++)
        {
            foreach (var currentGroup in sourceFiles)
            {
                int sourceFileCount = currentGroup.Count();
                int counter = 0;

                foreach (var invocation in currentGroup)
                {
                    /*
                     * Generalized grid code goes here
                     * In my code it was a call to:
                     * UpdateGridPosition(currentGroup,invocation,counter);
                     */
                    counter++;
                }
            }

            //Select the current sub source files
            sourceFiles = sourceFiles.SelectMany(current => current.Select(invocation => invocation))
                //Get all of the invocations paired with the new current level of source files
                .SelectMany(newCurrent => newCurrent.getInvocations()
                                     .Select(invocation => new { newCurrent, invocation }))
                //Group them so that we can loop through each set of invocations seperately
                .ToList().GroupBy(x => x.newCurrent, x => x.invocation);
        }
    }

Answer 1

Leaving aside the problem of deeply nested code for now, I think readability is greatly improved by the use of LINQ syntax:

var sourceFiles = from file in list
                  from invocation in file.getInvocations()
                  group invocation by (SourceFile)null into groupedByInvoker
                  select groupedByInvoker;

for (var currentDepth = 0; currentDepth < depth; currentDepth++)
{
    foreach (var invokerGroup in sourceFiles)
    {
        int sourceFileCount = currentGroup.Count();
        int counter = 0;

        foreach (var invocation in invokerGroup) {
            // Do stuff
            counter++;
        }
    }

    sourceFiles = from invokerGroup in sourceFiles
                  from file in invokerGroup
                  from invocation in file.getInvocations()
                  group invocation by file into groupedByInvoker
                  select groupedByInvoker;
}

For me, this makes the SelectMany a lot easier to follow. There are some other tweaks to the queries too:

• You can just group by null directly, rather than projecting to an anonymous type and selecting that back out as the key.
• You don't need to convert to a list if you follow it by a groupby. The groupby traverses and stores the result of the enumerable anyway. You can test this by removing the ToList and adding a break point/debug output inside getInvocations.
• By forcing it to evaluate to to a list instead of allowing deferred execution you are actually calling getInvocation too many times. When currentDepth == depth - 1, you don't need to evaluate sourceFiles again and so you don't need to call getInvocations. With deferred execution, because the final sourceFiles is never read getInvocations is never called and everything is fine and dandy.

Answer 2

No

Your code looks pretty messy and that last linq statement is just a killer. To top it off the triple-nested foreach statement looks like serious code smell. I'd suggest breaking your code up into smaller methods that each do one thing which might reduce the perceived complexity of it

Answer 3

I also decided to try spliting breaking the triple loop as mko into smaller methods, each is needed since i want to go down the hierachy x number of times, loop through the parents and update the grid based on the children so here is what I came up with. I am not sure it made it better though.

    private void UpdateGridFromSourceHierarchy(int depth, IEnumerable<SourceFile> list)
    {
        //Get the initial set of sourcefiles         
        var sourceFiles = list.SelectMany(current => current.getInvocations()                                 
                                       .Select(invocation => new {current = (SourceFile) null, invocation}))             
                                       .ToList() //This ensures that getInvocations is only called once for each sourcefile             
            .GroupBy(x => x.current, x => x.invocation);          
        for (var currentDepth = 0; currentDepth <= depth; currentDepth++)
        {
            LookThroughParentSourceFilesToUpdateGrid(sourceFiles);
            //Select the current sub source files             
            sourceFiles = GetNextSourceFileLevel(sourceFiles);
        }
    }

    private void LookThroughParentSourceFilesToUpdateGrid(IEnumerable<IGrouping<SourceFile, SourceFile>> sourceFiles)
    {
        foreach (var currentGroup in sourceFiles)
        {
            currentGroup.Count();                 
            LoopThroughSourceFilesToUpdateGrid(currentGroup);
        }
    }

    private void LoopThroughSourceFilesToUpdateGrid(IGrouping<SourceFile, SourceFile> currentGroup)
    {
        int counter = 0;
        foreach (var invocation in currentGroup)
        {
            /*                      
             * * Generalized grid code goes here
             * */                     
            counter++;
            UpdateGridPosition(currentGroup,invocation,counter);
        }
    }

Answer 4

This looks really weird:

    var sourceFiles = list
        .SelectMany(current => current.getInvocations()
                            .Select(invocation => new {current = (SourceFile) null, invocation}))
        .ToList() //This ensures that getInvocations is only called once for each sourcefile
        .GroupBy(x => x.current, x => x.invocation);

Why do you need to create a property current with null value and then group by it? Am I missing something?

Also if you do not use current variable from SelectMany lambda in Select statement then I think it is better to reduce nesting and pull Select out from SelectMany:

    var sourceFiles = list
        .SelectMany(current => current.getInvocations())
        .Select(invocation => new {current = (SourceFile) null, invocation})
        .ToList() //This ensures that getInvocations is only called once for each sourcefile
        .GroupBy(x => x.current, x => x.invocation);

Your code looks wrong to me (regarding grouping by null value). And if it wrong how can we improve it's readability?

Answer 5

I decided to work a bit on the second LINQ query and here is what I came up with:

                //Select the current sub source files             
            sourceFiles = GetNextSourceFileLevel(sourceFiles);
        }
    }

    private IEnumerable<IGrouping<SourceFile, SourceFile>> GetNextSourceFileLevel(IEnumerable<IGrouping<SourceFile, SourceFile>> sourceFiles)
    {
        var previousLevelOfSourceFiles = sourceFiles.SelectMany(current => current.Select(invocation => invocation));

        var previousLevelOfSourceFilesWithInvocations = previousLevelOfSourceFiles
            .SelectMany(newCurrent => newCurrent.getInvocations()
                                          .Select(invocation =>new {newCurrent, invocation}));
        var listOfSourceFiles = previousLevelOfSourceFilesWithInvocations.ToList();

        return listOfSourceFiles.GroupBy(x => x.newCurrent, x => x.invocation);
    }

I thought about making each loop in the method into its own method, and broke it up into smaller steps, I could have gone farther but did not want to have to create the annonymous class I was using.