0

No matter what option I choose, I'm always prompted to enter a new shift value (as if I entered 1 as my choice). Why does this code ignore the option that I selected?

#include <stdio.h>
#include <string.h>
#define SIZE 500
int getUserChoice()
{
    int decision = 0;

    printf("-------------------------------\n");
    printf("| 1) Change Shift (default 3) |\n");
    printf("| 2) Encrypt a message        |\n");
    printf("| 3) Decrypt a message        |\n");
    printf("| 4) Quit                     |\n");
    printf("-------------------------------\n");
    printf("Option: ");
    scanf("%d", &decision);
    if(decision == 0 || decision >= 4)
    {
        return 0;
    }

    return decision;
}
int getShift()
{
    int key = 3;
    printf("Enter new shift value: ");
    scanf("%d", &key);
   return 0;
}
void getString(char buf[])
{
    int act;
    act = SIZE - 1;
    int count[26] = {0};
    fgets(buf, act, stdin);
    return;
}
void encrypt(char buf[], int shift)
{
    int i = 0;
    for(i = 0; i < strlen(buf);++i)
    {
        buf[i] = buf[i] + shift;
    }
    return;
}
void decrypt(char buf[], int shift)
{
    int i = 0;
    for(i = 0; i < strlen(buf); ++i)
    {
        buf[i] = buf[i] - shift;
    }
    return;
}
int main()
{
    char userStr[SIZE];
    int number = 0;
    getUserChoice();
    int shift = 0;
    if(number = 1)
    {
        getShift();

    }
    if (number = 2)
   {
        getString(userStr);
        encrypt(userStr, shift);
    }
    if (number = 3)
    {
        getString(userStr);
        decrypt(userStr, shift);
    }


    return 0;

}
user3386109
  • 34,287
  • 7
  • 49
  • 68
mychem97
  • 51
  • 1
  • 5
  • When I compile and run this, it prints the menu before asking for any input. – Scott Hunter Jan 28 '17 at 01:09
  • 1
    1) `getUserChoice();` --> `number = getUserChoice();` – BLUEPIXY Jan 28 '17 at 01:14
  • I asked for help with the wrong thing, I've had a lot of problems with this recently. What I meant to ask help with was why, no matter what option I choose when prompted, am I always prompted to enter a new shift value, when that should only be prompted when the user selects option 1. – mychem97 Jan 28 '17 at 01:15
  • 1
    2) `if(number = 1) { getShift(); }` --> `if(number == 1) { shift = getShift(); }`, `scanf("%d", &key); return 0;` --> `scanf("%d", &key); return key;` – BLUEPIXY Jan 28 '17 at 01:15
  • 1
    3) `scanf("%d", &decision);` --> `scanf("%d%*c", &decision);` :p – BLUEPIXY Jan 28 '17 at 01:20
  • 1
    4) `main` needs loop and Display results. – BLUEPIXY Jan 28 '17 at 01:21

2 Answers2

1

That looks a lot like homework and you didn't seem to make much effort on finding the issues, but here we go.. There are several flaws on the code:

1) Ignoring return values:

Some of your functions, getUserChoice and getShift, are supposed to return a value but those values are completely ignored in the code. You are not assigning it to any local variables and they are not modifying anything outside their scope, so whatever value is being returned is getting lost.

The code:

if(number = 1){
    getShift();
}

Should be:

if(number == 1){
    shift = getShift();
}

Which brings me to another big mistake, reason of many headaches on C when it happens unintentionally:

2) Mistakenly writing the assignment operator '=' where a comparison operator '==' is required:

The assignment operator returns a value: the left operand value after assignment. So the expression if(number = 1) is valid and will evaluate to true every time, since the returning value of the assignment is always 1. if(number == 1) however, will only evaluate to true when number is equal to 1. I find it useful to make comparisons "Yoda-Style", like if(1 == number) to avoid those kind of mistakes (that can easily happen on a typo). They are hard to track, since the code is valid, but the behavior is not the expected. Using the "Yoda-Style" comparison, you would get a compiler error if you mistyped if(1 = number), since that's an invalid assignment.

3) Not properly filtering the user input:

On the function getUserChoice you have the following conditional statement to check if the input is valid (when invalid it defaults to the "Quit" option):

if(decision == 0 || decision >= 4)
{
    return 0;
}

However, since decision is a signed int, negative values are valid. You don't account for those. The following addresses that:

if(decision <= 0 || decision >= 4)
{
    return 0;
}

