Handhabungswarnung für mögliche mehrfache Aufzählung von IEnumerable

358

In meinem Code muss ein IEnumerable<>mehrmals verwendet werden, daher wird der Resharper-Fehler "Mögliche Mehrfachaufzählung von IEnumerable" angezeigt .

Beispielcode:

public List<object> Foo(IEnumerable<object> objects)
{
    if (objects == null || !objects.Any())
        throw new ArgumentException();

    var firstObject = objects.First();
    var list = DoSomeThing(firstObject);        
    var secondList = DoSomeThingElse(objects);
    list.AddRange(secondList);

    return list;
}
  • Ich kann den objectsParameter so ändern Listund dann die mögliche Mehrfachaufzählung vermeiden, aber dann erhalte ich nicht das höchste Objekt, das ich verarbeiten kann.
  • Eine andere Sache, die ich tun kann, ist, das IEnumerablezu ListBeginn der Methode in zu konvertieren :

 public List<object> Foo(IEnumerable<object> objects)
 {
    var objectList = objects.ToList();
    // ...
 }

Das ist aber nur umständlich .

Was würden Sie in diesem Szenario tun?

Gdoron unterstützt Monica
quelle

Antworten:

470

Das Problem bei der Verwendung IEnumerableals Parameter besteht darin, dass Anrufer erfahren, dass ich dies aufzählen möchte. Es sagt ihnen nicht, wie oft Sie aufzählen möchten.

Ich kann den Objektparameter in Liste ändern und dann die mögliche Mehrfachaufzählung vermeiden, aber dann erhalte ich nicht das höchste Objekt, das ich verarbeiten kann .

Das Ziel, das höchste Objekt zu nehmen, ist nobel, lässt aber Raum für zu viele Annahmen. Möchten Sie wirklich, dass jemand eine LINQ to SQL-Abfrage an diese Methode übergibt, nur damit Sie sie zweimal aufzählen (jedes Mal möglicherweise unterschiedliche Ergebnisse erhalten?)

Die hier fehlende Semantik besteht darin, dass ein Aufrufer, der sich möglicherweise nicht die Zeit nimmt, die Details der Methode zu lesen, davon ausgeht, dass Sie nur einmal iterieren - er übergibt Ihnen also ein teures Objekt. Ihre Methodensignatur gibt keinen Weg an.

Indem Sie die Methodensignatur in IList/ ändern ICollection, machen Sie dem Anrufer zumindest klarer, was Ihre Erwartungen sind, und sie können kostspielige Fehler vermeiden.

Andernfalls gehen die meisten Entwickler, die sich die Methode ansehen, möglicherweise davon aus, dass Sie nur einmal iterieren. Wenn IEnumerablees so wichtig ist, eine zu nehmen, sollten Sie dies .ToList()zu Beginn der Methode in Betracht ziehen .

Es ist eine Schande, dass .NET keine Schnittstelle hat, die IEnumerable + Count + Indexer ist, ohne Methoden zum Hinzufügen / Entfernen usw., was meines Erachtens dieses Problem lösen würde.

Paul Stovell
quelle
31
Entspricht ReadOnlyCollection <T> Ihren gewünschten Schnittstellenanforderungen? msdn.microsoft.com/en-us/library/ms132474.aspx
Dan spielt am
73
@DanNeely Ich würde vorschlagen IReadOnlyCollection(T)(neu mit .net 4.5) als beste Schnittstelle, um die Idee zu vermitteln, dass es sich um eine IEnumerable(T)handelt, die mehrfach aufgezählt werden soll. Wie diese Antwort besagt, ist sie IEnumerable(T)selbst so allgemein gehalten, dass sie sich möglicherweise sogar auf nicht zurücksetzbare Inhalte bezieht, die ohne Nebenwirkungen nicht erneut aufgezählt werden können. Aber IReadOnlyCollection(T)impliziert Wiederholbarkeit.
Binki
1
Wenn Sie dies .ToList()zu Beginn der Methode tun , müssen Sie zuerst prüfen, ob der Parameter nicht null ist. Das bedeutet, dass Sie zwei Stellen erhalten, an denen Sie eine ArgumentException auslösen: Wenn der Parameter null ist und keine Elemente enthält.
Comecme
Ich habe diesen Artikel über die Semantik von IReadOnlyCollection<T>vs gelesen IEnumerable<T>und dann eine Frage zur Verwendung IReadOnlyCollection<T>als Parameter und in welchen Fällen gestellt. Ich denke, die Schlussfolgerung, zu der ich schließlich gekommen bin, ist die Logik, der ich folgen muss. Dass es dort verwendet werden sollte, wo eine Aufzählung erwartet wird, nicht unbedingt dort, wo es mehrere Aufzählungen geben könnte.
Neo
32

