8

I am working on some homework and wanted to know if there is such as thing as too many nested while loops. Is there downsides to nesting several while loops? If so how would one refactor the code snippet I have below?

Below is code to read a file one line at a time, parse the fields delimited by some defined delimiters, and remove leading white space before printing to console.

// Read the file one line at a time
while (fgets(lineStr, MAXLINELENGTH, fp) != NULL)
{
    charPtr = strtok(lineStr, DELIMITERS);

    // Loop until line is parsed
    while (charPtr != NULL)
    {
        // Skip past leading whitespace
        while (isspace(*charPtr))
            charPtr++;

        puts(charPtr);
        charPtr = strtok(NULL, DELIMITERS);
    }
}
Greg Hewgill
  • 951,095
  • 183
  • 1,149
  • 1,285
Peter
  • 229
  • 4
  • 10
  • 7
    No, no problem in nested loops. – pmg Apr 25 '12 at 22:49
  • 1
    Although, you may want to encapsulate some of them into methods reflecting what they're doing - that last one becomes `skipSpaces()`, or some such. – Clockwork-Muse Apr 25 '12 at 22:51
  • if the input can contain negative chars (*with values larger than 127*) you might want to cast the argument to `isspace` to avoid undefined behaviour: `isspace((unsigned char)*charPtr)` – pmg Apr 25 '12 at 22:51
  • 1
    Deep nesting can be an indication that you should refactor to split things into different functions. For example, you could have one function that parses out a line, then one that strips off the whitespace, one that tokenizes, etc. Yes that might mean multiple passes over buffers, but if it's not a performance critical use case, cleaner modular code is better. – TJD Apr 25 '12 at 22:53

3 Answers3

8

This is really a rather subjective topic. In my view, there's nothing fundamentally wrong with three nested while loops, but you are reaching the limit of acceptability. Were you to add one or two more levels of nesting then you would, in my view, cross the boundary of what is reasonable to expect a reader to comprehend. The human brain can only handle so much complexity at any one point in time.

Some people would argue, counter to my opinion, that there should be no more than one level of nesting in a function, and that functions should not contain more than around 10 lines of code. The counter argument is that such a policy can result in more fragmented, disjoint code. My rule of thumb is that if you cannot think of a good function name for a chunk of code, then perhaps that chunk of code is not really meant to stand alone as a function.

Looking at ways in which you could break this function up, there are a couple of obvious options.

  1. Extract the body of the outermost while into a separate function. That extracted function would process a single line. It would be easy to name and clear to read.
  2. Extract the while loop that skips whitespace into a separate function. That again would be easy to name and would make your code easier to read. You would remove the whitespace comment because the name of the extracted function would render it needless. That's probably worth doing.

If you applied these ideas then your code might look a little like this:

char* skipWhitespace(char* str)
{
    while (isspace(*str))
        str++;
    return str;
}

void parseLine(char *lineStr)
{
    charPtr = strtok(lineStr, DELIMITERS);
    while (charPtr != NULL)
    {
        charPtr = skipWhitespace(charPtr);
        puts(charPtr);
        charPtr = strtok(NULL, DELIMITERS);
    }
}
......
while (fgets(lineStr, MAXLINELENGTH, fp) != NULL)
    parseLine(lineStr);

Note that the refactoring and naming of extracted methods makes the comments a little superfluous and I removed them. Another good rule of thumb is that if you need to comment code too much, then perhaps it is not yet well-factored.

Ultimately, there really are not hard and fast rules, and it comes down to judgement and personal preference. In my view, the code in the question is very clear and readable, but the refactored version is just a little clearer, in my view.

Disclaimer: I make no comment regarding the correctness or otherwise of the code. I simply ignored that aspect.

David Heffernan
  • 601,492
  • 42
  • 1,072
  • 1,490
  • 2
    I like your 'no name, no function' rule of thumb. Obviously just a guideline, but it brings home a good point - if you can't describe what the function would do, you probably don't need it to be a function! – corsiKa Apr 25 '12 at 23:06
  • Thanks for the refactored code. Will have to ask Prof. if he prefers the code to be as is or would prefer the individual nested loops be refactored into their own functions. I do like the idea of not having comments because the function name is clear enough to convey the idea. – Peter Apr 25 '12 at 23:18
  • Which do you prefer? You should at least have an opinion and be prepared to argue one way or the other when you go to your prof. – David Heffernan Apr 25 '12 at 23:19
  • I prefer would prefer the functions. All the code we have written so far has been contained within one function the test code called, that would explain the nested whiles. I suppose there should not be a problem moving onto a little more sophisticated way of coding. – Peter Apr 25 '12 at 23:30
  • Breaking out some code into dedicated functions will allow you to re-use those functions. But beware, the functions need to be really well formed for re-use to be effective. So `skipWhitespace` would be a candidate for re-use, but `parseLine` must be specific to the current problem only (I guess). – David Heffernan Apr 25 '12 at 23:31
  • I might add that it is quite possible for you to quickly gain better programming technique and style than your professor. That's all I'll state on SO :-) – Jonathon Reinhart Apr 30 '12 at 12:30
3

The only real downside is readability, for which there aren't really any hard and fast rules about... although more than 3 nests will usually irritate anyone else you're working with. As another poster said, sometimes its better to break the nest by moving the loop to another function, but what you have here is perfectly readable to me - and thats the only real metric out there; pure subjective opinion :)

JRaymond
  • 11,625
  • 5
  • 37
  • 40
0

As mentioned earlier, this is relatively subjective. However, the way you nest the loops can have direct performance impacts on your code. Consider cache-aware programming. That is, you want to arrange your code in such a way that the processor can pre-fetch (i.e. predict) the next block of data into cache memory before it needs it. This will allow for more cache hits and faster memory access times.

Note that this is not particularly important for your example, however, if you are making many memory accesses, this can be a significant performance increase or decrease. If you are traversing a multi-dimensional array in a column-wise fashion on a row-major architecture, it is possible to have many cache misses (note that a cache miss is very costly in terms of real-time).

So nesting loops is not necessarily bad, but it can definitely have noticeable effects on performance especially after some arbitrary number n loops.

RageD
  • 6,693
  • 4
  • 30
  • 37