0

My C project is a Windows console app that takes the time signature and BPM from a user's musical project and returns the length of a bar in seconds.
I am trying to use a do-while loop to add a "continue/exit?" prompt at the end of each successful calculation Here is the source code for that function. It performs one calculation based on user input, then terminates.

#include <stdio.h>
#include <stdlib.h>

int main()
{
    char timeSignature[6];
    float BPM;
    float beatsPerBar;
    float barLength;

    printf("Enter the working time signature of your project:");
    scanf("%s",timeSignature);

    beatsPerBar = timeSignature[0]-'0';

    printf("Enter the beats per minute:");
    scanf("%f", &BPM);

    barLength = BPM / beatsPerBar;

    printf("%f\n", barLength);
    return 0;
}

After each successful calculation, I want to prompt the user to choose "y" to return to the initial input prompt or "n" to end the program and exit the command prompt. This later update includes a do-while loop that is intended to add the feature.

#include <stdio.h>
#include <stdlib.h>
#include <windows.h>
#include <unistd.h>

int main()
{

    do{

        char timeSignature[6];
        char anotherCalculation;
        float BPM;
        float beatsPerBar;
        float barLength;

        printf("Enter the working time signature of your project:");
        scanf("%s",timeSignature);

        beatsPerBar = timeSignature[0]-'0';
        /*
         * Subtracts the integer value of the '0' character (48) from the integer value
         * of the character represented by the char variable timeSignature[0] to return
         * an integer value equal to the number character itself.
         */

        printf("Enter the beats per minute:");
        scanf("%f", &BPM);

        barLength = BPM / beatsPerBar;

        printf("%f\n", barLength);

        Sleep(3);

        printf("Would you like to do another calculation? (Y/N)");
        scanf(" %c", &anotherCalculation);

    }while((anotherCalculation = 'Y'||'y'));

    if((anotherCalculation != 'Y'||'y'))
        {
        printf("Goodbye!");
        return 0;
        }

    return 0;
}