4) getShift doesn't return the shift value:

You used scanf on the getShift function to get a shift value, but returned 0. You should return the key value instead.

5) Unused variable at getString + Wrong size argument on fgets:

You declared an array of 0's on getString which serves no purpose on it. Also, from fgets man page:

fgets() reads in at most one less than size characters from stream and stores them into the buffer pointed to by s. Reading stops after an EOF or a newline. If a newline is read, it is stored into the buffer. A terminating null byte ('\0') is stored after the last character in the buffer.

So you don't really need to subtract 1 from SIZE, since it will already account for the null character.

6) Not clearing the stdin before calling fgets:

After the call to scanf("%d",...);, you will have a hanging newline character in your stdin. That's because the format string parsed to scanf there only instructs it to read any number of decimal digits, not including the newline that follows those.

That hanging newline would cause your fgets to quit before getting any user input. You need to clear the stdin by reading the hanging characters from a previous scanf call. In the example, this is being done by this loop:

int c;
while((c = getchar()) != '\n' && c != EOF){
    //Clearing Stdin
}

An alternative is to use:

scanf("%*[^\n]"); scanf("%*c"); //Consume all characters up to \n (if present) then consume exactly one char ('\n' itself)

7) Not using a loop in main to prompt the user repeatedly:

You only prompt the user once, do what is asked and quit, instead of asking the user repeatedly for options until he asks to quit.

8) Unnecessary return statements on the end of void returning functions:

void return functions will return when they reach their end, so there is no need to add a return statement unless you are returning before the end of the function body.

The code below is edited to fix the mentioned issues:

#include <stdio.h>
#include <string.h>
#define SIZE 500
int getUserChoice()
{
    int decision = 0;

    printf("-------------------------------\n");
    printf("| 1) Change Shift (default 3) |\n");
    printf("| 2) Encrypt a message        |\n");
    printf("| 3) Decrypt a message        |\n");
    printf("| 4) Quit                     |\n");
    printf("-------------------------------\n");
    printf("Option: ");
    scanf("%d", &decision);
    int c;
    while((c = getchar()) != '\n' && c != EOF){
        //Clearing Stdin
    }
    if(decision <= 0 || decision >= 4)
    {
        return 0;
    }

    return decision;
}