Wenn Ihre Daten immer wiederholbar sind, machen Sie sich vielleicht keine Sorgen. Sie können es jedoch auch abrollen. Dies ist besonders nützlich, wenn die eingehenden Daten sehr groß sein können (z. B. Lesen von der Festplatte / dem Netzwerk):

if(objects == null) throw new ArgumentException();
using(var iter = objects.GetEnumerator()) {
    if(!iter.MoveNext()) throw new ArgumentException();

    var firstObject = iter.Current;
    var list = DoSomeThing(firstObject);  

    while(iter.MoveNext()) {
        list.Add(DoSomeThingElse(iter.Current));
    }
    return list;
}

Hinweis: Ich habe die Semantik von DoSomethingElse ein wenig geändert, dies dient jedoch hauptsächlich dazu, die nicht gerollte Verwendung anzuzeigen. Sie können den Iterator beispielsweise erneut umbrechen. Sie könnten es auch zu einem Iteratorblock machen, was nett sein könnte; dann gibt es keine list- und Sie würden yield returndie Artikel so erhalten, wie Sie sie erhalten, anstatt sie einer Liste hinzuzufügen, die zurückgegeben werden soll.

Marc Gravell
quelle
4
+1 Nun, das ist ein sehr schöner Code, aber ich verliere die Schönheit und Lesbarkeit des Codes.
Gdoron unterstützt Monica
5
@gdoron nicht alles ist hübsch; p Ich weiß nicht, dass das oben Genannte jedoch unlesbar ist. Vor allem, wenn es zu einem Iteratorblock gemacht wird - ziemlich süß, IMO.
Marc Gravell
Ich kann einfach schreiben: "var objectList = objects.ToList ();" und speichern Sie die gesamte Enumerator-Behandlung.
Gdoron unterstützt Monica
10
@gdoron in der Frage, die Sie ausdrücklich gesagt haben, dass Sie das nicht wirklich wollten; p Auch ein ToList () -Ansatz ist möglicherweise nicht geeignet, wenn die Daten sehr groß sind (möglicherweise unendlich - Sequenzen müssen nicht endlich sein, können es aber noch iteriert werden). Letztendlich präsentiere ich nur eine Option. Nur Sie kennen den vollständigen Kontext, um festzustellen, ob dies gerechtfertigt ist. Wenn Ihre Daten wiederholbar sind, ist eine andere gültige Option: Ändern Sie nichts! Ignoriere stattdessen R # ... alles hängt vom Kontext ab.
Marc Gravell
1
Ich finde Marc's Lösung ganz nett. 1. Es ist sehr klar, wie die 'Liste' initialisiert wird, es ist sehr klar, wie die Liste iteriert wird, und es ist sehr klar, welche Mitglieder der Liste bearbeitet werden.
Keith Hoffman
8

Die Verwendung von IReadOnlyCollection<T>oder IReadOnlyList<T>in der Methodensignatur anstelle von IEnumerable<T>hat den Vorteil, dass explizit angegeben wird, dass Sie möglicherweise die Anzahl vor dem Iterieren überprüfen oder aus einem anderen Grund mehrmals iterieren müssen.

Sie haben jedoch einen großen Nachteil, der zu Problemen führen kann, wenn Sie versuchen, Ihren Code so umzugestalten, dass er Schnittstellen verwendet, um ihn beispielsweise testbarer und für dynamisches Proxying benutzerfreundlicher zu machen. Der entscheidende Punkt ist, dass IList<T>nicht vonIReadOnlyList<T> und für andere Sammlungen und ihre jeweiligen schreibgeschützten Schnittstellen geerbt wird. (Kurz gesagt, dies liegt daran, dass .NET 4.5 die ABI-Kompatibilität mit früheren Versionen beibehalten wollte. Sie nutzten jedoch nicht einmal die Gelegenheit, dies in .NET Core zu ändern. )

Dies bedeutet, dass Sie dies nicht können, wenn Sie eine IList<T>von einem Teil des Programms erhalten und an einen anderen Teil übergeben möchten, der eine erwartet IReadOnlyList<T>! Sie können jedoch eine IList<T>als übergebenIEnumerable<T> .

Letztendlich IEnumerable<T>ist dies die einzige schreibgeschützte Schnittstelle, die von allen .NET-Sammlungen einschließlich aller Sammlungsschnittstellen unterstützt wird. Jede andere Alternative wird Sie beißen, wenn Sie feststellen, dass Sie sich von einigen Architekturentscheidungen ausgeschlossen haben. Ich denke, es ist der richtige Typ für Funktionssignaturen, um auszudrücken, dass Sie nur eine schreibgeschützte Sammlung wünschen.

