Obsługa ostrzeżenia o możliwym wielokrotnym wyliczeniu IEnumerable

358

W moim kodzie muszę używać IEnumerable<>kilka razy, dlatego otrzymuję błąd Resharper „Możliwe wielokrotne wyliczenie IEnumerable”.

Przykładowy kod:

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;
}
  • Mogę zmienić objectsparametr na, Lista następnie uniknąć możliwego wielokrotnego wyliczenia, ale wtedy nie otrzymuję najwyższego obiektu, który mogę obsłużyć.
  • Inną rzeczą, którą mogę zrobić, to przekonwertować IEnumerableaby Listna początku metody:

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

Ale to jest po prostu niezręczne .

Co byś zrobił w tym scenariuszu?

gdoron wspiera Monikę
źródło

Odpowiedzi:

470

Problem z przyjmowaniem IEnumerablejako parametru polega na tym, że mówi on dzwoniącym „chcę to wyliczyć”. Nie mówi im, ile razy chcesz wyliczyć.

Mogę zmienić parametr obiektów na List, a następnie uniknąć możliwego wielokrotnego wyliczenia, ale wtedy nie otrzymuję najwyższego obiektu, który mogę obsłużyć .

Cel wzięcia najwyższego przedmiotu jest szlachetny, ale pozostawia miejsce na zbyt wiele założeń. Czy naprawdę chcesz, aby ktoś przekazał kwerendę LINQ do SQL do tej metody, abyś mógł ją wyliczyć dwukrotnie (uzyskiwanie potencjalnie różnych wyników za każdym razem?)

Semantyczny brak tutaj polega na tym, że osoba dzwoniąca, która być może nie potrzebuje czasu na przeczytanie szczegółów metody, może założyć, że wykonasz iterację tylko raz - więc przekażą ci drogi przedmiot. Podpis metody nie wskazuje w żaden sposób.

Zmieniając sygnaturę metody na IList/ ICollection, przynajmniej sprawisz, że dzwoniący będzie bardziej przejrzysty, jakie są twoje oczekiwania, a także pozwoli uniknąć kosztownych błędów.

W przeciwnym razie większość programistów przyglądających się metodzie może założyć, że wykonasz iterację tylko raz. Jeśli podjęcie IEnumerablejest tak ważne, powinieneś rozważyć zrobienie tego .ToList()na początku metody.

Szkoda, że ​​.NET nie ma interfejsu, który jest IEnumerable + Count + Indexer, bez metod Add / Remove etc., co, jak podejrzewam, rozwiązałoby ten problem.

Paul Stovell
źródło
31
Czy ReadOnlyCollection <T> spełnia wybrane wymagania dotyczące interfejsu? msdn.microsoft.com/en-us/library/ms132474.aspx
Dan Is Fiddling By Firelight
73
@ DanNeely Sugerowałbym IReadOnlyCollection(T)(nowy w .net 4.5) jako najlepszy interfejs do przekazania idei, że jest on IEnumerable(T)przeznaczony do wielokrotnego liczenia. Jak stwierdza ta odpowiedź, IEnumerable(T)sama w sobie jest tak ogólna, że ​​może nawet odnosić się do treści, których nie można zresetować, których nie można ponownie wyliczyć bez skutków ubocznych. Ale IReadOnlyCollection(T)implikuje powtarzalność.
binki
1
Jeśli zrobisz to .ToList()na początku metody, najpierw musisz sprawdzić, czy parametr nie ma wartości null. Oznacza to, że otrzymasz dwa miejsca, w których zgłaszasz ArgumentException: jeśli parametr ma wartość NULL i nie zawiera żadnych elementów.
komiks
Przeczytałem ten artykuł na temat semantyki IReadOnlyCollection<T>vs, IEnumerable<T>a następnie opublikowałem pytanie dotyczące używania IReadOnlyCollection<T>jako parametru oraz w jakich przypadkach. Myślę, że wniosek, do którego ostatecznie doszedłem, jest logiką do naśladowania. To powinno być stosowane tam, gdzie oczekuje się wyliczenie, nie koniecznie tam, gdzie może być wielokrotnością wyliczenie.
Neo,
32

Jeśli Twoje dane zawsze będą powtarzalne, być może nie przejmuj się nimi. Można go jednak również rozwinąć - jest to szczególnie przydatne, jeśli przychodzące dane mogą być duże (na przykład odczyt z dysku / sieci):

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

Uwaga: Zmieniłem nieco semantię DoSomethingElse, ale ma to głównie na celu pokazanie rozwiniętego użycia. Możesz na przykład ponownie owinąć iterator. Możesz zrobić z niego również blok iteratora, co może być miłe; wtedy nie ma list- i dostaniesz yield returnprzedmioty, jak je zdobędziesz, zamiast dodawać do listy, która ma zostać zwrócona.

