My loop simply won't loop! what is missing? working w/ C++

Question

I would like to understand why the loop will not loop if i != 1234 and ask to "please try again".

#include <iostream>

int main() {
    //i want to create a program to stop after three attempts

    int i = 1234;

    for (i = 0; i < 3; i++) {

        std::cout << "enter pin ";
        std::cin >> i;

        if (i == 1234) {
            std::cout << "Thank you" << "\n";
        }
         
        else if (i < 1000 || i > 9999) {
            std::cout << "Invalid" << "\n";
        }

        else {
            std::cout << "incorrect, please try again" << "\n";

        }

    }
}

You loop on the condition that `i` is `< 3`, but you take `i` as input from the user .... — ChrisMM, Oct 26 '22 at 15:48
Why are you trying to use `i` for so many different things? Changing your loop counter's value is surely not what you want. You should have three distinct variables. — sweenish, Oct 26 '22 at 15:48
You also force the user to re-enter the PIN even if it's correct. — sweenish, Oct 26 '22 at 15:50
This also wouldn't handle the case of the PIN being 0000. You really should be reading in a string. — sweenish, Oct 26 '22 at 15:52
[What is a debugger and how can it help me diagnose problems?](https://stackoverflow.com/questions/25385173/what-is-a-debugger-and-how-can-it-help-me-diagnose-problems) — Jason, Oct 26 '22 at 15:52
After a prompt `std::cout << "enter pin ";` the next line should read `std::cin >> pin;` and nothing else. Variables are cheap, so no reason to reuse them. It only causes confusion (like here), — BoP, Oct 26 '22 at 15:57

sweenish · Answer 1 · 2022-10-26T16:29:09.910

First, a mini-code review:

#include <iostream>

int main() {
    //i want to create a program to stop after three attempts

    int i = 1234;  // Should have a different name

    for (i = 0; i < 3; i++) {

        std::cout << "enter pin ";
        std::cin >> i;  // Overwrites value of i, throwing loop off

        if (i == 1234) {
            std::cout << "Thank you" << "\n";  // Still repeats
        }
        // I subjectively don't like these blank lines.
        // I prefer if/else blocks to be contiguous.
        else if (i < 1000 || i > 9999) {
            std::cout << "Invalid" << "\n";  // No need to separate these
        }

        else {
            std::cout << "incorrect, please try again" << "\n";

        }

    }
}

Overall, not awful compared to some of the other beginner code I've seen here.

The issue is that you are trying to use the variable i for many different purposes. Anything you write/use in programming should be singular in purpose. You have three separate pieces of data to store: the stored PIN, the loop counter, and the user's input. You should have three separate variables.

Finally, you don't say whether or not 0000 is a valid PIN or not. Typically it is, even if the security of it is awful. However, reading the PIN as an int doesn't make this possible. So let's use a std::string instead.

#include <cctype>
#include <iostream>
#include <string>

bool is_all_digits(const std::string& val) {
    for (const auto& c : val) {
        if (!std::isdigit(c)) {
            return false;   
        }
    }
    
    return true;
}

int main()
{
    std::string pin{"1234"};

    // Altered number of iterations to exercise all possible scenarios.
    for (int i = 0; i < 4; ++i) {
        std::string input;
        std::cout << "Enter PIN: ";
        std::getline(std::cin, input);
        
        // Error check first
        // First, is the length correct?
        if (input.length() != 4) {
            std::cout << "Invalid. Try again.\n\n";
            continue;  // Go to next loop iteration; no need to
                       // make the other checks.
        }
        
        // input must have length of 4 to reach here
        if (!is_all_digits(input)) {
            std::cout << "Please enter a valid PIN.\n\n";
            continue;
        }
        
        // input must have length of 4 and be all digits to reach here
        if (input == pin) {
            std::cout << "Thank you.\n";
            break;  // User entered correct PIN, we get out of the loop.
        } else {
            // Not bothering with continue; if this branch is taken, the loop
            // restarts automatically.
            std::cout << "Incorrect.\n\n";
        }
    }
}

Output:

Enter PIN: cut & dry
Invalid. Try again.

Enter PIN: l337
Please enter a valid PIN.

Enter PIN: 5678
Incorrect.

Enter PIN: 1234
Thank you.

If you are allowed to pretend that 0000 is not valid, then you can continue to use ints. The logic above will only need minor tweaks.

Thanks very much for your explanation, based on my original code (mainly due to the fact that your suggested approach is still a tiny bit out of my knowledge of understanding). I was hoping to add some commentary to say something like "this is your final attempt" and "you are now blocked". I am assuming that another 'If' statement is required for each loop - so: if (pin == 1) { std::cout << "this is your final attempt \n"; } else (pin == 2); { std::cout << "you are now blocked \n"; } — NP81, Oct 27 '22 at 09:50
You are correct. The best way to figure out where to place those if checks is to just talk through the algorithm. Out loud. To a [rubber duck](https://en.wikipedia.org/wiki/Rubber_duck_debugging) or whatever other object you want. Then, based on your verbal description of the algorithm, you should know where to place the blocks. — sweenish, Oct 27 '22 at 14:43

score 0 · Answer 2 · answered Oct 26 '22 at 15:51

This has several mistakes. First, you "blow away" the value of i by replacing it with input:

std::cin >> i;

In general, reusing the same variable for multiple things is a bad practice because it can lead to defects like that one. One variable should only be used for one thing.

Also, you don't break out of the loop in the case where they enter the correct value.

score -1 · Answer 3 · answered Oct 26 '22 at 15:52

-1

you need to use a different variable for your input value. if you input anything 3 or greater then the loop will end.

answered Oct 26 '22 at 15:52

jhackso

1
4

My loop simply won't loop! what is missing? working w/ C++

3 Answers3