(Beachten Sie, dass Sie immer eine IReadOnlyList<T> ToReadOnly<T>(this IList<T> list)Erweiterungsmethode schreiben können, die einfach umgewandelt wird, wenn der zugrunde liegende Typ beide Schnittstellen unterstützt. Sie müssen sie jedoch beim Refactoring überall manuell hinzufügen, wo dies IEnumerable<T>immer kompatibel ist.)

Wie immer ist dies kein absoluter Kompromiss. Wenn Sie datenbankintensiven Code schreiben, bei dem eine versehentliche Mehrfachaufzählung eine Katastrophe darstellen würde, bevorzugen Sie möglicherweise einen anderen Kompromiss.

Gabriel Morin
quelle
2
+1 Für das Aufrufen eines alten Beitrags und das Hinzufügen von Dokumentation zu neuen Möglichkeiten zur Lösung dieses Problems mit den neuen Funktionen der .NET-Plattform (z. B. IReadOnlyCollection <T>)
Kevin Avignon
4

Wenn das Ziel wirklich darin besteht, mehrere Aufzählungen zu verhindern, ist die Antwort von Marc Gravell die zu lesende, aber wenn Sie dieselbe Semantik beibehalten, können Sie einfach die Redundanz Anyund die FirstAufrufe entfernen und fortfahren mit:

public List<object> Foo(IEnumerable<object> objects)
{
    if (objects == null)
        throw new ArgumentNullException("objects");

    var first = objects.FirstOrDefault();

    if (first == null)
        throw new ArgumentException(
            "Empty enumerable not supported.", 
            "objects");

    var list = DoSomeThing(first);  

    var secondList = DoSomeThingElse(objects);

    list.AddRange(secondList);

    return list;
}

Beachten Sie, dass dies voraussetzt, dass Sie IEnumerablenicht generisch sind oder zumindest ein Referenztyp sein müssen.

João Angelo
quelle
2
objectswird immer noch an DoSomethingElse übergeben, das eine Liste zurückgibt, sodass die Möglichkeit besteht, dass Sie immer noch zweimal aufzählen. In beiden Fällen haben Sie die Datenbank zweimal aufgerufen, wenn es sich bei der Sammlung um eine LINQ to SQL-Abfrage handelte.
Paul Stovell
1
@PaulStovell, Lesen Sie Folgendes: "Wenn das Ziel wirklich darin besteht, Mehrfachaufzählungen zu verhindern, lautet die Antwort von Marc Gravell" =)
gdoron unterstützt Monica
Es wurde in einigen anderen Stackoverflow-Posts vorgeschlagen, Ausnahmen für leere Listen auszulösen, und ich werde es hier vorschlagen: Warum eine Ausnahme in eine leere Liste werfen? Holen Sie sich eine leere Liste, geben Sie eine leere Liste. Als Paradigma ist das ziemlich einfach. Wenn der Anrufer nicht weiß, dass er eine leere Liste weitergibt, liegt das Problem vor.
Keith Hoffman
@Keith Hoffman, der Beispielcode behält das gleiche Verhalten wie der vom OP veröffentlichte Code bei, bei dessen Verwendung Firsteine Ausnahme für eine leere Liste ausgelöst wird. Ich denke nicht, dass es möglich ist zu sagen, niemals eine Ausnahme auf eine leere Liste zu werfen oder immer eine Ausnahme auf eine leere Liste zu werfen. Es kommt darauf an ...
João Angelo
Fair genug, Joao. Mehr Denkanstöße als Kritik an Ihrem Beitrag.
Keith Hoffman
3

Normalerweise überlade ich meine Methode in dieser Situation mit IEnumerable und IList.

public static IEnumerable<T> Method<T>( this IList<T> source ){... }

public static IEnumerable<T> Method<T>( this IEnumerable<T> source )
{
    /*input checks on source parameter here*/
    return Method( source.ToList() );
}

Ich erkläre in den zusammenfassenden Kommentaren der Methoden, dass der Aufruf von IEnumerable eine .ToList () ausführt.

Der Programmierer kann .ToList () auf einer höheren Ebene auswählen, wenn mehrere Vorgänge verkettet werden, und dann die IList-Überladung aufrufen oder meine IEnumerable-Überladung dafür sorgen lassen.

