Iterando una clase que representa una colección: IEnumerable <T> vs método personalizado

9

A menudo encuentro que necesito implementar una clase que es una enumeración / colección de algo. Considere para este hilo el ejemplo artificial del IniFileContentcual es una enumeración / colección de líneas.

La razón por la que esta clase debe existir en mi base de código es porque quiero evitar que la lógica de negocios se extienda por todo el lugar (= encapsulando el where) y quiero hacerlo de la manera más orientada a objetos posible.

Por lo general, lo implementaría de la siguiente manera:

public sealed class IniFileContent : IEnumerable<string>
{
    private readonly string _filepath;
    public IniFileContent(string filepath) => _filepath = filepath;
    public IEnumerator<string> GetEnumerator()
    {
        return File.ReadLines(_filepath)
                   .Where(l => !l.StartsWith(";"))
                   .GetEnumerator();
    }
    public IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();
}

Elijo implementar IEnumerable<string>porque hace que su uso sea conveniente:

foreach(var line in new IniFileContent(...))
{
    //...
}

Sin embargo, me pregunto si, al hacerlo, "sombrea" la intención de la clase Cuando uno mira IniFileContentla interfaz, solo se ve Enumerator<string> GetEnumerator(). Creo que no es obvio qué servicio proporciona realmente la clase.

Considere entonces esta segunda implementación:

public sealed class IniFileContent2
{
    private readonly string _filepath;
    public IniFileContent2(string filepath) => _filepath = filepath;
    public IEnumerable<string> Lines()
    {
        return File.ReadLines(_filepath)
                   .Where(l => !l.StartsWith(";"));
    }
}

Que se usa menos convenientemente (por cierto, ver new X().Y()que algo salió mal con el diseño de la clase):

foreach(var line in new IniFileContent2(...).Lines())
{
    //...
}

Pero con una interfaz clara que IEnumerable<string> Lines()hace obvio lo que esta clase realmente puede hacer.

¿Qué implementación fomentarías y por qué? Implicado, ¿es una buena práctica implementar IEnumerable para representar una enumeración de algo?

No estoy buscando respuestas sobre cómo:

  • prueba de unidad de este código
  • hacer una función estática en lugar de una clase
  • hacer que este código sea más propenso a la futura evolución de la lógica empresarial
  • optimizar el rendimiento

Apéndice

Este es el tipo de código real que vive en mi base de código que implementaIEnumerable

public class DueInvoices : IEnumerable<DueInvoice>
{
    private readonly IEnumerable<InvoiceDto> _invoices;
    private readonly IEnumerable<ReminderLevel> _reminderLevels;
    public DueInvoices(IEnumerable<InvoiceDto> invoices, IEnumerable<ReminderLevel> reminderLevels)
    {
        _invoices = invoices;
        _reminderLevels = reminderLevels;
    }
    public IEnumerator<DueInvoice> GetEnumerator() => _invoices.Where(invoice => invoice.DueDate < DateTime.Today && !invoice.Paid)
                                                               .Select(invoice => new DueInvoice(invoice, _reminderLevels))
                                                               .GetEnumerator();
    IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();
}
Manchado
fuente
2
Relacionados stackoverflow.com/questions/21692193
enkryptor
2
Votar para cerrar esto ya que la pregunta actualizada es pedir opiniones sobre dos estilos de codificación y ahora está completamente basado en opiniones.
David Arno
1
@DavidArno No estoy de acuerdo en el sentido de que preguntar si algo es una buena práctica no se basa exclusivamente en la opinión, sino también en hechos, experiencias y normas.
Visto el
Ninguno de los patrones funciona para mí: dependencia de clases concretas, dependencia dura del sistema de archivos, convenciones de eliminación ambiguas (creo que puede tener una fuga, señor), semántica contraintuitiva del uso newpara este caso de uso, dificultad en las pruebas unitarias, ambigua cuando yo / O pueden ocurrir excepciones, etc. Lo siento, no estoy tratando de ser grosero. Creo que me he acostumbrado demasiado a los beneficios de la inyección de dependencia .
John Wu
@JohnWu Tenga en cuenta que este es un ejemplo artificial (elegido torpemente, lo confieso). Si desea responder a esta pregunta (sin tratar de ser grosero tampoco), considere centrarse en la decisión de diseño de implementar IEnumerable o no para una clase que es un IniFileContent.
Visto el

