High maintenance
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.
Comments
Anonymous
September 08, 2008
The comment has been removedAnonymous
September 08, 2008
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.Anonymous
September 08, 2008
The comment has been removedAnonymous
September 08, 2008
The comment has been removedAnonymous
September 08, 2008
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?Anonymous
September 08, 2008
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. KostaAnonymous
September 08, 2008
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.Anonymous
September 08, 2008
The comment has been removedAnonymous
September 08, 2008
The comment has been removedAnonymous
September 08, 2008
The comment has been removedAnonymous
September 08, 2008
The comment has been removedAnonymous
September 08, 2008
The comment has been removedAnonymous
September 08, 2008
Really nice rewrite of the original code. I'd be proud if I'd done the same.Anonymous
September 08, 2008
The comment has been removedAnonymous
September 08, 2008
The comment has been removedAnonymous
September 08, 2008
I agree with the general sentiment. But I do take issue with a couple of comments: " No one should be messing with that stream but the reader." Well, for better or worse, .NET is specifically advertising this as a feature. The StreamReader provides no API itself to manage position within the stream, does provide the BaseStream property as well as the DiscardBufferedData method. The intent of the contract for StreamReader is clearly to allow random-access to the stream, and to do so by virtue of allowing the client of the StreamReader to manage the position, rather than having the StreamReader itself do it. I suppose you could argue that the .NET design is flawed. But it is what it is, and when used carefully there's nothing inherently wrong with this use of StreamReader. Also: "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." As an interface goes, imperfect perhaps. But I don't see anything fundamentally wrong with the contract. At worst, perhaps the method should have been named "AllLines" instead. The author could even have provided an additional "RemainingLines". I do agree that the proposed fully-encapsulated version is much better, and that by hiding the stream and the reader altogether, it fully avoids interference and strange side-effects from the client code, which is a good thing. But coding isn't black and white, and I think there's room for imperfect implementations, when used appropriately.Anonymous
September 08, 2008
If you take a file name instead of an open file, your contract now includes a significant number of IO and security exceptions which could be raised...Anonymous
September 08, 2008
And as an added bonus, the FileUtilities.Lines(string) version is harder to unit test, as it requires that a well-known file exist in a well-known location. The .Lines(StreamReader) suffers from the same problem. A .Lines(TextReader) version is quite easy to unit test, as you can pass it a StringReader to verify that it behaves the way you think it behaves (as long as StringReader and StreamReader follow the TextReader contract, which they should do...).Anonymous
September 08, 2008
Another example of how not to use extension methods may be? As for comments about Eric's implmentation taking a filename (path) and not a stream based reader - Eric's version encapsulates the file manipulation completely and abstracts the caller away from knowing how the file is being manipulated. Also I don't see any issue with unit testing this either...Anonymous
September 08, 2008
There's another reason why the original code did a seek to 0. If it didn't, then the following piece of code would have printed false. var lines = someStreamReader.Lines(); Console.WriteLine(lines.First() == lines.First()) All constructed enumerators would have referenced the same stream, so advancing one would have advanced all of them. Eric's implementation avoids the problem by constructing a new StreamReader inside the method.Anonymous
September 08, 2008
> " No one should be messing with that stream but the reader." Or to put it another way - the "Inappropriate intimacy" Code Smell. http://en.wikipedia.org/wiki/Code_smellAnonymous
September 09, 2008
The comment has been removedAnonymous
September 09, 2008
Indeed, I would love to have anonymous iterators.Anonymous
September 09, 2008
The comment has been removedAnonymous
September 09, 2008
The comment has been removedAnonymous
September 09, 2008
"Indeed, I would love to have anonymous iterators." <drool> Me too! Me too! :)Anonymous
September 09, 2008
The comment has been removedAnonymous
September 09, 2008
The comment has been removedAnonymous
September 09, 2008
I think by virtue of being an extension method the original has no obligation to clean up. I wouldn't expect a call to theStream.Method() to dispose of the stream. Of course that doesn't discount the issue of altering the stream, but without that I think the contract is then perfectly clear. (For a precedence see ReadToEnd) Though I would agree about not being too generic from the beginning.Anonymous
September 10, 2008
I am surprised no one commented on the name of the method. "Lines" ... lines what? That tells me nothing about what the method does, I can only guess it by looking at its signature. I am a big proponent of method names being verbs and class names being nouns. I also agree with some others that the method should work on TextReader. For me, the version using filename is just icing on the cake as it is probably the most common operation. Anyway, it all depends on the context where this class is used. "[...] you have all the design, implementation and testing burden of ensuring that your system works with ANYTHING that can be a TextReader [...]" Can you elaborate on that? As long as implementations of a TextReader respect its contract, I don't understand why you would impose on yourself such a burden, so I am curious to know your reasons for saying that.Anonymous
September 10, 2008
What's wrong with this code, part IIAnonymous
September 11, 2008
The comment has been removedAnonymous
September 12, 2008
I disagree with the "ownership" idea in this case and in many Stream scenarios similar to this. BinaryReader "correctly" takes ownership of the stream and closes the stream when you're done with the BinaryReader. But sometimes you just need BinaryReader functionality for a subset of the stream. If that's you're secenario you're out of luck--you have to reimplement BinaryReader! That's the greater of the evils.Anonymous
September 12, 2008
The comment has been removedAnonymous
September 14, 2008
The comment has been removedAnonymous
September 17, 2008
The comment has been removedAnonymous
September 22, 2008
Excellent discussion. Please don't let the pointy-haired boss have the last word.Anonymous
September 22, 2008
You've been kicked (a good thing) - Trackback from DotNetKicks.comAnonymous
September 23, 2008
Somehow my comment was lost. I like your solution so much that I documented it as a pattern, see community content at http://msdn.microsoft.com/en-us/library/65zzykke.aspx cheersAnonymous
September 24, 2008
Nice, thanks!Anonymous
September 30, 2008
A StreamReader / StreamWriter taking ownership of a Stream just seems wrong. Methods and classes should not be responsible for cleaning up resources that they didn't create, unless this is clearly documented and obvious. For example, the following looks like it should work, but fails unexpectedly with an ObjectDisposedException on the call to Seek: using (var s = new MemoryStream(capacity)) { using (var writer = new StreamWriter(s)) { Render(writer); } s.Seek(0L, SeekOrigin.Begin); using (var reader = new StreamReader(s)) { return reader.ReadToEnd(); } } Since the caller created the Stream, it should be the caller's responsibility to close it.Anonymous
September 30, 2008
In this carnival there're a lot of software design/patterns and frameworks a bit of SOA, UML, DSLAnonymous
November 11, 2008
I know this is an old topic but I'm really confused regarding why a reader or writer should close the stream itself.. If I had to create a reader or writer in a way that the reader had to aquire the stream itself, then I would expect the reader to handle cleaning it.. but personally I don't see the readers job as aquiring the stream.. I see that as maybe helper methods. It makes completely sense that the solution should be using(var stream = .. aquire stream ..) { using(var reader = new StreamReader(stream)) { .. do stuff.. } stream.Close(); } I certainly wouldn't be expecting the stream to close the stream.. the reader didn't get the stream, I gave it the stream, so I will dispose the stream as well, thats my contract in the code... It seems to go against seperation of concerns, reader becomes somewhat of a 'jack of all trades'.. hey guy don't worry - I'll close the stream when you're done, because thats generally what you want no? The code I wrote makes much more sense.. whos doing what.. I'm getting a stream.. I'm reading over the stream.. I'm closing/disposing the stream.. I agree changing .net now is impossible, the amount of code written thats now expecting this behavior.. but I'd go as far as classifying this in the relms of the clr array.. oh did you guys know i do unsafe invariance as covariance as well? (echoing off) nooooooooooooo!Anonymous
February 04, 2009
The other day my charming wife Leah and I were playing Scrabble Brand Crossword Game (a registered trademarkAnonymous
March 10, 2009
The comment has been removedAnonymous
September 27, 2012
- I see this problem all the time in managed code, you hand over a reference of an object to a method and expect it to do something (say, call only the properties in that object to read data and not call any other method), and the callee better adhere to that expectation. But that breaks the abstraction between the caller and callee. The only way I have found out of this is to clone the object and pass it on, or create a read only value object and pass it along.
- Do you think this is important enough for internal methods as well - for public, definitely, how about for others? Say, for private methods within the same class and internal methods within the same assembly?