When I compile, there are no errors, but when I run it, the program loops after ANY input. Why is the code ignoring my truth assignment? What can I do to fix this?

  • 2
    emmmm. where is the loop? – Sourav Ghosh Sep 08 '15 at 20:46
  • There's no do-while loop in the code you posted. – The Paramagnetic Croissant Sep 08 '15 at 20:46
  • Sorry, took me a moment to see/fix that. – Nathaniel Lindsey Sep 08 '15 at 20:48
  • 1
    this line: `if((anotherCalculation != 'Y'||'y'))` is completely unnecessary and will NOT produce the desired result. It is unnecessary because the 'while()' statement already checked that. it will not work as desired because only one condition can be checked at a time in C, suggest: `if((anotherCalculation != 'Y' && anotherCalculation != 'y'))` however, as I stated this 'if' statement is totally not needed – user3629249 Sep 08 '15 at 21:16
  • always check the returned value from a call to scanf() to assure the operation was successful, otherwise the 'while()' statement would be looking at what ever uninitalized trash is already in the anotherCalculation variable. Similar considerations for the timeSignature variable. Also when using the '%s' format specifier, always put a length modifier so the user cannot overrun the input buffer (resulting in undefined behaviour and can lead to a seg fault event.) – user3629249 Sep 08 '15 at 21:17
  • 1
    The calculation for bar time in seconds `barLength = BPM / beatsPerBar` is incorrect (inverted). It should be `barLength = beatsPerBar * 60.0 / BPM` – Weather Vane Sep 08 '15 at 21:20
  • this line: `}while((anotherCalculation = 'Y'||'y'));` will not work, the while parameter is actually: `anotherCalculation = 'Y' (which sets anotherCalculation to 'Y' rather than compares it) || 'y' (since this while() is 'true' if any of the parameter expressions are true (the '||') and 'Y' is not a 0 value and 'y' is not a 0 value, it will always evaluate to true. Suggest: `}while(('Y' == anotherCalculation ||'y' == anotherCalculation));` Note: always put the literal on the left in a comparison so the 'single =' error will be caught by the compiler instead of having to perform debuging – user3629249 Sep 08 '15 at 21:27
  • this line: `scanf("%s",timeSignature);` will fail on the second time through the loop. (always check the returned value to catch this failure) because %s stops inputting when encountering any white space and there will be the white space (newline) as the first character in the input stream due to the user entering the 'Y' or 'y' char. – user3629249 Sep 08 '15 at 21:43
  • @user3629249 that is incorrect: `%s` discards leading whitespace too. – Weather Vane Sep 08 '15 at 21:50

7 Answers7

4

Change this:

while (anotherCalculation = 'Y'||'y')

To this:

while(anotherCalculation == 'Y'|| anotherCalculation == 'y')

Otherwise you're doing:

while (anotherCalculation = ('Y'||'y'))

Note I also changed the assignment = to comparison ==.

Same goes for your if later:

if (anotherCalculation != 'Y'||'y')

Should be:

if (anotherCalculation != 'Y'|| anotherCalculation != 'y')

The if is redundant by the way, you've already left the while loop, so the user does not want any more calculations.

Reut Sharabani
  • 30,449
  • 6
  • 70
  • 88
3

Convert your value to an upper-case letter before doing the comparison:

while(toupper(anotherCalculation) == 'Y');

That will accept either a lower-case or upper-case 'Y', and keep your code simple and straightforward.

abelenky
  • 63,815
  • 23
  • 109
  • 159
2

Your problem is with the following line of code:

while((anotherCalculation = 'Y'||'y'));

Your first problem is that you are assigning to the variable anotherCalculation and then testing that value. This is because in C the = (assignment) operators can be used in test statements and the value that was set will be returned into the statement basically making your test statement the following:

while('Y'||'y');

Also there is a fault in the logic within your test statement even after substituting the = for ==,
What you are actually doing there is the same as:

TRUE if anotherCalculation == 'Y' or if 'y'.
Since 'y' actually has a value and FALSE is 0 in standard C, your loop will continuously run because it will always have a TRUE value.

To fix all errors, what that line actually needs to be is:

while(anotherCalculation == 'Y'|| anotherCalculation == 'y');

Finally, if you want to make this code more readable and avoid double testing, you can convert the input to uppercase and test that:

while(toupper(anotherCalculation) == 'Y');
Serdalis
  • 10,296
  • 2
  • 38
  • 58
2

First, you're using a mathematical notation for your logical checks that makes no sense in C, and you're using the assignment = operator instead of the "test for equivalence" == operator.

}while((anotherCalculation = 'Y'||'y'));

Should be:

}while((anotherCalculation == 'Y'|| anotherCalculation == 'y'));

Second, save yourself some hassle down the road, and use fgets() to get a line of a text, and parse it.

Finally, read into things like "trailing newlines" with scanf() and fgets(). The scanf() function, for example, won't "eat up" the newline at the end of your input, and you'll get unexpected behavior. This very question appears on SO once per month at least.

Community
  • 1
  • 1
Cloud
  • 18,753
  • 15
  • 79
  • 153
2

Logical operators don't work like that. x == a || b doesn't evaluate as "x is equal to one of a or b"; instead, it evalutes as "x is equal to the result of the Boolean expression (a OR b)". If you want to compare x against different values, you must write it as

x == a || x == b 

But you have another problem, you've used the assignment operator = where you meant to use the comparison operator ==1:

while((anotherCalculation = 'Y'||'y'))
                          ^
                          |
                        ooops

You're performing a logical-OR of two non-zero values, which evaluates to 1 (true), and assigning the result to anotherCalculation. The result of the expression is the value of anotherCalculation after the assignment (1), so you've effectively written

while( true )

You need to use the == operator for equality comparison, and you must do an explicit comparison on each side of the || operator:

while ( anotherCalculation == 'Y' || anotherCalculation == 'y' )

You can clean this up a bit by converting your input to a specific case before doing a comparison, getting rid of the need for the || operator altogether:

while ( tolower( anotherCalculation ) == 'y' )

or

while ( toupper( anotherCalculation) == 'Y' )


1. A bright young coder named Lee
wished to loop while i was 3
When writing the =
He forgot its sequel
And thus looped infinitely
John Bode
  • 119,563
  • 19
  • 122
  • 198
1

This is not doing what you expect:

}while((anotherCalculation = 'Y'||'y'));

Nor is this:

if((anotherCalculation != 'Y'||'y'))

You should be doing this:

}while((anotherCalculation == 'Y') || (anotherCalculation =='y'));

And this:

if((anotherCalculation != 'Y') && (anotherCalculation !='y'))
dbush
  • 205,898
  • 23
  • 218
  • 273
0

TLDR; Change your condition to

while( anotherCalculation == 'Y' || anotherCalculation == 'y' );
neoaggelos
  • 632
  • 1
  • 4
  • 18