Raster aus der Quellhierarchie aktualisieren

1129
Sean Lynch

In dieser Frage habe ich mit diesem Linq geantwortet. Ich mache zwar das, wonach ich gesucht habe, aber ich bin nicht sicher, wie einfach die Linq-Abfragen für andere sind. Daher bin ich auf der Suche nach Feedback zum Formatieren, welche Kommentare hilfreich wären und andere alternative Ansätze, um die Datensätze der Kinder durchzublättern.

    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);
        }
    }
Antworten
9

5 Antworten auf die Frage

8
ICR

Abgesehen von dem Problem von tief verschachteltem Code denke ich, dass die Lesbarkeit durch die Verwendung der LINQ-Syntax erheblich verbessert wird:

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;
}

Das macht SelectMany für mich viel einfacher zu folgen. Es gibt auch einige andere Verbesserungen an den Abfragen:

  • Sie können einfach direkt nach Null gruppieren, anstatt auf einen anonymen Typ zu projizieren und diesen wieder als Schlüssel auszuwählen.
  • Sie müssen nicht in eine Liste konvertieren, wenn Sie sie einer Gruppe folgen. Die Gruppe durchläuft und speichert das Ergebnis der Aufzählung trotzdem. Sie können dies testen, indem Sie die ToList entfernen und in getInvocations eine Haltepunkt- / Debug-Ausgabe hinzufügen.
  • Durch das Erzwingen der Auswertung in eine Liste, anstatt eine verzögerte Ausführung zuzulassen, rufen Sie getInvocation tatsächlich zu oft auf. Wenn currentDepth == depth - 1, müssen Sie sourceFiles nicht erneut auswerten, und Sie müssen getInvocations nicht aufrufen. Bei verzögerter Ausführung, weil die endgültigen sourceFiles nie gelesen werden, wird getInvocations nie aufgerufen und alles ist in Ordnung.
Dein Recht ist viel sauberer. Sean Lynch vor 9 Jahren 0
4
Homde

Nein

Ihr Code sieht ziemlich chaotisch aus und die letzte linq-Anweisung ist nur ein Killer. Um das Ganze abzurunden, sieht die dreifach verschachtelte foreach-Anweisung wie ein ernsthafter Codegeruch aus. Ich würde vorschlagen, Ihren Code in kleinere Methoden aufzuteilen, von denen jede etwas tut, was die wahrgenommene Komplexität reduzieren könnte

Sie könnten ein Refactoring-Tool wie Resharper verwenden, um diesen Code lesbarer zu machen. Aim Kai vor 9 Jahren 0
Könnten Sie mir Alternativen für die erste und letzte linq-Anweisung geben, vorzugsweise im Code? Nur zu sagen, dass es chaotisch ist, hilft nicht viel, es war der ganze Grund, warum ich die Frage gestellt habe. Sie sind beide im Grunde die gleiche Aussage, es ist nur so, dass in der obigen Frage ein Verweis auf die vorherige aktuelle Ebene des Baums beibehalten werden musste. Sean Lynch vor 9 Jahren 3
+1, um einen ungerechtfertigten Wert -1 zu löschen. @ Sean: Warum ist die Beantwortung der Frage, die Sie explizit gestellt haben, nicht hilfreich? Er hat keine Verpflichtung, Ihre Linq-Anfragen für Sie umzuschreiben. Dies ist Code Review, nicht Code Write It For Me. doppelgreener vor 9 Jahren 1
@Aim Kai Resharper hat nicht wirklich viel über den Code zu sagen. Sean Lynch vor 9 Jahren 0
@Jonathan Ihr Recht, meine Frage noch einmal zu lesen, hatte ich nicht ausdrücklich erklärt, dass ich wusste, dass die letzte linq-Anweisung unordentlich war. Ich denke jedoch, dass die etwas kopierte und frühere Antwort in kleinere Methoden zerlegt wird, ohne dass ein Link zu den Beispielen, die Sie vorschlagen, nur Unordnung hinzufügt. Sean Lynch vor 9 Jahren 2
4
Sean Lynch

Ich entschied mich auch für das Splitting, indem ich die Triple-Schleife als mko in kleinere Methoden zerlegte. Jede dieser Methoden wird benötigt, da ich die Hierarchie x-mal durchgehen, die Eltern durchlaufen und das Raster basierend auf den Kindern aktualisieren möchte mit. Ich bin mir nicht sicher, ob es besser ist.

    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);
        }
    }
3
Snowbear

Das sieht wirklich komisch aus:

    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);

Warum müssen Sie eine Eigenschaft currentmit Nullwert erstellen und danach gruppieren? Fehlt mir etwas?

Auch wenn Sie keine currentVariable aus SelectManyLambda in SelectAnweisung verwenden, denke ich, ist es besser, die Verschachtelung zu reduzieren und Selectaus 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);

Ihr Code sieht für mich falsch aus (bezüglich Gruppierung nach Nullwert). Und wenn es falsch ist, wie können wir die Lesbarkeit verbessern?

Ich habe es so gemacht, dass der Typ mit dem Typ übereinstimmt, der von der LINQ-Anweisung unten zurückgegeben wird. Sean Lynch vor 9 Jahren 0
@Sean - gibt `getInvocations ()` `IEnumerable zurück"? Snowbear vor 9 Jahren 0
Ja. Aber ich weiß nicht, was die eigentliche Implementierung ist, da es sich um ein Implementierungsdetail handelt, das nicht in http://codereview.stackexchange.com/questions/9/how-to-improve-very-loopy-method angegeben ist. Sean Lynch vor 9 Jahren 0
1
Sean Lynch

Ich habe mich entschieden, ein bisschen an der zweiten LINQ-Abfrage zu arbeiten, und hier ist Folgendes entstanden:

                //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);
    }

Ich überlegte, jede Schleife in der Methode zu einer eigenen Methode zu machen, und zerlegte sie in kleinere Schritte. Ich hätte noch weiter gehen können, wollte aber nicht die anonyme Klasse erstellen, die ich verwendete.