Marc Gravell
źródło
4
+1 Cóż, to bardzo fajny kod, ale- tracę ładność i czytelność kodu.
gdoron wspiera Monikę
5
@gdoron nie wszystko jest ładne; p Nie wydaje mi się, aby powyższe było nieczytelne. Zwłaszcza jeśli jest zrobiony z bloku iteratora - całkiem słodki, IMO.
Marc Gravell
Mogę po prostu napisać: „var objectList = objects.ToList ();” i zapisz całą obsługę modułu wyliczającego.
gdoron wspiera Monikę
10
@gdoron w pytaniu, które wyraźnie powiedziałeś, że tak naprawdę nie chcesz tego robić; p Również podejście ToList () może nie pasować, jeśli dane są bardzo duże (potencjalnie nieskończone - sekwencje nie muszą być skończone, ale mogą wciąż się iteruje). Ostatecznie przedstawiam tylko opcję. Tylko Ty znasz pełny kontekst, aby sprawdzić, czy jest to uzasadnione. Jeśli Twoje dane są powtarzalne, inną prawidłową opcją jest: nic nie zmieniaj! Zamiast tego zignoruj ​​R # ... wszystko zależy od kontekstu.
Marc Gravell
1
Myślę, że rozwiązanie Marca jest całkiem fajne. 1. Jest bardzo jasne, w jaki sposób inicjowana jest „lista”, jest bardzo jasne, w jaki sposób lista jest iterowana, i jest bardzo jasne, którzy członkowie listy są obsługiwani.
Keith Hoffman
8

Użycie IReadOnlyCollection<T>lub IReadOnlyList<T>w podpisie metody zamiast IEnumerable<T>, ma tę zaletę, że wyraźnie oznacza, że ​​może być konieczne sprawdzenie liczby przed iteracją lub wielokrotne iterowanie z innego powodu.

Mają jednak ogromny minus, który spowoduje problemy, jeśli spróbujesz przeredagować swój kod, aby używać interfejsów, na przykład aby uczynić go bardziej testowalnym i przyjaznym dla dynamicznego proxy. Kluczową kwestią jest to, że IList<T>nie dziedziczyIReadOnlyList<T> , i podobnie dla innych kolekcji i ich odpowiednich interfejsów tylko do odczytu. (Krótko mówiąc, to dlatego, że .NET 4.5 chciał zachować zgodność ABI z wcześniejszymi wersjami. Ale nawet nie skorzystali z okazji, aby zmienić to w .NET core. )

Oznacza to, że jeśli dostaniesz IList<T>z jakiejś części programu i chcesz przekazać ją do innej części, która oczekuje IReadOnlyList<T>, nie możesz! Możesz jednak przekazać IList<T>jakoIEnumerable<T> .

Ostatecznie IEnumerable<T>jest jedynym interfejsem tylko do odczytu obsługiwanym przez wszystkie kolekcje .NET, w tym wszystkie interfejsy kolekcji. Każda inna alternatywa wróci, by cię ugryźć, gdy zdasz sobie sprawę, że zablokowałeś się przed niektórymi opcjami architektury. Sądzę więc, że to właściwy typ w sygnaturach funkcji, aby wyrazić, że chcesz tylko kolekcję tylko do odczytu.

(Pamiętaj, że zawsze możesz napisać IReadOnlyList<T> ToReadOnly<T>(this IList<T> list)metodę rozszerzenia, która po prostu rzutuje, jeśli typ bazowy obsługuje oba interfejsy, ale musisz dodać ją ręcznie wszędzie podczas refaktoryzacji, gdzie jak IEnumerable<T>zawsze jest kompatybilna).

Jak zawsze nie jest to absolutne, jeśli piszesz obszerny kod bazy danych, w którym przypadkowe wielokrotne wyliczenie byłoby katastrofą, możesz preferować inny kompromis.

Gabriel Morin
źródło
2
+1 Za pojawienie się starego posta i dodanie dokumentacji dotyczącej nowych sposobów rozwiązania tego problemu dzięki nowym funkcjom udostępnianym przez platformę .NET (tj. IReadOnlyCollection <T>)
Kevin Avignon
4

Jeśli celem jest naprawdę zapobieganie wielokrotnym wyliczeniom, to odpowiedź należy do Marca Gravella, ale zachowując tę ​​samą semantykę, możesz po prostu usunąć zbędne Anyi Firstwywołania i zastosować:

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

Zauważ, że zakłada to, że nie jesteś IEnumerableogólny lub przynajmniej jest ograniczony do typu odniesienia.

