9

I often find myself needing to implement a class which is an enumeration/collection of something. Consider for this thread the contrived example of IniFileContent which is an enumeration/collection of lines.

The reason this class must exist in my codebase is that I want to avoid business logic being spread all over the place (=encapsulating the where) and I want to do it in the most object-oriented possible way.

Usually I would implement it as below:

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

I choose to implement IEnumerable<string> because it makes its usage convenient:

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

However I'm wondering if doing so "shadows" the class intent ? When one looks at IniFileContent's interface one only see Enumerator<string> GetEnumerator(). I think it makes non-obvious which service the class is actually providing.

Consider then this second implementation:

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

Which is used less conveniently (by the way, seeing new X().Y() feels like something went wrong with the class design):

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

But with a clear interface IEnumerable<string> Lines() making obvious what this class can actually do.

Which implementation would you foster and why ? Implied, is it a good practice to implement IEnumerable to represent an enumeration of something ?

I'm not looking for answers on how to:

  • unit test this code
  • make a static function instead of a class
  • make this code more prone to future business logic evolution
  • optimize performance

Appendix

Here is the kind of real code living in my codebase which implements IEnumerable

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();
}
6
  • 2
    Related stackoverflow.com/questions/21692193 – enkryptor Jul 16 '18 at 11:25
  • 2
    Voting to close this as the updated question is asking for opinions on two coding styles and so is now completely opinion-based. – David Arno Jul 16 '18 at 13:40
  • 1
    @DavidArno I disagree in the sense that asking if something is a good practice is not not entirely based on opinion but also on facts, experience and norms. – Spotted Jul 16 '18 at 13:58
  • Neither pattern works for me: dependency on concrete classes, hard dependency on file system, ambiguous disposal conventions (I believe you may have a leak, sir), counterintuitive semantics of using new for this use case, difficulty in unit testing, ambiguous when I/O exceptions can occur, etc. I'm sorry, I'm not trying to be rude. I think I have gotten too used to the benefits of dependency injection. – John Wu Jul 17 '18 at 10:20
  • @JohnWu Please consider this is a contrived example (clumsily chosen I confess). If you wish to answer this question (not trying to be rude either) consider focusing on the design decision of implementing IEnumerable or not for a class being an IniFileContent. – Spotted Jul 17 '18 at 10:23
14

I'm reviewing your approach before suggesting a wholly different approach. I'm preferring the different approach but it seems important to explain why your approach has flaws.


I choose to implement IEnumerable<string> because it makes its usage convenient

Convenience should not outweigh correctness.

I wonder if your MyFile class will contain more logic than this; because that will affect the correctness of this answer. I'm especially interested in:

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

because if this is complex or dynamic enough, you're hiding that logic in a class whose name does not reveal that it filters it content before serving it to the consumer.
Part of me hopes/assumes that this filter logic is intended to be hardcoded (e.g. a simple filter that ignores commented lines, e.g. like how .ini files consider lines starting with # to be a comment) and not a file-specific ruleset.


public class MyFile : IEnumerable<string>

There is something really grating about having a singular (File) represent a plural (IEnumerable). A file is a singular entity. It consist of more than just its content. It also contains metadata (filename, extension, creation date, modify date, ...).

A human is more than the sum of its children. A car is more than the sum of its parts. A painting is more than a collection of paints and canvas. And a file is more than a collection of lines.


If I assume that your MyFile class will never contain more logic than just this enumeration of lines (and the Where only applies a simple static hardcoded filter), then what you've got here is a confusing usage of the "file" name and its intended designation. This can easily be fixed by renaming the class as FileContent. It retains your intended syntax:

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

It also makes more sense from a semantical point of view. The content of a file can be broken into separate lines. This still assumes that the file's content is text and not binary, but that's fair enough.


If, however, your MyFile class will contain more logic, the situation changes. There are a few ways this could happen:

  • You start using this class to represent the file's metadata, not just its contents.

When you start doing this, then the file represents the file in the directory, which is more than just its content.
The correct approach here is then what you've done in MyFile2.

  • The Where() filter starts having complicated filter logic that is not hardcoded, e.g. when different files start being filtered differently.

When you start doing this, files start having identities of their own, as they have their own custom filter. This means that your class has more become a FileType than a FileContent. The two behaviors need to be either separated, or combined using composition (which favors your MyFile2 approach), or preferably both (separate classes for the FileType and FileContent behavior, and then having both composed into the MyFile class).


A wholly different suggestion.

As it stands, both your MyFile and MyFile2 exist purely to give you a wrapper around your .Where(l => ...) filter. Secondly, you're effectively creating a class to wrap a static method (File.ReadLines()), which is not a great approach.

As an aside, I don't get why you've chose to make your class sealed. If anything, I'd expect that inheritance would be its biggest feature: different derived classes with different filtering logic (assuming that it's more complex than a simple value change, because inheritance shouldn't be used just to change a single value)

I would rewrite your whole class as:

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

The only downside to this simplified approach is that you'd have to repeat the Where() filter every time you wish to access the file's contents. I agree that that is not desirable.

However, it seems overkill that when you want to create a reusable Where(l => ...) statement, that you then also force that class to implement File.ReadLines(...). You're bundling more than you really need.

Instead of trying to wrap the static method in a custom class, I think it's much more fitting if you wrap it in a static method of its own:

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

Assuming you have different filters, you could pass the appropriate filter as a a parameter. I'll show you an example that can handle multiple filters, which should be able to handle everything you need it to do while maximizing reusability:

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

And its usage:

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

This is just a run of the mill example that's meant to prove the point that static methods make more sense than creating a class here.

Don't get caught up on the specifics of implementing the filters. You can implement them however you want (I just personally like parametrizing Func<> because of its inherent extensibility and adaptability to refactoring). But since you didn't actually an example of the filters you intend to use, I made a few assumption to show you a workable example.


seeing new X().Y() feels like something went wrong with the class design)

