patterncsharpMinor
Update grid from source hierarchy
Viewed 0 times
updategridsourcehierarchyfrom
Problem
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 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 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);
}
}Solution
Leaving aside the problem of deeply nested code for now, I think readability is greatly improved by the use of LINQ syntax:
For me, this makes the SelectMany a lot easier to follow. There are some other tweaks to the queries too:
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.
Code Snippets
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;
}Context
StackExchange Code Review Q#182, answer score: 8
Revisions (0)
No revisions yet.