How to structure a loop that repeats until success and handles failures

Question

I am a self-taught programmer. I started programming about 1.5 years ago. Now I have started to have programming classes in school. We have had programming classes for 1/2 year and will have another 1/2 right now.

In the classes we are learning to program in C++ (which is a language that I already knew to use quite well before we started).

I have not had any difficulties during this class but there is one recurring problem that I have not been able to find a clear solution to.

The problem is like this (in Pseudocode):

 do something
 if something failed:
     handle the error
     try something (line 1) again
 else:
     we are done!

Here is an example in C++

The code prompts the user to input a number and does so until the input is valid. It uses cin.fail() to check if the input is invalid. When cin.fail() is true I have to call cin.clear() and cin.ignore() to be able to continue to get input from the stream.

I am aware that this code does not check of EOF. The programs we have written are not expected to do that.

Here is how I wrote the code in one of my assignments in school:

for (;;) {
    cout << ": ";
    cin >> input;
    if (cin.fail()) {
        cin.clear();
        cin.ignore(512, '\n');
        continue;
    }
    break;
}

My teacher said was that I should not be using break and continue like this. He suggested that I should use a regular while or do ... while loop instead.

It seems to me that using break and continue is the simplest way to represent this kind of loop. I actually thought about it for quite a while but did not really come up with a clearer solution.

I think that he wanted me to do something like this:

do {
    cout << ": ";
    cin >> input;
    bool fail = cin.fail();
    if (fail) {
        cin.clear();
        cin.ignore(512, '\n');
    }
} while (fail);

To me this version seems alot more complex since now we also have variable called fail to keep track of and the check for input failure is done twice instead of just once.

I also figured that I can write the code like this (abusing short circuit evaluation):

