High maintenance

High maintenance

Rate This
  • Comments 46

The other day I went to buy some snack from the snack machine in the kitchen. The snack I wanted was in slot B-10, so I put in my coins, press B - one - zero, hey wait a minute there's no zero button! And why is it serving me up the snack on the left end of the machine instead of the right? Aha, there is a button marked "10", which is the one I was supposed to press. Instead I got snack B1. How irksome!

And then I laughed at my plight, because of course Steve Maguire told the same story about Microsoft vending machines in Writing Solid Code fifteen years ago. Maguire went on to make an analogy between bad candy machine interfaces and bad software interfaces; a "candy machine interface" is one which leads the caller to make a plausible but wrong choice.

Coincidentally, I was asked to review a fragment of code recently that got me thinking again about candy machine interfaces. This isn't exactly the kind of bad interface that Maguire was talking about -- but I'm getting ahead of myself. Let's take a look at the code:

public static class StreamReaderExtensions
{
    public static IEnumerable<string> Lines(this StreamReader reader)
    {
        if (reader== null)
            throw new ArgumentNullException("reader");
        reader.BaseStream.Seek(0, SeekOrigin.Begin);
        string line;
        while ((line = reader.ReadLine()) != null)
            yield return line;
    }
}

The idea of this code is awesome. I use this technique all the time in my own programs when I need to manipulate a file. Being able to treat a text file as a sequence of lines is very handy. However, the execution of it has some problems.

The first flaw is the bug I discussed in my earlier post on psychic debugging. Namely, the null check is not performed when the method is called, but rather, when the returned enumerator is moved for the first time. That means that the exception isn't thrown until possibly far, far away from the actual site of the error, which is potentially confusing.

The second flaw is that the "while" loop condition is a bit hard to read because it tries to do so much in one line. Shorter code is not magically faster than longer code that does the same thing; write the code so that it is maximally readable.

But there's a deeper flaw here. To get at it, first let me state a crucial fact about the relationship between a stream reader and a stream:

A StreamReader “owns” its underlying stream. That is, once a stream has been handed to a stream reader by a caller, the caller should not muck with the stream ever again. The stream reader will be seeking around in the stream; mucking around with the stream could interfere with the stream reader, and the stream reader will interfere with anyone trying to use the stream. The stream reader emphasizes its ownership by taking responsibility for disposing the stream when the stream reader is itself disposed.

Now, “Lines” defines an object, and what does that object do right off the bat?  It mucks around with the underlying stream. This is big red flag #1. This smells terrible. No one should be messing with that stream but the reader.

Furthermore, think about it from the caller’s point of view. Maybe the caller knows that there are a bunch of bytes that it wants to skip, so it deliberately hands a StreamReader to Lines() which has been positioned somewhere past the beginning of the file. But Lines() thinks that it knows best and ignores the information that the caller has given it. This is big red flag #2.

