Advertencia de manejo para una posible enumeración múltiple de IEnumerable

359

En mi código necesito usar IEnumerable<>varias veces para obtener el error Resharper de "Posible enumeración múltiple de IEnumerable".

Código de muestra:

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;
}
  • Puedo cambiar el objectsparámetro para que sea Listy luego evitar la posible enumeración múltiple, pero luego no obtengo el objeto más alto que puedo manejar.
  • Otra cosa que puedo hacer es convertir el IEnumerableal Listal principio del método:

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

Pero esto es simplemente incómodo .

¿Qué haría usted en este escenario?

gdoron está apoyando a Monica
fuente

Respuestas:

471

El problema con tomar IEnumerablecomo parámetro es que le dice a las personas que llaman "Deseo enumerar esto". No les dice cuántas veces desea enumerar.

Puedo cambiar el parámetro de los objetos para que sea Lista y luego evitar la posible enumeración múltiple, pero no obtengo el objeto más alto que puedo manejar .

El objetivo de tomar el objeto más alto es noble, pero deja espacio para demasiados supuestos. ¿Realmente desea que alguien pase una consulta LINQ to SQL a este método, solo para que lo enumere dos veces (obteniendo resultados potencialmente diferentes cada vez)?

La falta semántica aquí es que una persona que llama, que tal vez no se toma el tiempo para leer los detalles del método, puede asumir que solo repite una vez, por lo que le pasa un objeto costoso. La firma de su método no indica de ninguna manera.

Al cambiar la firma del método a IList/ ICollection, al menos le aclarará a la persona que llama cuáles son sus expectativas y podrá evitar errores costosos.

De lo contrario, la mayoría de los desarrolladores que miran el método podrían asumir que solo iteras una vez. Si tomar una IEnumerablees tan importante, debe considerar hacerlo .ToList()al comienzo del método.

Es una pena que .NET no tenga una interfaz que sea IEnumerable + Count + Indexer, sin los métodos Add / Remove, etc., que es lo que sospecho resolvería este problema.

Paul Stovell
fuente
32
¿ReadOnlyCollection <T> cumple con los requisitos de interfaz deseados? msdn.microsoft.com/en-us/library/ms132474.aspx
Dan Is Fiddling By Firelight
74
@DanNeely Sugeriría IReadOnlyCollection(T)(nuevo con .net 4.5) como la mejor interfaz para transmitir la idea de que se trata de IEnumerable(T)enumerar varias veces. Como dice esta respuesta, en IEnumerable(T)sí misma es tan genérica que incluso puede referirse a contenido no reseteable que no se puede enumerar nuevamente sin efectos secundarios. Pero IReadOnlyCollection(T)implica repetibilidad.
binki
1
Si lo hace .ToList()al comienzo del método, primero debe verificar si el parámetro no es nulo. Eso significa que obtendrá dos lugares donde arroja una excepción ArgumentException: si el parámetro es nulo y si no tiene ningún elemento.
Comecme
Leí este artículo sobre la semántica de IReadOnlyCollection<T>vs IEnumerable<T>y luego publiqué una pregunta sobre el uso IReadOnlyCollection<T>como parámetro, y en qué casos. Creo que la conclusión a la que finalmente llegué es la lógica a seguir. Que debe usarse donde se espera la enumeración, no necesariamente donde podría haber enumeración múltiple.
Neo
32

Si sus datos siempre serán repetibles, tal vez no se preocupe por eso. Sin embargo, también puede desenrollarlo; esto es especialmente útil si los datos entrantes pueden ser grandes (por ejemplo, leer desde el disco / red):

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

Tenga en cuenta que cambié un poco la semántica de DoSomethingElse, pero esto es principalmente para mostrar el uso desenrollado. Podría volver a ajustar el iterador, por ejemplo. También podría convertirlo en un bloque iterador, lo que podría ser agradable; entonces no hay list, y usted recibiría yield returnlos artículos a medida que los obtiene, en lugar de agregarlos a una lista para ser devueltos.

