Iterando uma classe que representa uma coleção: IEnumerable <T> vs método personalizado

9

Muitas vezes me vejo precisando implementar uma classe que é uma enumeração / coleção de algo. Considere para este segmento o exemplo inventado do IniFileContentqual é uma enumeração / coleção de linhas.

A razão pela qual essa classe deve existir na minha base de código é que eu quero evitar que a lógica de negócios seja espalhada por todo o lugar (= encapsulando the where) e quero fazê-lo da maneira mais orientada a objetos possível.

Normalmente eu o implementaria como abaixo:

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

Eu escolho implementar IEnumerable<string>porque torna seu uso conveniente:

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

No entanto, eu estou querendo saber se isso "obscurece" a intenção da classe? Quando alguém olha para IniFileContenta interface, apenas vê Enumerator<string> GetEnumerator(). Eu acho que não é óbvio qual serviço a classe está realmente fornecendo.

Considere então esta segunda implementação:

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 é usado de forma menos conveniente (a propósito, ver new X().Y()parece que algo deu errado com o design da classe):

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

Mas com uma interface clara IEnumerable<string> Lines()tornando óbvio o que essa classe pode realmente fazer.

Qual implementação você adotaria e por quê? Implícito, é uma boa prática implementar IEnumerable para representar uma enumeração de algo?

Não estou procurando respostas sobre como:

  • teste de unidade este código
  • faça uma função estática em vez de uma classe
  • torne esse código mais propenso à evolução futura da lógica de negócios
  • otimizar o desempenho

Apêndice

Aqui está o tipo de código real que vive na minha 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();
}
Visto
fonte
2
Relacionados stackoverflow.com/questions/21692193
enkryptor
2
A votação para encerrar isso, pois a pergunta atualizada está pedindo opiniões sobre dois estilos de codificação e agora é completamente baseada em opiniões.
David Arno
1
@DavidArno Não concordo no sentido de que perguntar se algo é uma boa prática não se baseia inteiramente na opinião, mas também em fatos, experiências e normas.
manchado
Nenhum padrão funciona para mim: dependência de classes concretas, dependência rígida do sistema de arquivos, convenções de eliminação ambígua (acredito que você possa ter um vazamento, senhor), semântica contra-intuitiva de uso newpara este caso de uso, dificuldade no teste de unidade, ambígua quando I / O exceções podem ocorrer, etc. Sinto muito, não estou tentando ser rude. Acho que me acostumei demais aos benefícios da injeção de dependência .
John Wu
@ JohnWu Por favor, considere que este é um exemplo artificial (desajeitadamente escolhido, eu confesso). Se você deseja responder a essa pergunta (não tentando ser rude também), considere focar na decisão de design de implementar IEnumerable ou não para uma classe que seja IniFileContent.
manchado

Respostas:

13

Estou analisando sua abordagem antes de sugerir uma abordagem totalmente diferente. Estou preferindo a abordagem diferente, mas parece importante explicar por que sua abordagem tem falhas.


Eu escolho implementar IEnumerable<string>porque torna seu uso conveniente

A conveniência não deve exceder a correção.

Gostaria de saber se sua MyFileclasse conterá mais lógica do que isso; porque isso afetará a exatidão desta resposta. Estou especialmente interessado em:

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