Respuestas:

13

Estoy revisando su enfoque antes de sugerir un enfoque completamente diferente. Prefiero el enfoque diferente, pero parece importante explicar por qué su enfoque tiene fallas.


Elijo implementar IEnumerable<string>porque hace que su uso sea conveniente

La conveniencia no debe ser mayor que la corrección.

Me pregunto si tu MyFileclase contendrá más lógica que esta; porque eso afectará la exactitud de esta respuesta. Estoy especialmente interesado en:

.Where(l => ...) //some business logic for filtering

porque si esto es lo suficientemente complejo o dinámico, está ocultando esa lógica en una clase cuyo nombre no revela que filtra su contenido antes de servirlo al consumidor.
Parte de mí espera / asume que esta lógica de filtro está destinada a ser codificada (por ejemplo, un filtro simple que ignora las líneas comentadas, por ejemplo, cómo los archivos .ini consideran que las líneas que comienzan #como un comentario) y no un conjunto de reglas específico de archivo.


public class MyFile : IEnumerable<string>

Hay algo realmente irritante en tener un singular ( File) que representa un plural ( IEnumerable). Un archivo es una entidad singular. Consiste en algo más que su contenido. También contiene metadatos (nombre de archivo, extensión, fecha de creación, fecha de modificación, ...).

Un humano es más que la suma de sus hijos. Un automóvil es más que la suma de sus partes. Una pintura es más que una colección de pinturas y lienzos. Y un archivo es más que una colección de líneas.


Si supongo que su MyFileclase nunca contendrá más lógica que solo esta enumeración de líneas (y Wheresolo aplica un filtro codificado estático simple), entonces lo que tiene aquí es un uso confuso del nombre del "archivo" y su designación prevista. . Esto se puede solucionar fácilmente cambiando el nombre de la clase como FileContent. Conserva la sintaxis deseada:

foreach(var line in new FileContent(@"C:\Folder\File.txt"))

También tiene más sentido desde un punto de vista semántico. El contenido de un archivo puede dividirse en líneas separadas. Esto todavía supone que el contenido del archivo es texto y no binario, pero eso es lo suficientemente justo.


Sin embargo, si su MyFileclase contendrá más lógica, la situación cambia. Hay algunas maneras en que esto podría suceder:

  • Comienza a usar esta clase para representar los metadatos del archivo, no solo su contenido.

Cuando comienza a hacer esto, el archivo representa el archivo en el directorio , que es más que solo su contenido.
El enfoque correcto aquí es entonces lo que has hecho MyFile2.

  • El Where()filtro comienza a tener una lógica de filtro complicada que no está codificada, por ejemplo, cuando diferentes archivos comienzan a filtrarse de manera diferente.

Cuando comienza a hacer esto, los archivos comienzan a tener identidades propias, ya que tienen su propio filtro personalizado. Esto significa que tu clase se ha convertido más en un FileTypeque en un FileContent. Los dos comportamientos necesitan ser ya sea separados o combinados usando la composición (lo que favorece su MyFile2enfoque), o preferentemente ambos (clases separadas para el FileTypey FileContentcomportamiento, y luego tener ambos compuestos en la MyFileclase).


Una sugerencia totalmente diferente.

Tal como están las cosas, tanto tu MyFilecomo MyFile2existen para darte una envoltura alrededor de tu .Where(l => ...)filtro. En segundo lugar, está creando efectivamente una clase para envolver un método estático ( File.ReadLines()), que no es un gran enfoque.

Por otro lado, no entiendo por qué elegiste hacer tu clase sealed. En todo caso, esperaría que la herencia sea su principal característica: diferentes clases derivadas con diferente lógica de filtrado (suponiendo que sea más complejo que un simple cambio de valor, porque la herencia no debe usarse solo para cambiar un solo valor)

Reescribiría toda su clase como:

foreach(var line in File.ReadLines(...).Where(l => ...))

El único inconveniente de este enfoque simplificado es que tendría que repetir el Where() filtro cada vez que desee acceder al contenido del archivo. Estoy de acuerdo en que eso no es deseable.

Sin embargo, parece excesivo que cuando quieras crear un reutilizable Where(l => ...) declaración , también obligue a esa clase a implementar File.ReadLines(...). Estás agrupando más de lo que realmente necesitas.

En lugar de intentar ajustar el método estático en una clase personalizada, creo que es mucho más apropiado si lo ajusta en un método estático propio:

public static IEnumerable<string> GetFilteredFileContent(string filePath)
{
    return File.ReadLines(filePath).Where(l => ...);
}

Suponiendo que tiene filtros diferentes, puede pasar el filtro apropiado como un parámetro. Le mostraré un ejemplo que puede manejar múltiples filtros, que debería poder manejar todo lo que necesita hacer mientras maximiza la reutilización:

public static class MyFile
{
    public static Func<string, bool> IgnoreComments = 
                  (l => !l.StartsWith("#"));

    public static Func<string, bool> OnlyTakeComments = 
                  (l => l.StartsWith("#"));

    public static Func<string, bool> IgnoreLinesWithTheLetterE = 
                  (l => !l.ToLower().contains("e"));

    public static Func<string, bool> OnlyTakeLinesWithTheLetterE = 
                  (l => l.ToLower().contains("e"));

    public static IEnumerable<string> ReadLines(string filePath, params Func<string, bool>[] filters)
    {
        var lines = File.ReadLines(filePath).Where(l => ...);

        foreach(var filter in filters)
            lines = lines.Where(filter);

        return lines;
    }
}

Y su uso:

MyFile.ReadLines("path", MyFile.IgnoreComments, MyFile.OnlyTakeLinesWithTheLetterE);

Este es solo un ejemplo de ejecución que pretende demostrar que los métodos estáticos tienen más sentido que crear una clase aquí.

No se deje atrapar por los detalles de la implementación de los filtros. Puede implementarlos como desee (personalmente me gusta la parametrización Func<>debido a su extensibilidad y adaptabilidad inherentes a la refactorización). Pero dado que en realidad no fue un ejemplo de los filtros que tiene la intención de usar, hice una suposición para mostrarle un ejemplo viable.


ver new X().Y()parece que algo salió mal con el diseño de la clase)

