3

I made this code:

/*here is the main function*/
int x , y=0, returned_value;
int *p = &x;
while (y<5){
    printf("Please Insert X value\n");
    returned_value = scanf ("%d" , p);
    validate_input(returned_value, p);
    y++;
}

the function:

void validate_input(int returned_value, int *p){
    getchar();
    while (returned_value!=1){
        printf("invalid input, Insert Integers Only\n");
        getchar();
        returned_value = scanf("%d", p);
    }
}

Although it is generally working very well but when I insert for example "1f1" , it accepts the "1" and does not report any error and when insert "f1f1f" it reads it twice and ruins the second read/scan and so on (i.e. first read print out "invalid input, Insert Integers Only" and instead for waiting again to re-read first read from the user, it continues to the second read and prints out again "invalid input, Insert Integers Only" again...

It needs a final touch and I read many answers but could not find it.

David Ranieri
  • 39,972
  • 7
  • 52
  • 94

3 Answers3

2

If you don't want to accept 1f1 as valid input then scanf is the wrong function to use as scanf returns as soon as it finds a match.

Instead read the whole line and then check if it only contains digits. After that you can call sscanf

Something like:

#include <stdio.h>

int validateLine(char* line)
{
    int ret=0;
    
    // Allow negative numbers
    if (*line && *line == '-') line++;
    
    // Check that remaining chars are digits
    while (*line && *line != '\n')
    {
        if (!isdigit(*line)) return 0; // Illegal char found

        ret = 1;  // Remember that at least one legal digit was found
        ++line;
    }
    return ret;
}

int main(void) {
    char line[256];
    int i;

    int x , y=0;
    while (y<5)
    {
        printf("Please Insert X value\n");
        if (fgets(line, sizeof(line), stdin)) // Read the whole line
        {
            if (validateLine(line))  // Check that the line is a valid number
            {
                // Now it should be safe to call sscanf - it shouldn't fail
                // but check the return value in any case
                if (1 != sscanf(line, "%d", &x)) 
                {
                    printf("should never happen");
                    exit(1);
                }

                // Legal number found - break out of the "while (y<5)" loop
                break;
            }
            else
            {
                printf("Illegal input %s", line);
            }
        }
        y++;
    }
    
    if (y<5)
        printf("x=%d\n", x);
    else
        printf("no more retries\n");

    return 0;
}

Input

1f1
f1f1

-3

Output

Please Insert X value
Illegal input 1f1
Please Insert X value
Illegal input f1f1
Please Insert X value
Illegal input 
Please Insert X value
x=-3

Another approach - avoid scanf

You could let your function calculate the number and thereby bypass scanf completely. It could look like:

#include <stdio.h>

int line2Int(char* line, int* x)
{
    int negative = 0;
    int ret=0;
    int temp = 0;
    
    if (*line && *line == '-') 
    {
        line++;
        negative = 1;
    }
    else if (*line && *line == '+')  // If a + is to be accepted
        line++;                      // If a + is to be accepted
       
    while (*line && *line != '\n')
    {
        if (!isdigit(*line)) return 0; // Illegal char found
        ret = 1;

            // Update the number
        temp = 10 * temp;
        temp = temp + (*line - '0');

        ++line;
    }
    
    if (ret)
    {
        if (negative) temp = -temp;
        *x = temp;
    }
    return ret;
}

int main(void) {
    char line[256];
    int i;

    int x , y=0;
    while (y<5)
    {
        printf("Please Insert X value\n");
        if (fgets(line, sizeof(line), stdin)) 
        {
            if (line2Int(line, &x)) break;  // Legal number - break out

            printf("Illegal input %s", line);
        }
        y++;
    }
    
    if (y<5)
        printf("x=%d\n", x);
    else
        printf("no more retries\n");

    return 0;
}
Support Ukraine
  • 42,271
  • 4
  • 38
  • 63
  • This code's `fgets()` is the best approach. Minor. Does not accept `"+123"` or `" 456"` (leading spaces). In the end, using `strol()` may be easier. – chux - Reinstate Monica Jun 20 '16 at 17:42
  • @chux - Thanks for the input. I forgot the `+` which can be added quite easy. Not sure if OP would consider white spaces in front as valid but you have a good point. I'm not experienced in using `strol` but a quick read-up do suggest that it could be handy. Maybe you can give OP a good example using `strol` (as I can't) :-) – Support Ukraine Jun 20 '16 at 17:48
1

Generally speaking, it is my opinion that you are better to read everything from the input (within the range of your buffer size, of course), and then validate the input is indeed the correct format.

