9
\$\begingroup\$

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);
        }
    }
\$\endgroup\$
8
\$\begingroup\$

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.
\$\endgroup\$
  • \$\begingroup\$ Your right that is much cleaner. \$\endgroup\$ – Sean Lynch Jan 27 '11 at 21:30
4
\$\begingroup\$

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

\$\endgroup\$
  • \$\begingroup\$ You could use a refactoring tool like Resharper to make this code more readable. \$\endgroup\$ – Aim Kai Jan 25 '11 at 11:08
  • 3
    \$\begingroup\$ could you give me alternatives for the first and last linq statements, preferably in code? Just saying it is messy doesn't help much, it was the whole reason I asked the question. They are both basically the same statement, it is just that in the question above a reference to the previous current level of the tree needed to be maintained. \$\endgroup\$ – Sean Lynch Jan 25 '11 at 11:12
  • 1
    \$\begingroup\$ +1 to cancel out an unjustified -1. @Sean: Why is answering the question you explicitly asked unhelpful? He has no obligations to rewrite your Linq queries for you. This is Code Review, not Code Write It For Me. \$\endgroup\$ – doppelgreener Jan 25 '11 at 13:02
  • \$\begingroup\$ @Aim Kai resharper doesn't really have much to say about the code. \$\endgroup\$ – Sean Lynch Jan 25 '11 at 13:05
  • 2
    \$\begingroup\$ @Jonathan Your right, rereading my question I had not explicitly stated that I knew the last linq statement was a messy. However I think the somewhat copy and past answer of break it into smaller methods without even a links to examples what you are suggesting just adds clutter. \$\endgroup\$ – Sean Lynch Jan 25 '11 at 13:09
4
\$\begingroup\$

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);
        }
    }
\$\endgroup\$
3
\$\begingroup\$

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?

\$\endgroup\$
  • \$\begingroup\$ I did it so that the type would be the same as the type returned from LINQ statement at the bottom. \$\endgroup\$ – Sean Lynch Jan 27 '11 at 14:55
  • \$\begingroup\$ @Sean - does getInvocations() return IEnumerable<SourceFile>? \$\endgroup\$ – Snowbear Jan 27 '11 at 15:13
  • \$\begingroup\$ Yes. But I don't know what the real implementation of it is, since it is an implementation detail not given in codereview.stackexchange.com/questions/9/…. \$\endgroup\$ – Sean Lynch Jan 27 '11 at 15:29
1
\$\begingroup\$

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.

\$\endgroup\$

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.