Mauro Sampietro
quelle
20
Das ist schrecklich.
Mike Chamberlain
1
@ MikeChamberlain Ja, es ist wirklich schrecklich und gleichzeitig absolut sauber. Leider benötigen einige schwere Szenarien diesen Ansatz.
Mauro Sampietro
1
Dann würden Sie das Array jedoch jedes Mal neu erstellen, wenn Sie es an die parameratisierte IEnumerable-Methode übergeben. Ich stimme @MikeChamberlain in dieser Hinsicht zu, dies ist ein schrecklicher Vorschlag.
Krythic
2
@Krythic Wenn die Liste nicht neu erstellt werden muss, führen Sie eine ToList an einer anderen Stelle durch und verwenden die IList-Überladung. Darum geht es bei dieser Lösung.
Mauro Sampietro
0

Wenn Sie nur das erste Element überprüfen müssen, können Sie einen Blick darauf werfen, ohne die gesamte Sammlung zu wiederholen:

public List<object> Foo(IEnumerable<object> objects)
{
    object firstObject;
    if (objects == null || !TryPeek(ref objects, out firstObject))
        throw new ArgumentException();

    var list = DoSomeThing(firstObject);
    var secondList = DoSomeThingElse(objects);
    list.AddRange(secondList);

    return list;
}

public static bool TryPeek<T>(ref IEnumerable<T> source, out T first)
{
    if (source == null)
        throw new ArgumentNullException(nameof(source));

    IEnumerator<T> enumerator = source.GetEnumerator();
    if (!enumerator.MoveNext())
    {
        first = default(T);
        source = Enumerable.Empty<T>();
        return false;
    }

    first = enumerator.Current;
    T firstElement = first;
    source = Iterate();
    return true;

    IEnumerable<T> Iterate()
    {
        yield return firstElement;
        using (enumerator)
        {
            while (enumerator.MoveNext())
            {
                yield return enumerator.Current;
            }
        }
    }
}
Madruel
quelle
-3

Zunächst einmal bedeutet diese Warnung nicht immer so viel. Normalerweise habe ich es deaktiviert, nachdem ich sichergestellt hatte, dass es sich nicht um einen Performance-Flaschenhals handelt. Es bedeutet nur, dass das IEnumerablezweimal ausgewertet wird, was normalerweise kein Problem ist, es sei denn, dasevaluation selbst dauert lange. Auch wenn es lange dauert, verwenden Sie in diesem Fall beim ersten Mal nur ein Element.

In diesem Szenario können Sie die leistungsstarken Linq-Erweiterungsmethoden auch noch stärker nutzen.

var firstObject = objects.First();
return DoSomeThing(firstObject).Concat(DoSomeThingElse(objects).ToList();

Es ist IEnumerablein diesem Fall möglich, das nur einmal mit etwas Aufwand zu bewerten , aber zuerst das Profil zu erstellen und zu prüfen, ob es wirklich ein Problem ist.

vidstige
quelle
15
Ich arbeite viel mit IQueryable und das Ausschalten solcher Warnungen ist sehr gefährlich.
Gdoron unterstützt Monica
5
Zweimal zu bewerten ist eine schlechte Sache in meinen Büchern. Wenn die Methode IEnumerable verwendet, könnte ich versucht sein, eine LINQ to SQL-Abfrage an sie zu übergeben, was zu mehreren DB-Aufrufen führt. Da die Methodensignatur nicht angibt, dass sie möglicherweise zweimal auflistet, sollte sie entweder die Signatur ändern (IList / ICollection akzeptieren) oder nur einmal iterieren
Paul Stovell,
4
Die frühzeitige Optimierung und die Beeinträchtigung der Lesbarkeit und damit die Möglichkeit, Optimierungen vorzunehmen, die später einen Unterschied machen, ist in meinem Buch viel schlimmer.
Vidstige
Wenn Sie eine Datenbankabfrage haben, um sicherzustellen , ist es nur einmal ausgewertet bereits einen Unterschied macht. Es ist nicht nur ein theoretischer zukünftiger Nutzen, es ist momentan ein messbarer realer Nutzen. Nicht nur das, nur einmal zu bewerten, ist nicht nur vorteilhaft für die Leistung, sondern auch für die Korrektheit. Das zweimalige Ausführen einer Datenbankabfrage kann zu unterschiedlichen Ergebnissen führen, da möglicherweise jemand einen Aktualisierungsbefehl zwischen den beiden Abfrageauswertungen ausgegeben hat.
1
@hvd Ich habe so etwas nicht empfohlen. Natürlich sollten Sie nicht aufzählen, IEnumerablesdass jedes Mal, wenn Sie es wiederholen, unterschiedliche Ergebnisse erzielt werden oder dass die Iteration sehr lange dauert. Siehe Marc Gravells Antwort - Er sagt auch in erster Linie "vielleicht keine Sorge". Die Sache ist - In vielen Fällen, in denen Sie dies sehen, müssen Sie sich keine Sorgen machen.
Vidstige