In your approach, you could've made it new X().Y which is less grating.

However, I think that your dislike of new X().Y() proves the point that you feel like a class is not warranted here, but a method is; which can only be represented without a class by being static.

2
  • I really appreciate your feedback and mostly your thinking which lead you to rename the class to FileContent. It shows how bad my example was. Was also really bad how I formulated my question which has completely failed to gather the kind of feedback I was expected. I have edited it in hope to make my intent more clear. – Spotted Jul 16 '18 at 13:34
  • @Flater, even if GetFilteredContent(string filename) is executed in code with the filename most of the time, I would put the main body of the work in a method that took a Stream or a TextReader so that it makes testing a lot easier. So GetFilteredContent(string) would be a wrapper around GetFilteredContent(TextReader reader). But A agree with your assessment. – Berin Loritsch Jul 17 '18 at 12:15
4

To my mind, the issues with both approaches are:

  1. You are encapsulating File.ReadLines, which makes unit testing harder than it need be,
  2. A new class instance has to be created each time the file is enumerated, just to store the path as _filepath.

So I'd suggest making it a a static method, that is either passed an IEnumerable<string> or Stream representing the file contents:

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

Then it is called via:

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

This avoids putting unnecessary load on the heap and makes it far easier to unit test the method.

5
  • I agree with you, however it was absolutely not the kind of feedback I was expected (my question was poorly formulated in that sense). See my edited question. – Spotted Jul 16 '18 at 13:35
  • @Spotted, wow, are you really downvoting answers to your question because you asked the wrong question? That's low. – David Arno Jul 16 '18 at 13:39
  • Yes I did until I realized the problem was my poorly-formulated question. Except that now I'm unable to uncast my downvote as long as your answer is not edited... :-/ Please accept my apologizes (or dummy edit your answer so that I gladly remove my downvote). – Spotted Jul 16 '18 at 13:44
  • @Spotted, the problem with your question now is that you are asking for opinions on two coding styles, making the question off-topic. Even with your updated question, my answer remains the same: both designs are flawed for the two reasons I specify and therefore neither is a good solution in my view. – David Arno Jul 16 '18 at 13:50
  • I totally agree that the design in my example is flawed but it's not the point of my question (since it is a contrived example), it was just a pretext to introduce both these two approaches. – Spotted Jul 16 '18 at 14:00
2

The real world example you provieded, DueInvoices lends itself very well to the concept that this is a collection of invoices that are currently due. I understand completely how contrived examples can get people wrapped up around the terms you used vs. the concept you are trying to convey. I've been on the frustrating end of that myself multiple times.

That said, if the purpose of the class is strictly to be an IEnumerable<T>, and doesn't provide any other logic, I have to ask the question whether you need a whole class or can simply provide a method off of another class. For example:

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

The yield return works when you can't just wrap a LINQ query, or embedding the logic is easier to follow. The other option is simply to return the LINQ query:

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

In both of these cases you don't need a full wrapper class. You just need to provide a method and the iterator is essentially handled for you.

The only time where I needed a full class to handle the iteration was when I had to pull blobs out of a database in a long running query. The utility was for a one time extraction so we could migrate the data elsewhere. There was some weirdness I encountered with the database when I tried to stream the content out using yield return. But that went away when I actually implemented my custom IEnumerator<T> to better control when resources were cleaned up. This is the exception rather than the rule.

So in short, I recommend not implementing IEnumerable<T> directly if your problems can be solved in either of the ways outlined in code above. Save the overhead of creating the enumerator explicitly for when you cannot solve the problem any other way.

2
  • The reason why I created a separate class is that InvoiceDto comes from the persistance layer and thus is just a bag of data, I didn't want to clutter it with business-relevant method. Hence the creation of DueInvoice and DueInvoices. – Spotted Jul 17 '18 at 6:05
  • @Spotted, It doesn't have to be the DTO class itself. Heck, it can probably be a static class with extension methods. All I'm suggesting is that in most cases you can minimize your boilerplate code, and make the API digestible at the same time. – Berin Loritsch Jul 17 '18 at 12:11
0

Think of concepts. What is the relation between a file and its contents? That's a "has"-relation, not an "is"-relation.

Consequently, the file class should have a method/property for returning the contents. And that's still convenient to call:

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

foreach(string line in myFile.GetFilteredContents() { ... }
2
  • You are right about the relation between a file and its content. The example was not well thought out. I did some edits to my original question to precise my thoughts. – Spotted Jul 16 '18 at 13:37
  • Accept my apologies for the unjustified downvote, however I'm not able to remove it except if you edit your answer. :-/ – Spotted Jul 16 '18 at 13:45

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.