Marc Gravell
fuente
44
+1 Bueno, ese es un código muy bueno, pero ... pierdo la belleza y la legibilidad del código.
Gdoron está apoyando a Mónica el
55
@gdoron no todo es bonito; p No sé que lo anterior es ilegible. Especialmente si se convierte en un bloque iterador, bastante lindo, en mi opinión.
Marc Gravell
Solo puedo escribir: "var objectList = objects.ToList ();" y guarde todo el manejo del enumerador.
Gdoron está apoyando a Mónica el
10
@gdoron en la pregunta que usted dijo explícitamente que realmente no quería hacer eso; p Además, un enfoque ToList () puede no ser adecuado si los datos son muy grandes (potencialmente, las secuencias infinitas no necesitan ser finitas, pero pueden todavía se iterará). En última instancia, solo estoy presentando una opción. Solo usted conoce el contexto completo para ver si está justificado. Si sus datos son repetibles, otra opción válida es: ¡no cambie nada! Ignora R # en su lugar ... todo depende del contexto.
Marc Gravell
1
Creo que la solución de Marc es bastante buena. 1. Está muy claro cómo se inicializa la 'lista', está muy claro cómo se itera la lista y está muy claro en qué miembros de la lista se opera.
Keith Hoffman
9

Usar IReadOnlyCollection<T>o IReadOnlyList<T>en la firma del método en lugar de IEnumerable<T>, tiene la ventaja de hacer explícito que es posible que deba verificar el recuento antes de iterar, o iterar varias veces por alguna otra razón.

Sin embargo, tienen un gran inconveniente que causará problemas si intentas refactorizar tu código para usar interfaces, por ejemplo, para que sea más comprobable y amigable para el proxy dinámico. El punto clave es que IList<T>no hereda deIReadOnlyList<T> , y de manera similar para otras colecciones y sus respectivas interfaces de solo lectura. (En resumen, esto se debe a que .NET 4.5 quería mantener la compatibilidad ABI con versiones anteriores. Pero ni siquiera aprovecharon la oportunidad para cambiar eso en .NET core ) .

Esto significa que si obtienes un IList<T>de alguna parte del programa y quieres pasarlo a otra parte que espera un IReadOnlyList<T>, ¡no puedes! Sin embargo, puede pasar un IList<T>como unIEnumerable<T> .

Al final, IEnumerable<T>es la única interfaz de solo lectura compatible con todas las colecciones .NET, incluidas todas las interfaces de colección. Cualquier otra alternativa volverá a morderte cuando te des cuenta de que te has bloqueado de algunas opciones de arquitectura. Por lo tanto, creo que es el tipo adecuado para usar en las firmas de funciones para expresar que solo desea una colección de solo lectura.

(Tenga en cuenta que siempre puede escribir un IReadOnlyList<T> ToReadOnly<T>(this IList<T> list)método de extensión que convierta simples si el tipo subyacente admite ambas interfaces, pero debe agregarlo manualmente en todas partes al refactorizar, donde IEnumerable<T>siempre es compatible).

Como siempre, esto no es absoluto, si está escribiendo un código pesado en la base de datos donde la enumeración múltiple accidental sería un desastre, es posible que prefiera una compensación diferente.

Gabriel Morin
fuente
2
+1 Por venir en una publicación anterior y agregar documentación sobre nuevas formas de resolver este problema con las nuevas funciones proporcionadas por la plataforma .NET (es decir, IReadOnlyCollection <T>)
Kevin Avignon
4

Si el objetivo es realmente evitar enumeraciones múltiples, entonces la respuesta de Marc Gravell es la que debe leerse, pero manteniendo la misma semántica, simplemente puede eliminar el redundante Anyy las Firstllamadas e ir con:

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

Tenga en cuenta que esto supone que usted IEnumerableno es genérico o al menos está limitado a ser un tipo de referencia.