En su enfoque, podría haber hecho new X().Yque sea menos irritante.

Sin embargo, creo que su disgusto new X().Y()demuestra el punto de que siente que una clase no está justificada aquí, pero sí un método; que solo se puede representar sin una clase siendo estático.

Flater
fuente
Realmente aprecio sus comentarios y sobre todo su pensamiento que lo llevó a cambiar el nombre de la clase FileContent. Muestra cuán malo fue mi ejemplo. También fue realmente malo cómo formulé mi pregunta, que no logró reunir el tipo de comentarios que esperaba. Lo he editado con la esperanza de aclarar mi intención.
Visto el
@Flater, incluso si GetFilteredContent(string filename)se ejecuta en código con el nombre de archivo la mayor parte del tiempo, pondría el cuerpo principal del trabajo en un método que requirió una Streamo una TextReaderpara que las pruebas sean mucho más fáciles. Entonces GetFilteredContent(string)sería una envoltura alrededor GetFilteredContent(TextReader reader). Pero A está de acuerdo con su evaluación.
Berin Loritsch
4

En mi opinión, los problemas con ambos enfoques son:

  1. Estás encapsulando File.ReadLines, lo que hace que las pruebas unitarias sean más difíciles de lo necesario,
  2. Se debe crear una nueva instancia de clase cada vez que se enumera el archivo, solo para almacenar la ruta como _filepath.

Por lo tanto, sugeriría convertirlo en un método estático, que se pasa IEnumerable<string>o Streamrepresenta el contenido del archivo:

public static GetFilteredLines(IEnumerable<string> fileContents)
    => fileContents.Where(l => ...);

Luego se llama a través de:

var filteredLines = GetFilteredLines(File.ReadLines(filePath));

Esto evita poner una carga innecesaria en el montón y hace que sea mucho más fácil probar el método por unidad.

