Home > front end >  Is my code okay or will it fail in some occasion?
Is my code okay or will it fail in some occasion?

Time:05-02

I recently learned about the keyboard buffer and how important it is to clean it after getting input from the user. So I made this function to handle that:

// Get input from user
void get_input(char *buff, int size)
{
    fgets(buff, size, stdin);
    int newlineIndex = strcspn(buff, "\n");

    /* If no newline is found, fgets stopped at `size` and
    didn't read all input. Keyboard buffer is dirty */
    if (newlineIndex == strlen(buff))
    {
        // Clear keyboard buffer
        int c;
        while ((c = getchar()) != '\n' && c != EOF);
    }
    else
    {
        // Remove newline from buff
        buff[newlineIndex] = '\0';
    }
}

Here, buff is where you want to store the input and size is sizeof(buff). After using fgets to read input from the user, I then look for the \n character left by fgets with strcspn(). If newlineIndex == strlen(buff), the \n newline wasn't found, which means that the user has typed more characters than size, so I proceed to clear the keyboard buffer with:

int c;
while ((c = getchar()) != '\n' && c != EOF);

else, the keyboard buffer didn't get dirty, so I remove \n from the user input to handle with it more easily as a string later.
Is my code okay or am I missing something? Is there a more efficient way to do it?

CodePudding user response:

It is somewhat more complex and inefficient than necessary, especially given the way strings work in C. You do not need to search for the newline. It is either at the end of the string, or it is not - the only place you need to look is at the end of the string.

fgets() can return NULL is for example EOF is encountered before any characters are read - this can easily happen if using input redirection from a file for example. So you should check for that. Your code will fail in unpredictable ways as-is if buff[0] != '\0' on entry and fgets() returns NULL.

It is normally useful to return something - even if often you discard the return value. You could return an error indication or return a valid (but empty) string on error, or possibly return the length of the input string to save the caller yet another strlen() traversal. Personally I'd suggest returning a pointer to the caller's buffer and initialising buff[0] = '\0' to ensure a valid string is always returned. That way you can do things such as:

char inp[20] ;
printf( "%s\n", get_input( inp, sizeof(inp) ) ) ;

So I would suggest:

char* get_input( char* buff, size_t size )
{
    buff[0] = '\0' ;
    
    if( fgets(buff, size, stdin) != NULL )
    {
        char* endp = &buff[strlen( buff ) - 1] ;
        
        if(  *endp != '\n' )
        {
            int discard ;
            while( (discard = getchar()) != '\n' && discard != EOF ) ;
        }
        else
        {
            // Remove newline from buff
            *endp = '\0';
        }
    }
    
    return buff ;
}

One possibly useful modification, since you go to the trouble of determining the length of the input would be to return that to the caller via a reference parameter:

// Get input from user
char* get_input( char *buff, size_t size, size_t* inplen )
{
    buff[0] = '\0' ;
    size_t len = 0 ;
    
    if( fgets(buff, size, stdin) != NULL )
    {
        len = strlen( buff ) ;
        char* endp = &buff[len - 1] ;
        
        if(  *endp != '\n' )
        {
            int discard ;
            while( (discard = getchar()) != '\n' && discard != EOF ) ;
        }
        else
        {
            // Remove newline from buff
            *endp = '\0';
            len-- ;
        }
    }
    
    // Return string length via inplen if provided
    if( inplen != NULL ) *inplen = len ;
    
    return buff ;
}

Then for example:

char inp[20] ;
size_t length = 0 ;
printf( "%s\n", get_input( inp, sizeof(inp), &length ) ) ;
printf( "%lu characters entered\n", length ) ;

Or if you want to discard the length:

get_input( inp, sizeof(inp), NULL ) ;

CodePudding user response:

Yes, your code is ok, and it should not fail. However always check for fgets fail: (from manual)

If an error occurs, they return NULL and the buffer contents are indeterminate.

CodePudding user response:

.... it fail in some occasion?

Yes.

  1. Missing check of return value from fgets(). fgets() returns NULL to indicate an immediate end-of-file or input error just occurred, so should get_input(). In such cases for OP's code, buff[] is in an indeterminate state as so strcspn(buff, "\n"); risks undefined behavior (UB).

  2. Code fails to return any indication of success or error (end-of-file, input error that occurred here or excessive long line.).

  3. Extreme buffer sizes may exceed the int range. size_t size will not exceed an array or allocation size. strcspn(buff, "\n") returns a size_t, not int. Saving in an int will cause issues if the value was more than INT_MAX.

  4. strcspn(buff, "\n") fails to detect a '\n' if a prior null character was read.

Standard C lacks a robust way to read a line into a string. fgets() gets us mostly there.

  • Related