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\$
0

5 Answers 5

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\$
1
  • \$\begingroup\$ Your right that is much cleaner. \$\endgroup\$
    – Sean Lynch
    Jan 27, 2011 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\$
5
  • \$\begingroup\$ You could use a refactoring tool like Resharper to make this code more readable. \$\endgroup\$
    – Aim Kai
    Jan 25, 2011 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, 2011 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\$ Jan 25, 2011 at 13:02
  • \$\begingroup\$ @Aim Kai resharper doesn't really have much to say about the code. \$\endgroup\$
    – Sean Lynch
    Jan 25, 2011 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, 2011 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\$
3
  • \$\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, 2011 at 14:55
  • \$\begingroup\$ @Sean - does getInvocations() return IEnumerable<SourceFile>? \$\endgroup\$
    – Snowbear
    Jan 27, 2011 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, 2011 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 and acknowledge you have read our privacy policy.

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