In your case, you are seeing errors using a string like f1f1f because you are not reading in the entire STDIN buffer. As such, when you go to call scanf(...) again, there is still data inside of STDIN, so that is read in first instead of prompting the user to enter some more input. To read all of STDIN, you should do something the following (part of code borrowed from Paxdiablo's answer here: https://stackoverflow.com/a/4023921/2694511):

#include <stdio.h>
#include <string.h>
#include <stdlib.h> // Used for strtol

#define OK          0
#define NO_INPUT    1
#define TOO_LONG    2
#define NaN         3 // Not a Number (NaN)

int strIsInt(const char *ptrStr){
    // Check if the string starts with a positive or negative sign
    if(*ptrStr == '+' || *ptrStr == '-'){
        // First character is a sign. Advance pointer position
        ptrStr++;
    }

    // Now make sure the string (or the character after a positive/negative sign) is not null
    if(*ptrStr == NULL){
        return NaN;
    }

    while(*ptrStr != NULL){
        // Check if the current character is a digit
        // isdigit() returns zero for non-digit characters
        if(isdigit( *ptrStr ) == 0){
            // Not a digit
            return NaN;
        } // else, we'll increment the pointer and check the next character
        ptrStr++;
    }

    // If we have made it this far, then we know that every character inside of the string is indeed a digit
    // As such, we can go ahead and return a success response here
    // (A success response, in this case, is any value other than NaN)
    return 0;
}

static int getLine (char *prmpt, char *buff, size_t sz) {
    int ch, extra;

    // Get line with buffer overrun protection.
    if (prmpt != NULL) {
        printf ("%s", prmpt);
        fflush (stdout);
    }
    if (fgets (buff, sz, stdin) == NULL)
        return NO_INPUT;

    // If it was too long, there'll be no newline. In that case, we flush
    // to end of line so that excess doesn't affect the next call.
    // (Per Chux suggestions in the comments, the "buff[0]" condition
    //   has been added here.)
    if (buff[0] && buff[strlen(buff)-1] != '\n') {
        extra = 0;
        while (((ch = getchar()) != '\n') && (ch != EOF))
            extra = 1;
        return (extra == 1) ? TOO_LONG : OK;
    }

    // Otherwise remove newline and give string back to caller.
    buff[strlen(buff)-1] = '\0';
    return OK;
}

void validate_input(int responseCode, char *prompt, char *buffer, size_t bufferSize){
    while( responseCode != OK ||
            strIsInt( buffer ) == NaN )
    {
        printf("Invalid input.\nPlease enter integers only!\n");
        fflush(stdout); /* It might be unnecessary to flush here because we'll flush STDOUT in the
                           getLine function anyway, but it is good practice to flush STDOUT when printing
                           important information. */
        responseCode = getLine(prompt, buffer, bufferSize); // Read entire STDIN
    }

    // Finally, we know that the input is an integer
}

int main(int argc, char **argv){
    char *prompt = "Please Insert X value\n";
    int iResponseCode;
    char cInputBuffer[100];
    int x, y=0;
    int *p = &x;
    while(y < 5){
        iResponseCode = getLine(prompt, cInputBuffer, sizeof(cInputBuffer)); // Read entire STDIN buffer
        validate_input(iResponseCode, prompt, cInputBuffer, sizeof(cInputBuffer));

        // Once validate_input finishes running, we should have a proper integer in our input buffer!
        // Now we'll just convert it from a string to an integer, and store it in the P variable, as you
        // were doing in your question.
        sscanf(cInputBuffer, "%d", p);
        y++;
    }
}

Just as a disclaimer/note: I have not written in C for a very long time now, so I do apologize in advance if there are any error in this example. I also did not have an opportunity to compile and test this code before posting because I am in a rush right now.

Community
  • 1
  • 1
Spencer D
  • 3,376
  • 2
  • 27
  • 43
  • 1
    `if (buff[strlen(buff)-1] != '\n')` is a hacker exploit as `buf[0]` could be 0. Suggest ``if (buff[0] && buff[strlen(buff)-1] != '\n')`` or `buff[strcspn(buff,"\n')] = '\0';` – chux - Reinstate Monica Jun 20 '16 at 17:45
  • @chux, thanks for the suggestion. I'll edit that shortly. I copied that function from the post linked in my answer, so I had not bothered with editing it or checking to make sure everything was secure. – Spencer D Jun 20 '16 at 18:04
1

If you're reading an input stream that you know is a text stream, but that you are not sure only consists of integers, then read strings.

Also, once you've read a string and want to see if it is an integer, use the standard library conversion routine strtol(). By doing this, you both get a confirmation that it was an integer and you get it converted for you into a long.

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

bool convert_to_long(long *number, const char *string)
{
    char *endptr;

    *number = strtol(string, &endptr, 10);

    /* endptr will point to the first position in the string that could
     * not be converted.  If this position holds the string terminator
     * '\0' the conversion went well.  An empty input string will also
     * result in *endptr == '\0', so we have to check this too, and fail
     * if this happens.
     */

    if (string[0] != '\0' && *endptr == '\0')
        return false; /* conversion succesful */

    return true; /* problem in conversion */
}

int main(void)
{
    char buffer[256];

    const int max_tries = 5;
    int tries = 0;

    long number;

    while (tries++ < max_tries) {
        puts("Enter input:");

        scanf("%s", buffer);

        if (!convert_to_long(&number, buffer))
            break; /* returns false on success */

        printf("Invalid input. '%s' is not integer, %d tries left\n", buffer,
               max_tries - tries);
    }

    if (tries > max_tries)
        puts("No valid input found");
    else
        printf("Valid input: %ld\n", number);

    return EXIT_SUCCESS;
}

ADDED NOTE: If you change the base (the last parameter to strtol()) from 10 to zero, you'll get the additional feature that your code converts hexadecimal numbers and octal numbers (strings starting with 0x and 00 respectively) into integers.

Kusalananda
  • 14,885
  • 3
  • 41
  • 52