João Angelo
fuente
2
objectstodavía se pasa a DoSomethingElse, que devuelve una lista, por lo que aún existe la posibilidad de que sigas enumerando dos veces. De cualquier manera, si la colección fue una consulta LINQ to SQL, presionó la base de datos dos veces.
Paul Stovell
1
@PaulStovell, lea esto: "Si el objetivo es realmente evitar múltiples enumeraciones, la respuesta de Marc Gravell es la que se debe leer" =)
gdoron está apoyando a Monica el
Se ha sugerido en algunas otras publicaciones de stackoverflow sobre el lanzamiento de excepciones para listas vacías y lo sugeriré aquí: ¿por qué lanzar una excepción en una lista vacía? Obtenga una lista vacía, dé una lista vacía. Como paradigma, eso es bastante sencillo. Si la persona que llama no sabe que está pasando una lista vacía, ese es el problema.
Keith Hoffman
@Keith Hoffman, el código de muestra mantiene el mismo comportamiento que el código que publicó el OP, donde el uso de Firstarrojará una excepción para una lista vacía. No creo que sea posible decir nunca lanzar una excepción en la lista vacía o siempre lanzar una excepción en la lista vacía. Depende ...
João Angelo
Muy bien Joao. Más para reflexionar que una crítica de tu publicación.
Keith Hoffman
3

Por lo general, sobrecargo mi método con IEnumerable e IList en esta situación.

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

Me aseguro de explicar en los comentarios resumidos de los métodos que llamar a IEnumerable realizará un .ToList ().

El programador puede elegir .ToList () en un nivel superior si se concatenan varias operaciones y luego llamar a la sobrecarga de IList o dejar que mi sobrecarga IEnumerable se encargue de eso.

Mauro Sampietro
fuente
20
Esto es horrible
Mike Chamberlain
1
@MikeChamberlain Sí, es realmente horrible y completamente limpio al mismo tiempo. Desafortunadamente, algunos escenarios pesados ​​necesitan este enfoque.
Mauro Sampietro
1
Pero entonces estaría recreando la matriz cada vez que la pase al método parametrizado IEnumerable. Estoy de acuerdo con @MikeChamberlain a este respecto, esta es una sugerencia terrible.
Krythic
2
@Krythic Si no necesita recrear la lista, realice una ToList en otro lugar y use la sobrecarga de IList. Ese es el punto de esta solución.
Mauro Sampietro
0

Si solo necesita verificar el primer elemento, puede echarle un vistazo sin iterar toda la colección:

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
fuente
-3

En primer lugar, esa advertencia no siempre significa mucho. Por lo general, lo desactivé después de asegurarme de que no sea un cuello de botella de rendimiento. Simplemente significa que IEnumerablese evalúa dos veces, lo que generalmente no es un problema a menos que el evaluationmismo tome mucho tiempo. Incluso si toma mucho tiempo, en este caso solo está usando un elemento la primera vez.

En este escenario, también podría explotar los poderosos métodos de extensión de linq aún más.

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

Es posible evaluar solo IEnumerableuna vez en este caso con algunos problemas, pero primero perfile y vea si realmente es un problema.

vidstige
fuente
15
Trabajo mucho con IQueryable y desactivar ese tipo de advertencias es muy peligroso.
Gdoron está apoyando a Mónica el
55
Evaluar dos veces es algo malo en mis libros. Si el método toma IEnumerable, puedo sentir la tentación de pasarle una consulta LINQ to SQL, lo que da como resultado múltiples llamadas a la base de datos. Dado que la firma del método no indica que puede enumerar dos veces, debe cambiar la firma (aceptar IList / ICollection) o solo repetir una vez
Paul Stovell
44
optimizar la legibilidad temprana y arruinar y, por lo tanto, la posibilidad de hacer optimizaciones que marquen la diferencia más adelante es mucho peor en mi libro.
vidstige
Cuando tienes una consulta de base de datos, asegúrate de que solo se evalúe una vez que ya haga la diferencia. No es solo un beneficio teórico futuro, es un beneficio real medible en este momento. No solo eso, evaluar solo una vez no solo es beneficioso para el rendimiento, sino que también es necesario para la corrección. Ejecutar una consulta de base de datos dos veces puede producir resultados diferentes, ya que alguien puede haber emitido un comando de actualización entre las dos evaluaciones de consulta.
1
@hvd No recomendé tal cosa. Por supuesto, no debe enumerar IEnumerableseso que da resultados diferentes cada vez que lo repite o que toma mucho tiempo repetirlo. Vea la respuesta de Marc Gravells: también declara en primer lugar "tal vez no se preocupe". La cuestión es que muchas veces ves esto y no tienes de qué preocuparte.
vidstige