(The reason why the original code was seeking back was because the same reader was being "recycled" many times to read the same file over and over again, which is yet another red flag. Readers are cheap; you can have multiple readers on one file, there's no need to hoard them and reuse them.)

The third big red flag for me here is the ownership issue. When you hand a stream to a reader, the reader takes care of everything for you from then on – that’s the “contract” between the reader and the caller.  “Lines()” does not have that contract. Lines()’s contract says “attention caller: I am taking ownership of this reader. I am going to muck with its underlying stream. You must never use this reader for anything ever again – except that I’m not going to dispose it for you. If you want the reader and its stream closed then you are going to have to keep a reference to it around until you can prove that I am done with it. And if you get it wrong, either I crash or you get a resource leak. So get it right.”

This is a terrible contract to impose upon a caller, and of course, the imposition here is in no way represented by the signature of the method. You just have to know that “Lines()” owns the reader for the purposes of all its functionality, but not for its cleanup, which is deeply weird.

In short, the caller needs to know all the details of what is happening in the method in order to use it correctly; this is a violation of the whole purpose of creating methods. Methods are supposed to abstract away an operation so that you do not have to know what they are doing.

A fourth red flag is that the task performed by Lines() does not have a clear meaning in terms of the logic of the program. In looking at every caller of this method it became clear that the desired semantics were “give me the lines of this text file one at a time”. But we haven’t written a method that does that. In order to get the lines of a text file from Lines() the caller is required to do all kinds of work to make Lines() happy – open the stream, create a reader, call Lines() but keep the reader around, close the reader after we know that the iterator is done.

This code makes the caller do a whole bunch of things -- things that have to be done in exactly the right order but potentially distributed out over long stretches of time and disparate code locations. This method is very "high-maintenance"! Everything has to be just right all the time in order for it to be happy and get along with others; anything out of place and things will start going wrong.

Finally, we have an example of premature generality here -- if the intention is to read a text file, then write a method that reads a text file. If you don't need the power of reading lines from an arbitrary stream, maybe don't implement that. Cut down on your testing burden.

Some relatively simple changes fix it up:

public static class FileUtilities
{
    public static IEnumerable<string> Lines(string filename)
    {
        if (filename == null)
            throw new ArgumentNullException("filename");
        return LinesCore(filename);
    }
    private static IEnumerable<string> LinesCore(string filename)
    {
        Debug.Assert(filename != null);
        using(var reader = new StreamReader(filename))
        {
            while (true)
            {
                string line = reader.ReadLine();
                if (line == null)
                   yield break;
                yield return line;
            }
        }
    }
}

And now everything is crystal clear. The purpose of this thing is to read the lines out of a text file. The caller gives it the name of a file, it gives you the lines, and we’re done. The caller does not have to worry about anything else – the iterator takes care of opening the file, cleaning up when its done, and so on. There’s no messing around with streams at all; we have now provided an abstraction over the file system to the caller.

Whenever you write a method think about the contract of that method. What burdens are you imposing upon the caller? Are they reasonable burdens?  The purpose of a method should be to make the caller’s life easier; the original version of Lines() makes life harder on the caller. The new version makes life easier. Don't write high-maintenance methods.

  • This is just picking nits, but there may be reasons why you want to give this method a Stream, or a text Reader, instead of a filename.  Obvious examples: a socket, a named pipe, a String, or an in-memory buffer.

    Also, I'm somewhat surprised that, as shown in the original, StreamReader even has a BaseStream property.  If the StreamReader owns its Stream, then why is the Stream even being exposed for the client to muck around with?

  • In the particular case, the code was being used ONLY to read from text files on disk. Better to not be prematurely general, as premature generality got the author of this code into trouble in the first place.

    If you were to design a Lines() method that took an arbitrary reader, it would be a good idea to explicitly document that the returned enumerator owns the reader and will close it when it is done.

    To answer your question -- yes, this is a bit of a leaky abstraction, isn't it?  And quite dangerous.  But one can imagine situations in which code handed a stream reader might want to interrogate the underlying stream for interesting facts about it before blindly calling the streamreader methods on it -- like "just how big is this stream?", so that the code could put up a user interface element saying how much more of the stream there was to be processed, and so on.

  • I agree with Michael.

    - but -

    The stream can also be used in a much better way, simply by adding a using (reader) around the body of the function. Also, I'd remove the seek, and add an XML comment stating that the function returns the rest of the lines up to the end of the stream, then disposes of it.

    I'd also like to point this out: using 'using' in iterator functions is perfectly unsafe. This is the one scenario that eludes the semantics of 'using'. Whenever we use using, we know that when used 'properly', the object will always be disposed as soon as possible. But when will it dispose if we happen to read only halfway through the file, then break out of the foreach loop, or even throw an exception? It seems that the iterator object would still be alive until GC collects it, wouldn't it? And as long as the iterator object is alive, the function will be stuck in mid-execution, meaning that it would not exit the using block and not dispose of the file. Am I missing something here?

  • `using` within iterators is safer than you give it credit for.

    If someone uses `foreach' and breaks out of the loop early, the `using' will still work.

    If an exception is generated from the `foreach', the `using' will still work.

    The reason both of these work is because IEnumerator<T> implements IDisposable, and the generated iterator .Dispose() will execute the `finally' block of the `using' statement.  Try it; it works.

    The only time that `using' won't work is if you (1) forego `foreach` (i.e. manually reimplement foreach), AND (2) forget to .Dispose() of the iterator.

    Doing both (1) and (2) is possible, but I would suggest that it's not very likely.

  • Hello Eric,

    thanks much - as always, very interesting post.

    it would be great if you can explain 2 points:

    1- why do you have a private & public interfaces?

    2- and I didn't get your first objection (Namely, the null check is not performed when the method is called)?

    how is the check for null different between original and corrected implementations?

  • to configurator:

    yes, you are. I asked a very same question, but looked to IL first. IEnumerator<T> is IDisposable, so you should dispose it by contract when you are done and foreach does it correctly. Even though non generic IEnumerator is not IDisposable (BCL flaw), for foreach the compiler does a good job by checking if it 'is' IDisposable.

    Kosta

  • Jonathan is correct. Remember, foreach(A b in c) D(b); is actually codegenned as though you had written something like:

    {
      E e = c.GetEnumerator();
      try
      {
        while(e.MoveNext()) { b = (A)e.Current; D(b); }
      }
      finally
      {
        e.Dispose();
      }
    }

    Disposing an enumerator causes any not-yet-run finally blocks in the iterator block to run.

    So in both the cases you describe, the reader would be disposed immediately upon prematurely leaving the foreach, whether via exception, break, goto, or return.

  • Muhammad:

    1) The public method checks its argument. The private method does not.  Don't expose two methods which do the same thing, but where one is safe and the other is dangerous.

    2) Since the iterator block does not run until the first MoveNext, the check for null doesn't happen until possibly long after the call to Lines().  You want the check to happen immediately so that if it is going to fail, it fails at the earliest possible point.  That is, consider the original code called like this:

    StreamReader reader = null;
    var lines = reader.Lines(); // you expect it to throw here
    // ...
    foreach(var line in lines) // but it actually throws here, because the iterator block doesn't run until the first MoveNext. This could be in completely different code and long after the call to Lines(), which is confusing.

  • Now, in the interests of full disclosure, note that this is "unsafe" in the sense that we have many times shipped bugs where the codegen for some cases does not correctly run the finally blocks when something disposes of an iterator halfway through.  The codegen for these cases is the single hardest part of the compiler and we've screwed it up in the past. For example, there were cases with nested finally blocks where the finally blocks would run in the wrong order.

    If anyone finds such bugs in the service release of the compiler, PLEASE tell me about them.

  • Wow - you managed to say all of that without once mentioning 'invariant' :-)

    BTW - any reason for inferring the type of 'reader' using 'var', while you declared 'line' directly as a string? What reasoning did you have for declaring the two variables in different ways?

  • RichB: It is dead obvious from the initialization of reader what the type will be. The "new StreamReader" sitting right there tells you what the type is. Stating the type is completely redundant for reader.

    But it is not obvious from just the initialization of "line" that it will be a string. In that case you have to know what type ReadLine returns. By stating "string" explicitly it makes it obvious to the reader that my intention was for this thing to be a string.

  • As for the original design of Lines(), my biggest beef is that it mucks around with .BaseStream, which is evil (for all the reasons mentioned).

    My 2nd beef, though, isn't mentioned: .Lines() is on StreamReader.

    It _should_ be on TextReader, which would allow us to do:

    foreach (var line in Console.In.Lines ()) { ... }

    Which just seems perfectly natural to me.

    Which further makes things more complicated about the `ownership` argument: .Lines() certainly should NOT be disposing of Console.In when it's complete, as Console.In is managed by the runtime.

    Which says to me that .Lines() shouldn't "own" the TextReader, at least not by default.

    I'm also not sure it's a "leaky abstraction," either, as all .Lines() is documented to do is extract lines of text from the input, NOT to actually own the input (as it may be ideal to reuse the existing StreamReader instead of creating a new one).

    (As for "Readers are cheap," I'm not entirely sure of that, unless you also argue that HANDLEs are cheap.  Since a StreamReader owns the underlying Stream, and a Stream is a kernel HANDLE to a file, by definition multiple readers will require multiple kernel HANDLEs.  Plus you'll need to provide the appropriate flags to the FileStream constructor, lest you only be able to have *one* FileStream open for that file (the FileShare enumeration), which would kill your "multiple readers" defense rather quickly...)

    I think there is one problem most apparent, though: FileUtilities.Lines() shouldn't be necessary.  In particular, File.ReadAllLines() should have done this in the first place!  Who's brilliant idea was it to make File.ReadAllLines() return a string[]?  That design decision restricts any actual improvement in that method, AND requires that the entire file be loaded into memory at once.  Brillant!

  • Really nice rewrite of the original code. I'd be proud if I'd done the same.

  • Well, again, it comes down to "premature generalization is evil". The purpose of the code was "open a bunch of files for reading and read them in one line at a time", and when you write code that implements something more general than that, then you have all the design, implementation and testing burden of ensuring that your system works with ANYTHING that can be a TextReader instead of merely anything that can be a file name.  

    If you wanted this system to work on arbitrary text readers then you would need to decide and document who owns the thing and whether or not it is closed when you're done reading; if you need to impose the tax of the caller keeping ownership of the reader then that's a consequence of having to solve the more general problem. Generality is expensive, and therefore to be avoided if it cannot be made cheap.

    As for the design flaw in "lines" in the first place -- you will get no argument from me. I have a separate rant queued up about methods that return arrays. However, keep in mind that this method was invented before generic types were added to the CLR. Obviously today you would use IEnumerable<string>, but in CLR v1, that was not an option.

  • The method may have been *designed* before CLR v2.0 (who's to say for sure outside of Microsoft?), but it wasn't *introduced* until .NET 2.0:

    http://msdn.microsoft.com/en-us/library/s2tte0y1.aspx

    "Supported in: 3.5, 3.0, 2.0"

    Even if it were in 1.0/1.1, it could have returned an IEnumerable, which would allow future alteration of the method's implementation, as opposed to (effectively) leaking implementation details to one and all.  Alas, this wouldn't be strongly typed, but it would allow migrating the implementation from an array-based to a C# iterator based implementation.  The only downside is you lose type safety. :-(

    (You also lose array indexing, which makes it ~trivial to write out a line along with it's corresponding line number.  Providing the line number is easy without arrays, but not _as_ easy.)

    (I think this would also be in keeping with the FxDG: Section 5.7 says "DO use the least derived parameter type that provides the functionality required by the member."  A return type isn't a parameter, but I believe the the same basic idea holds true: return the type that provides you the greatest future flexibility.  string[] has _no_ future flexibility, while IEnumerable does.)

    iirc, there are several other methods added in 2.0 which return non-generic types where it really would have made more sense.  It seems that not everyone got the `C# iterator` love memo (while LINQ is predecated upon lots of C# iterator love, so I guess that makes up for it).

    (Furthermore, many v1.0 types haven't been retrofited to support the v2.0 generic interfaces, e.g XmlNodeList.)

Page 1 of 4 (46 items) 1234