porque se isso é suficientemente complexo ou dinâmico, você está ocultando essa lógica em uma classe cujo nome não revela que ele filtra o conteúdo antes de servi-lo ao consumidor.
Parte de mim espera / pressupõe que essa lógica de filtro seja codificada (por exemplo, um filtro simples que ignore as linhas comentadas, por exemplo, como os arquivos .ini consideram as linhas que começam com #um comentário) e não um conjunto de regras específico do arquivo.


public class MyFile : IEnumerable<string>

Há algo realmente irritante em ter um singular ( File) representando um plural ( IEnumerable). Um arquivo é uma entidade singular. Consiste em mais do que apenas seu conteúdo. Ele também contém metadados (nome do arquivo, extensão, data de criação, data de modificação, ...).

Um humano é mais do que a soma de seus filhos. Um carro é mais do que a soma de suas partes. Uma pintura é mais do que uma coleção de tintas e telas. E um arquivo é mais do que uma coleção de linhas.


Se eu presumir que sua MyFileclasse nunca conterá mais lógica do que apenas essa enumeração de linhas (e a Whereúnica se aplica a um filtro estático codificado estático simples), o que você tem aqui é um uso confuso do nome do "arquivo" e da designação pretendida . Isso pode ser facilmente corrigido renomeando a classe como FileContent. Ele mantém a sintaxe desejada:

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

Também faz mais sentido do ponto de vista semântico. O conteúdo de um arquivo pode ser dividido em linhas separadas. Isso ainda pressupõe que o conteúdo do arquivo seja texto e não binário, mas isso é justo.


Se, no entanto, sua MyFileclasse contiver mais lógica, a situação mudará. Existem algumas maneiras de isso acontecer:

  • Você começa a usar essa classe para representar os metadados do arquivo, não apenas o conteúdo.

Quando você começa a fazer isso, o arquivo representa o arquivo no diretório , que é mais do que apenas seu conteúdo.
A abordagem correta aqui é o que você fez MyFile2.

  • O Where()filtro começa a ter uma lógica de filtro complicada que não é codificada, por exemplo, quando arquivos diferentes começam a ser filtrados de maneira diferente.

Quando você começa a fazer isso, os arquivos começam a ter identidades próprias, pois possuem seu próprio filtro personalizado. Isso significa que sua classe se tornou mais do FileTypeque a FileContent. Os dois comportamentos devem ser quer separado, ou combinadas usando a composição (que favorece a sua MyFile2abordagem), ou de preferência ambos (classes separadas para o FileTypee FileContentcomportamento, e, em seguida, tendo ambas compostas na MyFileclasse).


Uma sugestão totalmente diferente.

Tal como está, tanto o seu MyFilee MyFile2existem apenas para dar-lhe um invólucro em torno de seu .Where(l => ...)filtro. Em segundo lugar, você está efetivamente criando uma classe para agrupar um método estático ( File.ReadLines()), o que não é uma ótima abordagem.

Como um aparte, eu não entendo por que você escolheu fazer sua aula sealed. De qualquer forma, eu esperaria que a herança fosse sua maior característica: classes derivadas diferentes com lógica de filtragem diferente (supondo que seja mais complexo que uma simples alteração de valor, porque a herança não deve ser usada apenas para alterar um único valor)

Eu reescreveria toda a sua turma como:

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

A única desvantagem dessa abordagem simplificada é que você precisará repetir o Where()filtro sempre que desejar acessar o conteúdo do arquivo. Eu concordo que isso não é desejável.

No entanto, parece um exagero que, quando você deseja criar uma Where(l => ...)declaração reutilizável , também força essa classe a implementar File.ReadLines(...). Você está agrupando mais do que realmente precisa.

Em vez de tentar agrupar o método estático em uma classe personalizada, acho muito mais adequado se você o envolver em um método estático próprio:

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

Supondo que você tenha filtros diferentes, você pode passar o filtro apropriado como um parâmetro. Vou mostrar um exemplo que pode lidar com vários filtros, que devem ser capazes de lidar com tudo o que você precisa e maximizar a reutilização:

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

E seu uso:

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

Este é apenas um exemplo do moinho que serve para provar que os métodos estáticos fazem mais sentido do que criar uma classe aqui.

Não fique de olho nas especificidades da implementação dos filtros. Você pode implementá-los como quiser (eu pessoalmente gosto de parametrizar Func<>devido à sua extensibilidade e adaptabilidade inerentes à refatoração). Mas como você não fez um exemplo dos filtros que pretende usar, fiz algumas suposições para mostrar um exemplo viável.


ver new X().Y()parece que algo deu errado com o design da classe)

Na sua abordagem, você poderia ter feito isso new X().Ymenos irritante.

No entanto, acho que sua aversão new X().Y()demonstra que você sente que uma classe não se justifica aqui, mas um método; que só pode ser representado sem uma classe sendo estático.

Flater
fonte
Eu realmente aprecio o seu feedback e principalmente o seu pensamento, o que o levou a renomear a classe FileContent. Isso mostra o quão ruim foi o meu exemplo. Também foi muito ruim como eu formulei minha pergunta, que falhou completamente em reunir o tipo de feedback esperado. Eu o editei na esperança de tornar minha intenção mais clara.
manchado
@Flater, mesmo que GetFilteredContent(string filename)seja executado em código com o nome do arquivo na maioria das vezes, eu colocaria o corpo principal do trabalho em um método que levou um Streamou um TextReaderpara facilitar o teste. Então GetFilteredContent(string)seria um invólucro GetFilteredContent(TextReader reader). Mas A concorda com sua avaliação.
Berin Loritsch
4

Na minha opinião, os problemas com ambas as abordagens são:

  1. Você está encapsulando File.ReadLines, o que torna o teste de unidade mais difícil do que precisa,
  2. Uma nova instância de classe deve ser criada cada vez que o arquivo é enumerado, apenas para armazenar o caminho como _filepath.