João Angelo
źródło
2
objectsjest nadal przekazywany do DoSomethingElse, który zwraca listę, więc istnieje możliwość, że nadal wyliczasz dwa razy. Tak czy inaczej, jeśli kolekcja była zapytaniem LINQ to SQL, trafiłeś w DB dwukrotnie.
Paul Stovell,
1
@PaulStovell, Przeczytaj to: „Jeśli celem jest naprawdę uniknięcie wielu wyliczeń, to odpowiedź Marca Gravella to ta, którą należy przeczytać” =)
gdoron obsługuje Monikę
Zostało to zasugerowane w niektórych innych postach przepełnienia stosu podczas zgłaszania wyjątków dla pustych list, a ja zasugeruję tutaj: po co rzucać wyjątek na pustą listę? Zdobądź pustą listę, podaj pustą listę. Jako paradygmat jest to całkiem proste. Jeśli dzwoniący nie wie, że podaje pustą listę, oznacza to problem.
Keith Hoffman
@Keith Hoffman, przykładowy kod zachowuje to samo zachowanie, co kod opublikowany przez OP, gdzie użycie Firstspowoduje wygenerowanie wyjątku dla pustej listy. Nie sądzę, że można powiedzieć, że nigdy nie rzucaj wyjątków na pustą listę lub zawsze rzucaj wyjątki na pustą listę. To zależy ...
João Angelo
Wystarczająco Joao. Więcej jedzenia do przemyślenia niż krytyka twojego postu.
Keith Hoffman
3

W tej sytuacji zwykle przeciążam moją metodę IEnumerable i 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() );
}

Staram się wyjaśnić w komentarzach podsumowujących metody, że wywołanie IEnumerable wykona funkcję .ToList ().

Programista może wybrać opcję .ToList () na wyższym poziomie, jeśli wiele operacji jest konkatenowanych, a następnie wywołać przeciążenie IList lub pozwolić, aby moje IEnumerable przeciążenie się tym zajęło.

Mauro Sampietro
źródło
20
To jest okropne.
Mike Chamberlain,
1
@MikeChamberlain Tak, to naprawdę jest przerażające i całkowicie czyste w tym samym czasie. Niestety niektóre ciężkie scenariusze wymagają takiego podejścia.
Mauro Sampietro,
1
Ale wtedy odtwarzasz tablicę za każdym razem, gdy przekazujesz ją do sparamatyzowanej metody IEnumerable. Zgadzam się z @MikeChamberlain w tym względzie, jest to okropna sugestia.
Krythic
2
@Krythic Jeśli nie potrzebujesz ponownie utworzyć listy, wykonaj ToList gdzie indziej i użyj przeciążenia IList. O to właśnie chodzi w tym rozwiązaniu.
Mauro Sampietro
0

Jeśli chcesz tylko sprawdzić pierwszy element, możesz zerknąć na niego bez iteracji całej kolekcji:

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
źródło
-3

Po pierwsze, to ostrzeżenie nie zawsze tyle znaczy. Zwykle wyłączałem go po upewnieniu się, że nie jest to szyjka butelki wydajności. Oznacza to tylko, że IEnumerableocena jest oceniana dwukrotnie, co zwykle nie stanowi problemu, chyba że evaluationsamo zajmuje dużo czasu. Nawet jeśli zajmuje to dużo czasu, w tym przypadku używasz tylko jednego elementu za pierwszym razem.

W tym scenariuszu można jeszcze bardziej wykorzystać potężne metody rozszerzenia linq.

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

W IEnumerabletym przypadku można ocenić tylko raz z pewnym kłopotem, ale najpierw profiluj i sprawdź, czy to naprawdę problem.

vidstige
źródło
15
Dużo pracuję z IQueryable, a wyłączenie takich ostrzeżeń jest bardzo niebezpieczne.
gdoron wspiera Monikę
5
Dwukrotna ocena jest złą rzeczą w moich książkach. Jeśli metoda przyjmuje IEnumerable, może mnie kusić przekazanie do niej zapytania LINQ do SQL, co powoduje wiele wywołań DB. Ponieważ podpis metody nie wskazuje, że może on wyliczyć dwa razy, powinien albo zmienić podpis (zaakceptować IList / ICollection), albo tylko iterować tylko raz
Paul Stovell,
4
optymalizacja wczesnej i rujnującej czytelności, a tym samym możliwość dokonywania optymalizacji, które później różnicują, jest znacznie gorsza w mojej książce.
vidstige
Gdy masz zapytanie do bazy danych, upewnienie się, że jest ono oceniane tylko raz, już robi różnicę. To nie tylko teoretyczna korzyść w przyszłości, to teraz wymierna realna korzyść. Nie tylko to, że ocena tylko raz jest nie tylko korzystna dla wydajności, ale także dla poprawności. Dwukrotne wykonanie zapytania do bazy danych może dać różne wyniki, ponieważ ktoś mógł wydać polecenie aktualizacji między dwiema ocenami zapytania.
1
@hvd Nie polecałem żadnej takiej rzeczy. Oczywiście nie powinieneś wyliczyć, IEnumerablesktóra daje różne wyniki za każdym razem, gdy ją powtarzasz, lub która zajmuje naprawdę dużo czasu. Zobacz odpowiedź Marca Gravellsa - Stwierdza on przede wszystkim: „może nie martw się”. Rzecz w tym, że wiele razy to widzisz, nie masz się czym martwić.
vidstige