Income is always called

Question

I am currently reading a collection of items from a stream. I do it as follows:

public class Parser{ private TextReader _reader; //Get set in Constructor private IEnumerable<Item> _items; public IEnumerable<Item> Items{ get{ //I >>thought<< this would prevent LoadItems() from being called twice. return _items ?? (_items = LoadItems()); } } public IEnumerable<Item> LoadItems(){ while(_reader.Peek() >= 0){ yield return new Item(_reader.ReadLine()); //Actually it a little different } } } 

Let's say I have a stream that contains two elements, and I do the following:

 var textReader = //Load textreader here var parser = new Parser(textReader); var result1 = parser.Items.Count(); var result2 = parser.Items.Count(); 

Now result1 is 2, and result2 is one.

Now I noticed that my zero check is useless? It seems that every time I call this function, it is inferior anyway.

Can someone explain to me why this is? And what would be the best solution for this situation (please tell me what I'm doing, this is a complete hell: P).

Answer 1

Since LoadItems is a lazy enumeration (uses yield ) and you assign it to a field, this means that every time you enumerate _items , you actually call the loop in LoadItems() to start again, i.e. ( Enumerable.Count creates a new Enumerator each time, which causes the body of the LoadItems to start again). Since you do not re-create the reader each time within the LoadItems , its cursor will be located at the end of the stream, so most likely it will not be able to read more lines. I suspect that it returns null , and your single Item object returned in the second call contains the string null .

Solutions for this would be to "implement" the result of the LoadItems by calling Enumerable.ToList , which will provide you with a specific list:

 return _items ?? (_items = LoadItems().ToList()); 

Or ask the reader to return to the beginning of the stream (if possible) so that LoadItems can run the same way again each time.

But I would recommend that you just get rid of yield ing in this case and return a specific list, since there is little use, so you pay the price of complexity for the lack of winnings.

Answer 2

Your variable name has led you astray. For now:

  private IEnumerable<Item> _items; 

you are lazy-load and save the iterator , while you probably want to be lazy loading and save the elements (as the name of the variable suggests):

 public class Parser{ private TextReader _reader; //Get set in Constructor private List<Item> _items; public IEnumerable<Item> Items{ get{ return _items ?? (_items = LoadItems().ToList()); } } private IEnumerable<Item> LoadItems(){ while(_reader.Peek() >= 0){ yield return new Item(_reader.ReadLine()); //Actually it a little different } } } 

Answer 3

Note that yield used as a short hand. Your code turns into something like:

 private class <>ImpossibleNameSoItWontCollide : IEnumerator<Item> { private TextReader _rdr; /* other state-holding fields */ public <>ImpossibleNameSoItWontCollide(TextReader rdr) { _rdr = rdr; } /* Implement MoveNext, Current here */ } private class <>ImpossibleNameSoItWontCollide2 : IEnumerable<Item> { private TextReader _rdr; /* other state-holding fields */ public <>ImpossibleNameSoItWontCollide2(TextReader rdr) { _rdr = rdr; } public <>ImpossibleNameSoItWontCollide GetEnumerator() { return new <>ImpossibleNameSoItWontCollide(_rdr); } /* etc */ } public IEnumerable<Item> LoadItems() { return new <>ImpossibleNameSoItWontCollide2(_rdr); } 

Therefore, LoadItems() indeed called only once, but the returned object has GetEnumerator() called on it twice.

And since the TextReader moved, it gives the wrong results for you. Although note that this leads to lower memory usage than storage on all elements, so it benefits when you don't want to use the same set of elements twice.

Since you really want to, you need to create an object that stores them:

 return _items = _items ?? _items = LoadItems().ToList();