Stuck in a type of loop

Question

I just started programming in C and I am using Code Blocks to learn. I was working on a simple ATM program and decided to use a goto function to use when an invalid entry is entered. When I 1st used it, it ran as expected. But now it won't move past one of the statements. Code is below.

When any of the options 1-3 is pressed, it runs as supposed to, but also proceeds to run the selection error section after it. If I just try to run the selection error portion, it goes through it, and keeps repeating it over and over. How do I stop this from happening? I only need invalid selection portion to run when the conditions are met and only then. Thank you!

int iSelection = 0;

float fTransAmount = 0.0;

float fBalance = 100.25;

    printf("\n\n\tATM\n");

menu_options:

    printf("\n1\t To Make a Deposit Press One");
    printf("\n2\t To Make a Withdrawal Press Two");
    printf("\n3\t To End Transaction, Press Three\n");
    scanf("%d", &iSelection);

    if (iSelection == 1) {
        printf("\n Enter Amount to Deposit: ");
        scanf("%f", &fTransAmount);
        printf("\n Your new balance is: $%.2f", fBalance + fTransAmount);

    }  //End if for 1

    if (iSelection == 2) {
        printf("\n Enter Amount to Withdraw: ");
        scanf("%f", &fTransAmount);

        if (fTransAmount > fBalance)
            printf("\n Insufficient funds, ending transaction.....\n");
        else
            printf("\n Your new balance is $%.2f\n", fBalance - fTransAmount);

    } //End if for 2

    if (iSelection == 3) {
        printf("\n ending transaction");

    } //End if for 3

       if (iSelection != 1 || iSelection != 2 || iSelection != 3  ) {
        printf("\nInvalid selection, please try again");

        goto menu_options;
    } //End if for Selection Error

Don't use `goto`. You can rewrite this as a `while` loop, which will make it easier to work with and debug. See: http://stackoverflow.com/questions/46586/goto-still-considered-harmful. — Rushy Panchal, Jun 21 '16 at 19:15
The logic of `if (iSelection != 1 || iSelection != 2 || iSelection != 3 )` is wrong, should be `&&`. But don't even do that, use `else if` for conditions 2 and 3, and then `else`. — Weather Vane, Jun 21 '16 at 19:17
Despite what CS profs say, `goto` very well has its applications in C. But they should be kept to places it enhances readability. Your code is not a good use case. Actually it not even is an acceptable one. Use other iteration statements. — too honest for this site, Jun 21 '16 at 20:01

dbush · Answer 1 · 2016-06-21T19:26:29.520

Your if condition is wrong:

(iSelection != 1 || iSelection != 2 || iSelection != 3  )

This will be true when iSelection is not 1 OR is not 2 OR is not 3. This will always be true. You instead want to use a logical AND (&&):

(iSelection != 1 && iSelection != 2 && iSelection != 3  )

Also, this is not an appropriate use of goto. You're better off using a while loop:

while(1) {
    printf("\n1\t To Make a Deposit Press One");

    ...

   if (iSelection != 1 && iSelection != 2 && iSelection != 3  ) {
      printf("\nInvalid selection, please try again");
   } else {
       break;
   }
}

Better yet, use a switch statement instead of multiple if blocks:

do {

    printf("\n1\t To Make a Deposit Press One");
    printf("\n2\t To Make a Withdrawal Press Two");
    printf("\n3\t To End Transaction, Press Three\n");
    scanf("%d", &iSelection);

    int invalidSelection = 0;
    switch (iSelection) {
    case 1:
        printf("\n Enter Amount to Deposit: ");
        scanf("%f", &fTransAmount);
        printf("\n Your new balance is: $%.2f", fBalance + fTransAmount);
        break;
    case 2:
        printf("\n Enter Amount to Withdraw: ");
        scanf("%f", &fTransAmount);

        if (fTransAmount > fBalance)
            printf("\n Insufficient funds, ending transaction.....\n");
        else
            printf("\n Your new balance is $%.2f\n", fBalance - fTransAmount);
        break;
    case 3:
        printf("\n ending transaction");
        break;
    default:
        printf("\nInvalid selection, please try again");
        invalidSelection = 1;
        break;
    }
} while (invalidSelection);

score 0 · Answer 2 · answered Jun 21 '16 at 19:22

0

For something as simple as looping back to the beginning on failure, you should be using a while(1) or for(;;) loop to repeat endlessly until the statement break is executed. It's more readable and straight up works better. goto is used in C mainly for resource cleanup, because without objects or exception, handling error conditions AND freeing memory can be tough.

That all said, your problem is the if (iSelection != 1 || iSelection != 2 || iSelection != 3 ). You are testing that your selection is not 1 or it's not 2 or it's not 3. This is always true, because it's never all three at the same time.

You want: if (iSelection != 1 && iSelection != 2 && iSelection != 3 )

answered Jun 21 '16 at 19:22

define cindy const

632
5
14

You don't even need the last if statement: – FredK Jun 21 '16 at 19:26
^ Agreed, you could have `break` in all your other `if`s to continue past the loop, and in the case of entering none of those, you could just print a message and then at the end of the loop, you'd jump to the top. On that note, you could just use an `if`-`else` tree considering all of your conditions are mutually exclusive. The last condition would be the final `else` block. – define cindy const Jun 21 '16 at 19:28

score 0 · Answer 3 · answered Jun 21 '16 at 19:31

You don't even need the final if-statement:

int repeat = 1;
while ( repeat) {
   repeat = 0;
   if (iSelection == 1) {
      ...
   } else if ( iSelection == 2 ) {
      ...
   } else if ( iSelection == 3 ) {
      ...
   } else {
      // print error here
      repeat = 1;
   }
}

Or you could also use a switch-case construct. this has the advantage that if you ever add other valid values for iSelection, you only need to add an extra "else if" block without also editing a final if-statement.

Stuck in a type of loop

3 Answers3