do {
    cout << ": ";
    cin >> input;
    if (fail) {
        cin.clear();
        cin.ignore(512, '\n');
    }
} while (cin.fail() && (cin.clear(), cin.ignore(512, '\n', true);

This version works exactly like the other ones. It does not use break or continue and the cin.fail() test is only done once. It does however not seem right to me to abuse the "short circuit evaluation rule" like this. I do not think my teacher would like it either.

This problem does not only apply to just cin.fail() checking. I have used break and continue like this for many other cases that involve repeating a set of code until a condition is met where something also has to be done if the condition is not met (like calling cin.clear() and cin.ignore(...) from the cin.fail() example).

I have kept using break and continue throughout the course and now my teacher has now stopped complaining about it.

What are your opinions about this?

Do you think my teacher is right?

Do you know a better way to represent this kind of problem?

You aren't looping forever, so why are you using something that implies you are looping forever? A loop forever is not always a bad choice (although it usually is) in this case it is obviously a bad choice because it communicates the wrong intent of the code. The second method is closer to the intent of the code, so it is better. The code in the second snippet could be made a bit more tidy and easier to read with some minimal reorganization. — Dunk, Jan 08 '15 at 22:43
@Dunk That seems a bit dogmatic. It's actually very rare to need to loop forever, other than at the top level of interactive programs. I see `while (true)` and instantly assume there's a `break`, `return`, or `throw` somewhere in there. Introducing an extra variable that needs to be kept track of just to dodge a `break` or `continue` is counterproductive. I'd argue the problem with the initial code is that the loop *never* finishes an iteration normally, and that you need to know there's a `continue` inside the `if` to know the `break` won't be taken. — Doval, Jan 09 '15 at 00:07
@Doval:Dogmatic would be insisting never to use break/continue or insisting there should only ever be one exit point. Pointing out that the wrong tool for the job is being used isn't dogmatic. You aren't introducing an extra variable to dodge a break/continue. You are doing it to make the code easier to read and understand. As I said, the 2nd example could be better but is better than the OP's preferred method. do{....}while(!dataIsValid); is far superior to the OP's way. You can immediately look at the code and know exactly what was intended without having to pause and think. — Dunk, Jan 09 '15 at 14:54
@Dunk You may immediately know what was *intended*, but you *will* have to pause and think *harder* to check that it's *correct*, which is worse. `dataIsValid` is mutable, so to know that the loop terminates correctly I need to check that it's set when the data is valid, *and also* that it's not unset at *any* point afterwards. In a loop of the form `do { ... if (!expr) { break; } ... } while (true);` I know that if `expr` evaluates to `false`, the loop *will* break without having to look at the rest of the loop body. — Doval, Jan 09 '15 at 15:05
@Mawg I see that you read `handles errors` in the title and started thinking about exceptions. This is about handling errors reported by functions like `cin.fail()` which does not throw exceptions. Read my example to see what I am refering to. — wefwefa3, Jan 09 '15 at 16:27
@Doval:You do realize that your explanation just made the claim that "for", "do" and "while" loops are wrong. You've essentially claimed there should only be a single "loop" keyword and inside the processing is where you decide to exit the loop or not, not at the entry/exit of the loop. So now you have to dive into the code of the loop to figure out how to get out of the loop. Sounds too nasty to me. — Dunk, Jan 09 '15 at 20:31
@Doval:I also don't know how you read code, but I certainly don't read every line when I am trying to figure out what it is doing or where might the error I am looking for be. If I saw the while loop, it is immediately apparent (from my example) that the loop is getting valid data. I don't care how it is doing it, I can skip everything else inside the loop unless it just so happens that for some reason I am getting invalid data. Then I would look at the code inside the loop. Your idea requires me to look at all the code in the loop to figure out that it isn't of interest to me. — Dunk, Jan 09 '15 at 20:36
@Dunk My claim is only that if you need to abort a loop in the middle of an iteration, you can't do better than actually aborting the loop. Fiddling with mutable state so the loop ends at the start of the next iteration is a really roundabout and error-prone way to go about it. Dragging debugging into this is to derail the discussion to a topic too broad even for a question, let alone comments. If you start with the premise that you don't care "how it is doing it", all the loop needs to do to satisfy you is be wrapped in a function and you can move along if you don't find its name interesting. — Doval, Jan 09 '15 at 21:00
I guess I would consider "aborting a loop" to only happen for unexpected conditions, not the expected condition. If we had an example like that then I very well may agree with you. But for the OP's specific example I see too many issues, not being semantically correct and harder to read IMO are my top complaints. About the only thing it has going for it is that it works. — Dunk, Jan 09 '15 at 21:16

score 15 · Accepted Answer · answered Jan 08 '15 at 23:58

15

I would write the if-statement slightly different, so it is taken when the input is successful.

for (;;) {
    cout << ": ";
    if (cin >> input)
        break;
    cin.clear();
    cin.ignore(512, '\n');
}

It's shorter as well.

Which suggests a shorter way that might be liked by your teacher:

cout << ": ";
while (!(cin >> input)) {
    cin.clear();
    cin.ignore(512, '\n');
    cout << ": ";
}

answered Jan 08 '15 at 23:58

Sjoerd

2,906
1
19
18

Good idea reversing the conditionals like that so I do not require a `break` at the end. That alone makes the code alot easier to read. I did not know that `!(cin >> input)` was valid either. Thanks for letting me know! – wefwefa3 Jan 09 '15 at 07:49
8

The problem with the second solution is that it duplicates the line `cout << ": ";`, so violating the DRY principle. For real-world code this would be a no-go, since it becomes error-prone (when you have to change that part later, you will have to make sure you don't forget to change two lines in similar manner now). Neverthelss I gave you a +1 for your first version, which is IMHO an example of how to use `break` properly. – Doc Brown Jan 09 '15 at 17:16
2

This feels like you picking at hairs, considering the line duplicated is obviously a bit of humdrum UI. You could remove the first `cout << ": "` entirely and nothing would break. – jdevlin Jan 09 '15 at 21:11
2

@codingthewheel: you are correct, this example is too trivial to demonstrate the potential risks in ignoring the DRY pricinple. But imagine such a situation in real world code, where introducing bugs accidentally can have real consequences - then you might think different about it. That's why after three decades of programming I got into the habit of **never resolve such a loop exit problem by duplicating the first part** just "because a `while` command looks nicer". – Doc Brown Jan 09 '15 at 22:04
1

@DocBrown The duplication of the cout is mostly an artifact of the example. In practice, the two cout statements will be different as the second will contain an error message. E.g. *"Please enter :"* and *"Error! Please re-enter :"*. – Sjoerd Jan 11 '15 at 13:11

score 15 · Answer 2 · answered Jan 09 '15 at 17:52

15

What you have to strive for is avoiding raw loops.

Move the complex logic into a helper function and suddenly things are a lot clearer:

bool getValidUserInput(string & input)
{
    cout << ": ";
    cin >> input;
    if (cin.fail()) {
        cin.clear();
        cin.ignore(512, '\n');
        return false;
    }
    return true;
}

int main() {
    string input;
    while (!getValidUserInput(input)) { 
        // We wait for a valid user input...
    }
}

answered Jan 09 '15 at 17:52

glampert

1,131
1
9
13

1

This is pretty much what I was going to suggest. Refactor it to separate getting the input from the decision to get the input. – Rob K Jan 09 '15 at 19:18
4

I do agree that this is a nice solution. It becomes the far superior alternative when `getValidUserInput` is called many times and not just once. I give you +1 for that but I will accept Sjoerd's answer instead because I find that it answers my question better. – wefwefa3 Jan 09 '15 at 21:38
@user3787875: exactly what I thought when I read the two answers - good choice. – Doc Brown Jan 09 '15 at 22:23
This is the logical next step after the rewrites in my answer. – Sjoerd Jan 11 '15 at 13:15

score 9 · Answer 3 · answered Jan 09 '15 at 04:18

9

It's not so much that for(;;) is bad. It's just not as clear as patterns like:

while (cin.fail()) {
    ...
}

Or as Sjoerd put it:

while (!(cin >> input)) {
    ...
}

Let's consider the primary audience for this stuff as being your fellow programmers, including the future version of yourself who no longer remembers why you stuck the break at the end like that, or jumped the break with the continue. This pattern from your example:

for (;;) {
    ...
    if (blah) {
        continue;
    }
    break;
}

...requires a couple seconds of extra thought to simulate versus other patterns. It's not a hard and fast rule, but jumping the break statement with the continue feels messy, or clever without being useful, and putting the break statement at the end of the loop, even if it works, is unusual. Normally both break and continue are used to prematurely evacuate the loop or that iteration of the loop, so seeing either of them at the end feels a little weird even if it's not.

Other than that, good stuff!

answered Jan 09 '15 at 04:18

jdevlin

416
2
7

Your answer looks nice - but only on a first glance. In fact, it hides the inherent problem we can see in @Sjoerd`s answer: using a `while` loop this way can lead to unncessary code duplication for the given example. And that's a problem which is IMHO at least as important to address as the overall readability of the loop. – Doc Brown Jan 09 '15 at 17:24
Relax, Doc. OP asked for some basic advice/feedback on his loop structure, I gave him the standard interpretation. You can argue for other interpretations -- the standard approach isn't always the best or most mathematically rigorous approach -- but especially since his own instructor leaned toward the use of the `while`, I think the "only on a first glance" bit is unjustified. – jdevlin Jan 09 '15 at 21:09
Don't get me wrong, this has nothing to do with beeing "mathematically rigorous" - this is all about writing **evolvable** code - code where adding new features or changing existing features has the smallest possible risk of introducing bugs. In my job, that is really not an academic problem. – Doc Brown Jan 09 '15 at 22:12
I would like to add that I agree mostly to your answer. I just wanted to point out that your simplification at the beginning *might* hide a frequent problem. When I have a chance to use "while" without any code duplication, I will not hesitate to use it. – Doc Brown Jan 09 '15 at 22:19
1

Agreed, and sometimes an unnecessary line or two before the loop is the price we pay for that declarative front-loaded `while` syntax. I don't like to do something before the loop that is also done inside the loop, then again, I also don't like a loop that ties itself in knots trying to refactor out the prefix. So a bit of a balancing act either way. – jdevlin Jan 09 '15 at 23:08

score 3 · Answer 4 · answered Jan 08 '15 at 22:11

3

My logic instructor at school always said, and pounded it into my brain, that there should only be one entrance and one exit to loops. If not you start getting spaghetti code and not knowing where the code is going during debugging.

In real life I feel that code readability is really important so that if someone else needs to fix or debug an issue with your code it's easy for them to do.

As well if it's a performance issue because you are running through this look millions of times the for loop might be faster. Testing would answer that question.

I don't know C++ but I completely understood the first two of your code examples. I am assuming that continue goes to the end of the for loop and then loops it again. The while example I understood completely but for me it was easier to understand than your for(;;) example. The third one I would have to do some thought work to figure it out.

So going by for me which was easier to read (not knowing c++) and what my logic instructor said I would use the while loop one.

answered Jan 08 '15 at 22:11

Jaydel Gluckie

139
4

4

So, your instructor is against using exceptions? And early return? And similar things? Properly employed, they all make code more readable... – Deduplicator Jan 08 '15 at 23:52
1

The key phrase in your ellipsized sentence is "properly employed". My bitter experience seems to me to indicate that damned near NOBODY in this crazy racket knows how to employ those things "properly", for almost any reasonable definition of "properly" (or "readable", for that matter). – John R. Strohm Jan 09 '15 at 00:10
2

Remember that we're talking about a class where the students are beginners. Certainly, training them to avoid these things that cause common problems while they're beginners (before they understand the implications and can reason about what would be an exception to the rule) is a good idea. – user1118321 Jan 09 '15 at 04:06
performance is not an issue, since he reads the standard input – BЈовић Jan 09 '15 at 07:51
2

The "one entrance - once exit" dogma is IMHO an antiquated point of view, probably "invented" by Edward Dijkstra at a time where people where messing around with GOTOs in languages like Basic, Pascal and COBOL. I agree fully to what Deduplicator wrote above. Moreover, the first two examples of the OP actually *contain just one* entry and one exit, nevertheless the readability is mediocre. – Doc Brown Jan 09 '15 at 17:35
4

Agree with @DocBrown. "One entrance one exit" in modern production codebases tends to be a recipe for code cruft and encourages deep nesting of control structures. – jdevlin Jan 09 '15 at 21:13
Antiquated, code cruft, deep nesting sure I guess, but it works and is extremely easy to follow and debug. This is one of those arguments that stackoverflow wants to negate, there's never one 100% correct answer. To each their own as long as it works and the next developer looking at your code doesn't curse your name each time a bug needs to be fixed. – Jaydel Gluckie Jan 17 '15 at 21:28

score -2 · Answer 5 · answered Jan 09 '15 at 18:43

to me, the most straight forward C translation of the pseudocode is

do
    {
    success = something();
    if (success == FALSE)
        {
        handle_the_error();
        }
    } while (success == FALSE)
\\ moving on ...

i don't get why this obvious translation is a problem.

perhaps this:

while (!something())
    {
    handle_the_error();
    }

that looks simpler.

How to structure a loop that repeats until success and handles failures

Here is an example in C++

5 Answers5