Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

When you have to iterate a reader where the number of items to read is unknown, and the only way to do is it to keep reading until you hit the end.

This is often the place you need an endless loop.

  1. There is the always true that indicates there must be a break or return statement somewhere inside the block.

    int offset = 0;
    while(true)
    {
        Record r = Read(offset);
        if(r == null)
        {
            break;
        }
        // do work
        offset++;
    }
    
  2. There is the double read for loop method.

    Record r = Read(0);
    for(int offset = 0; r != null; offset++)
    {
        r = Read(offset);
        if(r != null)
        {
            // do work
        }
    }
    
  3. There is the single read while loop. Not all languages support this method.

    int offset = 0;
    Record r = null;
    while((r = Read(++offset)) != null)
    {
        // do work
    }
    

I'm wondering which approach is the least likely to introduce a bug, most readable and commonly used.

Every time I have to write one of these I think "there has to be a better way".

share|improve this question
2  
Why are you maintaining an offset? Don't most Stream Readers allow you to simply "read next?" –  Robert Harvey Dec 25 '13 at 17:11
    
@RobertHarvey in my current need the reader has an underlying SQL query that uses the offset to paginate results. I don't know how long the query results are until it returns an empty result. But, the for the question it's not really a requirement. –  Mathew Foscarini Dec 26 '13 at 0:34
3  
You seem confused - the question title is about endless loops, but the question text is all about terminating loops. The classical solution (from the days of structured programming) is to do a pre-read, loop while you have data, and read again as the last action in the loop. It's simple (meeting the bug requirement), the most common (since it's been written for 50 years). Most readable is a matter of opinion. –  andy256 Dec 26 '13 at 3:47
    
@andy256 confused is pre-coffee condition for me. –  Mathew Foscarini Dec 26 '13 at 14:30
1  
Ah, so the correct procedure is 1) Drink coffee while avoiding keyboard, 2) Begin coding loop. –  andy256 Dec 26 '13 at 20:58

4 Answers 4

up vote 46 down vote accepted
+50

I would take a step back here. You're concentrating on the picky details of the code but missing the larger picture. Let's take a look at one of your example loops:

int offset = 0;
while(true)
{
    Record r = Read(offset);
    if(r == null)
    {
        break;
    }
    // do work
    offset++;
}

What is the meaning of this code? The meaning is "do some work to each record in a file". But that is not what the code looks like. The code looks like "maintain an offset. Open a file. Enter a loop with no end condition. Read a record. Test for nullity." All that before we get to the work! The question you should be asking is "how can I make this code's appearance match its semantics?" This code should be:

foreach(Record record in RecordsFromFile())
    DoWork(record);

Now the code reads like its intention. Separate your mechanisms from your semantics. In your original code you mix up the mechanism -- the details of the loop -- with the semantics -- the work done to each record.

Now we have to implement RecordsFromFile(). What's the best way of implementing that? Who cares? That's not the code that anyone is going to be looking at. It's basic mechanism code and its ten lines long. Write it however you want. How about this?

public IEnumerable<Record> RecordsFromFile()
{
    int offset = 0;
    while(true)
    {
        Record record = Read(offset);
        if (record == null) yield break;
        yield return record;
        offset += 1;
    }
}

Now that we are manipulating a lazily computed sequence of records all sorts of scenarios become possible:

foreach(Record record in RecordsFromFile().Take(10))
    DoWork(record);

foreach(Record record in RecordsFromFile().OrderBy(r=>r.LastName))
    DoWork(record);

foreach(Record record in RecordsFromFile().Where(r=>r.City == "London")
    DoWork(record);

And so on.

Any time you write a loop, ask yourself "does this loop read like a mechanism or like the meaning of the code?" If the answer is "like a mechanism", then try to move that mechanism to its own method, and write the code to make the meaning more visible.

share|improve this answer
3  
+1 finally a sensible answer. This is exactly what I'll do. Thanks. –  Mathew Foscarini Dec 26 '13 at 22:33
1  
"try to move that mechanism to its own method" -- that sounds much like Extract Method refactoring doesn't it? "Turn the fragment into a method whose name explains the purpose of the method." –  gnat Dec 27 '13 at 8:19
2  
@gnat: What I'm suggesting is slightly more involved than "extract method", which I think of as merely moving one hunk of code to another place. Extracting methods is definitely a good step in making code read more like its semantics. I'm suggesting that method extraction be done thoughtfully, with an eye towards keeping policies and mechanisms separated. –  Eric Lippert Dec 27 '13 at 17:45
1  
@gnat: Exactly! In this case the detail that I want to extract is the mechanism by which all records are read from the file, while maintaining the policy. The policy being "we have to do some work on every record". –  Eric Lippert Dec 27 '13 at 21:32
1  
I see. That way, it's easier to read and maintain. Studying this code, I can focus separately on policy and mechanism, it doesn't force me to split attention –  gnat Dec 28 '13 at 12:40

You don't need an endless loop. You should never need one in C# read scenarios. This is my preferred approach, assuming that you really do need to maintain an offset:

Record r = Read(0);
offset=1;
while(r != null)
{
    // Do work
    r = Read(offset);
    offset++
}

This approach acknowledges the fact that there is a setup step for the reader, so there are two read method calls. The while condition is at the top of the loop, just in case there is no data at all in the reader.

share|improve this answer

Well it depends on your situation. But one of the more "C#-ish" solutions I can think of is to use the built-in IEnumerable interface and a foreach loop. The interface for IEnumerator only calls out MoveNext with true or false, so size can be unknown. Then your termination logic is written once - in the enumerator - and you don't have to repeat in more than one spot.

MSDN provides an example of IEnumerator< T >. You will also need to create an IEnumerable< T > to return the IEnumerator< T >.

share|improve this answer
    
Can you give a code example. –  Mathew Foscarini Dec 25 '13 at 21:14
    
thanks, this is what I've decided to do. While I don't think creating new classes each time you have an endless loop solves the question. –  Mathew Foscarini Dec 26 '13 at 0:36
    
Yeah, I know what you mean - a lot of overhead. The real genius/problem of iterators is that they are definitely set up to be able to have two things iterate over a collection at the same time. So many times you don't need that functionality so you could just have the wrapper around the objects implement both IEnumerable and IEnumerator. Another way of looking at is that whatever underlying collection of stuff you're iterating on wasn't designed with the accepted C# paradigm in mind. And that's OK. An added bonus of iterators though is that you can get all the parallel LINQ stuff for free! –  J Trana Dec 26 '13 at 2:18

When I have initialization, condition and increment operations I like to use the for loops of languages like C, C++ and C#. Like this:

for (int offset = 0, Record r = Read(offset); r != null; r = Read(++offset)){
    // loop here!
}

Or this if you think this is more readable. I personally prefer the first one.

for (int offset = 0, Record r = Read(offset); r != null; offset++, r = Read(offset)){
    // loop here!
}
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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