int getShift()
{
    int key = 3;
    printf("Enter new shift value: ");
    scanf("%d", &key);
    int c;
    while((c = getchar()) != '\n' && c != EOF){
        //Clearing Stdin
    }
    return key;
}
void getString(char *buf)
{
    fgets(buf, SIZE, stdin);
    return;
}
void encrypt(char *buf, int shift)
{
    int i = 0;
    for(i = 0; i <= (strlen(buf)-1);++i)
    {
        buf[i] = buf[i] + shift;
    }
    return;
}
void decrypt(char *buf, int shift)
{
    int i = 0;
    for(i = 0; i <= (strlen(buf)-1); ++i)
    {
        buf[i] = buf[i] - shift;
    }
    return;
}
int main()
{
    char userStr[SIZE];
    int shift = 3;

    while(1){
        int number = getUserChoice();
        if(number == 1)
        {
            shift = getShift();

        }
        if (number == 2)
        {
            getString(userStr);
            printf("Message: %s\n", userStr);
            encrypt(userStr, shift);
            printf("Shifted Message: %s\n", userStr);
        }
        if (number == 3)
        {
            getString(userStr);
            printf("Shifted Message: %s\n", userStr);
            decrypt(userStr, shift);
            printf("Message: %s\n", userStr);
        }
        if (number == 0){
            break;
        }
    }
    return 0;
}
IanC
  • 1,968
  • 14
  • 23
  • @user3386109 I still use it, guess I got used to it so it doesn't look that bad to me, but I can see why people would disagree. Guess if you compile with `-Wall` you should be safe.. – IanC Jan 28 '17 at 03:29
  • 1
    @BLUEPIXY It was my mistake, I was thinking of the comparison as "lower or equal", instead of just "lower than". The error was hidden because of the newline character, so all visible characters except the newline were being shifted on the tests. I'll edit it out! – IanC Jan 28 '17 at 03:39
  • "... flush the `stdin`? Oh, yeh, I know how to do that! `fflush(stdin);`!"... The very presense of the word "flush" will cause some people to think this. – autistic Jan 28 '17 at 04:26
  • Also, `scanf("%*[^\n]");` is the shortcut; you don't need that loop. – autistic Jan 28 '17 at 04:29
  • 2
    @Seb Although `fflush(stdin)` could work depending on the implementation, take a loot at [this answer](http://stackoverflow.com/a/18170435/6676042) and [this another one](http://stackoverflow.com/a/38325926/6676042): It isn't defined in the standards, so its behavior is undefined and it should be avoided.. – IanC Jan 28 '17 at 09:36
  • Fantastic answer. – Micrified Jan 28 '17 at 17:41
  • @Seb I apologize, I misunderstood your comment and was thinking you were saying the OP should use `fflush`. I see your point and will soon edit it so it doesn't mislead people into using it. – IanC Jan 29 '17 at 02:27
  • 1
    One possible alternative would be to describe the entire action of the loop, e.g. to *(read and) discard the remainder of the line*. You could start by explaining that `scanf("%d", ...)` instructs the computer to read *a sequence of decimal digits*, **not** the newline that follows it; subsequent calls to `scanf` will come across that newline later, so it needs to be read and discarded. This can be done using `scanf("%*[^\n]");`. I'd recommend it in `getUserChoice` and `getShift`, rather than `getString`, as that'd decouple user interface refactoring from logic; there's a bug otherwise... – autistic Jan 29 '17 at 02:52
  • The bug: consider `int main() { char userStr[SIZE]; getString(userStr); printf("That string is: %s\n", userStr); }`... – autistic Jan 29 '17 at 02:54
  • @Seb That sounds good! I'll edit the answer as soon as I get back from work (I'm on my phone). I didn't get to test it yet, but I guess you're right about the bug: the user is going to get stuck in the loop until he types enter, right? – IanC Jan 29 '17 at 13:59
  • @Seb I used other words in the paragraph about clearing the `stdin` as suggested, but couldn't get `scanf("%*[^\n]%*c");` nor `scanf("%*[^\n]\n");` to work as you suggested. Theoretically they should work, but for some reason they are not getting rid of the hanging newline. For that reason I kept the `getchar` loop approach and wrote a question regarding `scanf` not working for that [here](http://stackoverflow.com/q/41928145/6676042). If you have any clues, take a look at it! Thanks :) – IanC Jan 30 '17 at 02:07
  • @IanC What do you think the `[` directive is *supposed* to do when it encounters an empty field? – autistic Jan 30 '17 at 06:15
  • @IanC There is a reason I wrote `scanf("%*[^\n]");`, you invented the `\n` on the end... Learn `scanf` before you try to use it; the same goes with ALL features of C, since undefined behaviour is EVERYWHERE! – autistic Jan 30 '17 at 06:17
  • @Seb You wrote `scanf("%*[^\n]");`, which by itself wouldn't work neither, leaving the newline hanging. I found those two other patterns while looking for ways to fix it. Unfortunately, some [older](http://stackoverflow.com/a/30070821/6676042) answers and [questions](http://stackoverflow.com/q/17248442/6676042) were misleading. They didn't count with the fact that the first pattern failure would quit `scanf` before it consumed the newline. It is no undefined behavior, actually pretty defined according to the man page, I just didn't know it yet, since I don't use this command frequently. – IanC Jan 30 '17 at 09:35
  • @IanC Yes. Better yet, a straight-forward solution to removing a single character would be to simply call `getchar()`. I wouldn't describe this as *clearing*, either, as the C standard uses that term to describe a *zeroing* operation on flags, for example: *"The `clearerr` function clears the end-of-file and error indicators for the stream pointed to by stream."* If you're looking for a more standard term, you'll find the term "advance" used in [the spec for `fgetc`](http://www.iso-9899.info/n1570.html#7.21.7.1) (among others). – autistic Jan 31 '17 at 00:17
0

The are two things in the code that cause it to ignore the user input.

First, the getUserChoice function returns a value, but main doesn't store or use the return value. The function call should look like this:

number = getUserChoice();

Second, a single equal sign if(number = 1) acts as an assignment. It sets number to the value 1. To compare number with 1, use the equality operator, e.g.

if ( number == 1 )

Note that any decent compiler will generate a warning when you mistakenly put an assignment inside an if statement. You should make sure that you enable all warnings (use -Wall with gcc or clang, /W4 with microsoft), and fix all of the warnings. If you ignore compiler warnings, you'll get nowhere fast.

There are other problems in your code, but those are the two problems that are causing the user input to be ignored.

user3386109
  • 34,287
  • 7
  • 49
  • 68