Então, eu sugiro torná-lo um método estático, que é passado um IEnumerable<string>ou Streamrepresentando o conteúdo do arquivo:

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

Então é chamado via:

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

Isso evita colocar carga desnecessária na pilha e facilita muito o teste de unidade do método.

David Arno
fonte
Concordo com você, no entanto, não era absolutamente o tipo de feedback que eu esperava (minha pergunta foi mal formulada nesse sentido). Veja minha pergunta editada.
manchado
@ Manchado, uau, você está realmente diminuindo as respostas à sua pergunta porque fez a pergunta errada? Isso é baixo.
David Arno
Sim, fiz até perceber que o problema era minha pergunta mal formulada. Só que agora não consigo cancelar minha votação decrescente, desde que sua resposta não seja editada ...: - / Aceite minhas desculpas (ou faça uma simulação da sua resposta para que eu remova com satisfação minha votação decrescente).
manchado
@ Manchado, o problema com sua pergunta agora é que você está pedindo opiniões sobre dois estilos de codificação, tornando a questão fora de tópico. Mesmo com a sua pergunta atualizada, minha resposta permanece a mesma: ambos os designs são falhos pelas duas razões especificadas e, portanto, também não é uma boa solução para mim.
David Arno
Concordo totalmente que o design no meu exemplo é defeituoso, mas não é o ponto da minha pergunta (uma vez que é um exemplo artificial), foi apenas um pretexto para introduzir essas duas abordagens.
Visto
2

O exemplo do mundo real que você forneceu DueInvoicesse presta muito bem ao conceito de que essa é uma coleção de faturas que estão vencidas no momento. Entendo completamente como exemplos planejados podem envolver as pessoas com os termos que você usou versus o conceito que você está tentando transmitir. Eu já estive frustrado várias vezes.

Dito isto, se o objetivo da classe é estritamente ser um IEnumerable<T>e não fornecer nenhuma outra lógica, tenho que fazer a pergunta se você precisa de uma classe inteira ou pode simplesmente fornecer um método para outra classe. Por exemplo:

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

O yield returnfunciona quando você não pode apenas envolver uma consulta LINQ, ou incorporando a lógica é mais fácil de seguir. A outra opção é simplesmente retornar a 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;
    }
}

Nos dois casos, você não precisa de uma classe de wrapper completa. Você só precisa fornecer um método e o iterador é essencialmente tratado por você.

A única vez em que precisei de uma classe completa para lidar com a iteração foi quando tive que extrair blobs de um banco de dados em uma consulta de longa execução. O utilitário era para uma extração única, para que pudéssemos migrar os dados para outro lugar. Houve alguma estranheza que encontrei no banco de dados quando tentei transmitir o conteúdo usando yield return. Mas isso desapareceu quando eu realmente implementei meu costume IEnumerator<T>para controlar melhor quando os recursos foram limpos. Esta é a exceção e não a regra.

Portanto, em resumo, eu recomendo não implementar IEnumerable<T>diretamente se seus problemas puderem ser resolvidos de uma das maneiras descritas no código acima. Salve a sobrecarga de criar o enumerador explicitamente para quando você não puder resolver o problema de outra maneira.

Berin Loritsch
fonte
A razão pela qual eu criei uma classe separada é que InvoiceDtovem da camada de persistência e, portanto, é apenas um pacote de dados, não queria desorganizá-la com o método relevante para os negócios. Daí a criação de DueInvoicee DueInvoices.
manchado
@ Spotted, não precisa ser a própria classe DTO. Heck, provavelmente pode ser uma classe estática com métodos de extensão. Tudo o que estou sugerindo é que, na maioria dos casos, você pode minimizar seu código padrão e tornar a API digerível ao mesmo tempo.
Berin Loritsch
0

Pense em conceitos. Qual é a relação entre um arquivo e seu conteúdo? Essa é uma relação "tem" , não uma relação "é" .

Conseqüentemente, a classe de arquivo deve ter um método / propriedade para retornar o conteúdo. E ainda é conveniente ligar para:

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

foreach(string line in myFile.GetFilteredContents() { ... }
Bernhard Hiller
fonte
Você está certo sobre a relação entre um arquivo e seu conteúdo. O exemplo não foi bem pensado. Fiz algumas edições na minha pergunta original para precisar meus pensamentos.
manchado
Aceite minhas desculpas pelo voto negativo injustificado, no entanto, não posso removê-lo, exceto se você editar sua resposta. : - /
Manchou