David Arno
fuente
Estoy de acuerdo con usted, sin embargo, no fue absolutamente el tipo de comentarios que esperaba (mi pregunta estaba mal formulada en ese sentido). Ver mi pregunta editada.
Visto el
@Spotted, wow, ¿realmente estás rechazando las respuestas a tu pregunta porque hiciste la pregunta incorrecta? Eso es bajo.
David Arno
Sí, lo hice hasta que me di cuenta de que el problema era mi pregunta mal formulada. Excepto que ahora no puedo anular mi voto negativo siempre que su respuesta no se edite ...: - / Por favor, acepte mis disculpas (o modifique su respuesta para que con mucho gusto elimine mi voto negativo).
Visto el
@Spotted, el problema con su pregunta ahora es que está pidiendo opiniones sobre dos estilos de codificación, lo que hace que la pregunta esté fuera de tema. Incluso con su pregunta actualizada, mi respuesta sigue siendo la misma: ambos diseños son defectuosos por las dos razones que especifico y, por lo tanto, tampoco es una buena solución en mi opinión.
David Arno
Estoy totalmente de acuerdo en que el diseño en mi ejemplo es defectuoso, pero no es el punto de mi pregunta (ya que es un ejemplo artificial), fue solo un pretexto para introducir estos dos enfoques.
Visto el
2

El ejemplo del mundo real que probó, se DueInvoicespresta muy bien al concepto de que se trata de una colección de facturas que se vencen actualmente. Entiendo completamente cómo los ejemplos artificiales pueden hacer que la gente se involucre en los términos que usó en comparación con el concepto que está tratando de transmitir. He estado en el lado frustrante de eso muchas veces.

Dicho esto, si el propósito de la clase es estrictamente ser un IEnumerable<T>, y no proporciona ninguna otra lógica, tengo que hacer la pregunta si necesita una clase completa o simplemente puede proporcionar un método fuera de otra clase. Por ejemplo:

public class Invoices
{
    // ... skip all the other stuff about Invoices

    public IEnumerable<Invoice> GetDueItems()
    {
         foreach(var line in File.ReadLines(_invoicesFile))
         {
             var invoice = ReadInvoiceFrom(line);
             if (invoice.PaymentDue <= DateTime.UtcNow)
             {
                 yield return invoice;
             }
         }
    }
}

El yield returnfunciona cuando no se puede simplemente envolver una consulta LINQ, o la incorporación de la lógica es más fácil de seguir. La otra opción es simplemente devolver la consulta LINQ:

public class Invoices
{
    // ... skip all the other stuff about invoices

    public IEnumerable<Invoice> GetDueItems()
    {
        return from Invoice invoice in GetAllItems()
               where invoice.PaymentDue <= DateTime.UtcNow
               select invoice;
    }
}

En ambos casos, no necesita una clase de envoltura completa. Solo necesita proporcionar un método y el iterador se maneja esencialmente por usted.

El único momento en que necesitaba una clase completa para manejar la iteración fue cuando tuve que extraer blobs de una base de datos en una consulta de larga duración. La utilidad era para una extracción única para que pudiéramos migrar los datos a otra parte. Hubo algunas rarezas que encontré con la base de datos cuando intenté transmitir el contenido usando yield return. Pero eso desapareció cuando realmente implementé mi costumbreIEnumerator<T> para controlar mejor cuándo se limpiaron los recursos. Esta es la excepción más que la regla.

En resumen, recomiendo no implementar IEnumerable<T>directamente si sus problemas se pueden resolver de cualquiera de las formas descritas en el código anterior. Ahorre la sobrecarga de crear el enumerador explícitamente para cuando no pueda resolver el problema de otra manera.

Berin Loritsch
fuente
La razón por la que creé una clase separada es que InvoiceDtoproviene de la capa de persistencia y, por lo tanto, es solo una bolsa de datos, no quería saturarla con un método relevante para el negocio. De ahí la creación de DueInvoicey DueInvoices.
Visto el
@Spotted, no tiene que ser la clase DTO en sí. Diablos, probablemente puede ser una clase estática con métodos de extensión. Todo lo que sugiero es que en la mayoría de los casos puede minimizar su código repetitivo y hacer que la API sea digerible al mismo tiempo.
Berin Loritsch
0

Piensa en conceptos. ¿Cuál es la relación entre un archivo y su contenido? Esa es una relación "tiene" , no una relación "es" .

En consecuencia, la clase de archivo debe tener un método / propiedad para devolver el contenido. Y todavía es conveniente llamar:

public IEnumerable<string> GetFilteredContents() { ... }

foreach(string line in myFile.GetFilteredContents() { ... }
Bernhard Hiller
fuente
Tienes razón sobre la relación entre un archivo y su contenido. El ejemplo no fue bien pensado. Hice algunas ediciones a mi pregunta original para precisar mis pensamientos.
Visto el
Acepte mis disculpas por el voto negativo injustificado, sin embargo, no puedo eliminarlo, excepto si edita su respuesta. : - /
Visto el