8

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?

  • 6
    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 8 '15 at 22:43
  • 3
    @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 9 '15 at 0:07
  • 1
    Is it too early for you to learn exception handling? – Mawg Jan 9 '15 at 10:30
  • 2
    @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 9 '15 at 15:05
  • 1
    @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 9 '15 at 20:36
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 << ": ";
}
  • 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 9 '15 at 7: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 9 '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 9 '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 9 '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 <foo>:" and "Error! Please re-enter <foo>:". – Sjoerd Jan 11 '15 at 13:11
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...
    }
}
  • 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 9 '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 9 '15 at 21:38
  • @user3787875: exactly what I thought when I read the two answers - good choice. – Doc Brown Jan 9 '15 at 22:23
  • This is the logical next step after the rewrites in my answer. – Sjoerd Jan 11 '15 at 13:15
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!

  • Your answer looks nice - but only on a first glance. In fact, it hides the inherent problem we can see in @Sjoerds 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 9 '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 9 '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 9 '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 9 '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 9 '15 at 23:08
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.

  • 4
    So, your instructor is against using exceptions? And early return? And similar things? Properly employed, they all make code more readable... – Deduplicator Jan 8 '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 9 '15 at 0: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 9 '15 at 4:06
  • 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 9 '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 9 '15 at 21:13
-2

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.

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