4
\$\begingroup\$

This question is an extension of this other question from Stackoverflow.

The objective is to limit the input of characters read by fgets to up to a certain number of characters. And in the following code, that I've come up with as a solution, we are working with a limit of 10 characters as an example:

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

int main()
{
    char v[12];
    
    printf("Type a word with up to 10 chracters:\n");
    //Loop checks if fgets succeeded and if there is '\n' in the string:
    while (!fgets(v, 12, stdin) || strcspn(v, "\n") == 11){
        if(feof(stdin)) {
             printf("Error: End of file reached.\n");
             exit(EXIT_FAILURE);
        }
        printf("Error: The word is longer than 10 characters, try again.\n");
        for(int ch=getchar(); ch != '\n' && ch != EOF; ch=getchar());
        printf("Type a word with up to 10 chracters:\n");
    }
    v[strcspn(v, "\n")] = '\0';
    
    printf("%s\n", v);

    return 0;
}

It works as intended. However @chux-ReinstateMonica has pointed out that there may still be corner weaknesses and possible improvements to be made to this solution.

So, how can this code/solution be improved?

\$\endgroup\$
4
\$\begingroup\$

This would obviously be better as a function, so we can call with different parameters.

Instead of repeating the constant 12 in the fgets() call, we could simply use sizeof v. Then it remains consistent if we change v.

Instead of strcspn() with only one character, I'd prefer strchr(). Conveniently, that returns a null pointer if not found, so we don't need to calculate the length to search. And we shouldn't repeat the search to replace the newline with null character.

Error messages should go to stderr, not stdout.

We can skip the rest of line with a simple scanf("%*[^\n]") (note the * to suppress assignment). That would replace the ch loop.

\$\endgroup\$
12
  • \$\begingroup\$ Make a function: Yes. -- Use sizeof v instead of 12: In a function I think the maximum number of characters (10 in the example) would be more important, and it would better to simply assume that the char array is large enough (length of at least the maximum number of character + 2). \$\endgroup\$
    – isrnick
    Feb 18 at 13:57
  • 1
    \$\begingroup\$ "add %*c to the format string to consume the newline" as with %*[^\n]%*c fails to read a trailing newline if it is the first character. \$\endgroup\$ Feb 18 at 23:32
  • 1
    \$\begingroup\$ A nice attribute about strcspn() is v[strcspn(v, "\n\r")] = '\0'; to handle text files from foreign systems. \$\endgroup\$ Feb 19 at 2:08
  • 1
    \$\begingroup\$ @chux: (1) yes, we need to read the newline as well as the rest of the non-newline characters. I didn't think that needed saying (but earlier comments proved otherwise!). (2) Can you give an example of where a scanf()/getchar() could fail but not be detected on the next stream operation? (3) I like that strcspn() use to handle CRLF so simply. \$\endgroup\$ Feb 19 at 7:53
  • 1
    \$\begingroup\$ TobySpeight Pedantic concern: The rare input error causes scanf(), getchar() to return EOF. The next input function may/may not return EOF - it depends on the nature of the input error. So without checking the first, an input error may lack detection. This is unlike "end-of-file", where "If the end-of-file indicator for the stream is set, or if the stream is at end-of-file". Some additional thoughts. \$\endgroup\$ Feb 19 at 16:14
3
\$\begingroup\$

Does not detect input errors on stdin

When fgets() returns NULL due to an input error, code simply loops when a loop exit is more common. If the input error is permanent, code is stuck in a infinite loop.

Avoid naked magic numbers

Rather than 12, 11, etc, use a #define BUF_N 12 and code accordingly.

// while (!fgets(v, 12, stdin) || strcspn(v, "\n") == 11){
while (!fgets(v, BUF_N, stdin) || strcspn(v, "\n") == BUF_N - 1){

Good use of int

//  vvv --- proper type to save the typical 257 different responses from `fgetc()`
for(int ch=getchar(); ch != '\n' && ch != EOF; ch=getchar());

Spell check

chracters --> characters


Sample code

Following OP's style, as a function returning a flag rather than printing errors:

// Look for an input <= N-2 non-\n characters.
// Return error/EOF flag
bool OP_readline(char *v, size_t n, const char *re_prompt) {
  char *fgets_retval;
  size_t len = 0;
  while ((fgets_retval = fgets(v, n, stdin)) != NULL && (len = strcspn(v, "\n")) + 1 == n) {
    int ch;
    while ((ch = getchar()) != '\n' && ch != EOF) {
      ;
    }
    if (ch == EOF) {
      return true; // Error or EOF;
    }
    if (re_prompt) {
      fputs(re_prompt, stdout);
      fflush(stdout):
    }
    len = 0;
  }
  v[len] = 0;
  return fgets_retval == NULL;
}

Other improvements

  1. Better code would not need a buffer of +1 size. This allows one to read directly into an array of size N without providing a N+1 size array just for the benefit the read function. Detecting long lines is the read function's problem and should not oblige the caller to provide a +1 buffer to solve it.

  2. Null character input can fool above code a bit, but leave that for another day.

  3. Detection/Handling of extreme n. Should be in the range 2 <= n <= INT_MAX, or some other well thought out limits.

  4. Consider a prompt parameter too.

\$\endgroup\$
4
  • \$\begingroup\$ Very important about input errors; should also detect input errors from getchar. \$\endgroup\$
    – Neil
    Feb 19 at 1:51
  • \$\begingroup\$ @Neil Agreed. Good to test ch. \$\endgroup\$ Feb 19 at 1:54
  • \$\begingroup\$ returning true for errors is confusing. Most other functions return true on success. ferror is a notable exception, of course. \$\endgroup\$ Feb 19 at 23:34
  • \$\begingroup\$ @RolandIllig Yes, stream was a poor choice. Code amended. \$\endgroup\$ Feb 20 at 3:18
2
\$\begingroup\$

Really nice style; super clear. I'll try not to duplicate existing and really good answers. What I think you intend to do is a kind of state machine looking something like this:

The state machine with fgets and getchar.

The error, in this case, includes stdin EOF before return was pressed. However, you are only checking for EOF and ignoring other errors, as pointed out. I believe that the fgets, getchar, and error states will probably make your code clearer.

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <assert.h>
#include <errno.h>
#include <limits.h>

// user-defined errno values; should all be negative to avoid conflicts
enum { NOT_POSIX_COMPLIANT = -2, EOF_ERROR };

/** Blocks, prompting the user again and again. If true, `v` is full up to
 `v_size`, including "". If false, `errno` is set to and expanded range. `v`
 must be capable of holding `v_size` bytes. */
static int prompt(char *const v, const size_t v_size) {
    size_t len;
    int ch;
    assert(v && v_size > 2 && v_size <= INT_MAX);
    //Loop checks if `fgets` succeeded and if there is '\n' in the string:
    for( ; ; ) {
        printf("Type a word with up to %zu chracters:\n", v_size - 2);
        if(!fgets(v, (int)v_size, stdin)) goto eof;
        if(v[len = strcspn(v, "\n")] == '\n') return v[len] = '\0', 1;
        //fprintf(stderr, "Error: input line too long.\n");
        fprintf(stderr, "Discarding malformed line.\n");
        while(ch = getchar(), ch != '\n') if(ch == EOF) goto eof;
    }
eof:
    if(feof(stdin)) errno = EOF_ERROR; // in this use-case, it is an error
    else if(!errno) errno = NOT_POSIX_COMPLIANT; // "Extension to the ISO"
    return 0;
}

int main(void) {
    char v[12];

    if(!prompt(v, sizeof v)) goto error;
    printf("%s\n", v);

    return 0;

error:
    switch(errno) {
    case NOT_POSIX_COMPLIANT:
        fprintf(stderr, "An unknown read error occurred.\n"); break;
    case EOF_ERROR:
        fprintf(stderr, "Premature EOF.\n"); break;
    default:
        perror("input");
    }
    return EXIT_FAILURE;
}

I've re-arraged some stuff using what it says in the man pages about errors to match the diagram. It's hard to do interactive programmes in purely ISO C. I would consider, depending on your application, returning failure immediately upon mis-formed input. Especially if you think there is a good reason to pipe data to stdin.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ Consider a redirected stdin where the last line of input is "abc" with no '\n'. prompt(v, 12) here rejects that short line with "Error: input line too long.\n" instead of accepting it. IMO, better to accept such lines. Lines that end with an end-of-file rather than a '\n' are often a corner case to deliberate. \$\endgroup\$ Feb 19 at 19:25
  • \$\begingroup\$ @chux-ReinstateMonica good catch; there are definitely even more edge cases that I didn't think of. On my computer, an EOF in state getchar causes the message too long and then it keeps reading. I think this is probably undesirable, too. I think a terminal control library is really what is needed, but probably overkill for most applications. \$\endgroup\$
    – Neil
    Feb 19 at 22:49
  • \$\begingroup\$ Neil, Good catch on the "keeps reading". IMO, the standard C library simple lacks a nice "read a line into a buffer" function. fgets() is OK, but has weaknesses. \$\endgroup\$ Feb 19 at 23:29
1
\$\begingroup\$

IMO a much simpler solution is to use POSIX getline(). And if your platform doesn't provide getline(), there are more than a few open-source implementations available.

Bare code to illustrate the processing in a smaller number of lines (as in without scroll bars...):

int main()
{
    char *line = NULL;
    size_t len = 0;
    for ( ;; )
    {
        printf( "Type a word with up to 10 characters:\n" );
        ssize_t result = getline( &line, &len, stdin );
        if ( result < 0 )
        {
            exit( EXIT_FAILURE );
        }

        line[ strcspn( line, "\n" ) ] = '\0';
        if ( strlen( line ) <= 10 )
        {
            break;
        }

        printf( "Error: The word is longer than 10 characters, try again.\n" );
    }

    printf( "Your input: %s\n",  line );    
    return 0;
}

The use of strcspn() and strlen() could be made more efficient so the line only gets traversed once, but we're dealing with user input here so code clarity overrides almost every performance consideration.

With full error checking:

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

int main()
{
    char *line = NULL;
    size_t len = 0;

    for ( ;; )
    {
        printf( "Type a word with up to 10 characters:\n" );

        ssize_t result = getline( &line, &len, stdin );
        if ( result < 0 )
        {
            if ( feof( stdin ) )
            {
                fprintf( stderr, "EOF reached\n" );
            }
            else
            {
                perror( "getline(..., stdin)" );
            }

            free( line );
            exit( EXIT_FAILURE );
        }

        line[ strcspn( line, "\n" ) ] = '\0';

        if ( strlen( line ) <= 10 )
        {
            break;
        }

        printf("Error: The word is longer than 10 characters, try again.\n");
    }

    printf( "Your input: %s\n",  line );

    free( line );

    return 0;
}

As a function:

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

char *getLine( unsigned int maxLen )
{
    char *line = NULL;
    size_t len = 0;

    // sanity check passed variable
    if ( 0 == maxLen )
    {
        return( NULL );
    }

    for ( ;; )
    {
        printf( "Type a word with up to %u characters:\n", maxLen );

        ssize_t result = getline( &line, &len, stdin );
        if ( result < 0 )
        {
            if ( feof( stdin ) )
            {
                fprintf( stderr, "EOF reached\n" );
            }
            else
            {
                perror( "getline(..., stdin)" );
            }

            free( line );
            return( NULL );
        }

        line[ strcspn( line, "\n" ) ] = '\0';

        if ( strlen( line ) <= maxLen )
        {
            break;
        }

        printf( "Error: The word is longer than %u characters, try again.\n",
            maxLen );
    }

    printf( "Your input: %s\n",  line );

    return( line );
}
\$\endgroup\$
6
  • 1
    \$\begingroup\$ I was assuming that the code was posted as an attempt to reimplement getline(), but you're right that's not stated, so good answer! \$\endgroup\$ Feb 18 at 16:13
  • \$\begingroup\$ What is the size used as an argument in the getline() function? Please elaborate on why a solution using getline() is better in your opinion. And if possible tell me where I can find a reliable open-source implementation of getline() that I can use with MinGW and other Windows compilers. \$\endgroup\$
    – isrnick
    Feb 18 at 19:03
  • 1
    \$\begingroup\$ @isrnick Please elaborate on why a solution using getline() is better in your opinion. Because "Get complete line, check length" is a lot simpler and less bug-prone than "read string, check length, see if it ends with a newline, if it doesn't clear input buffer"? Count the lines of code - and remember to count while (!fgets(v, 12, stdin) || strcspn(v, "\n") == 11) as five or six lines because you've put that many operations there, and for(int ch=getchar(); ch != '\n' && ch != EOF; ch=getchar()); counts as five to seven also. Every line of code you write is a chance to create bugs. \$\endgroup\$ Feb 18 at 20:21
  • \$\begingroup\$ (cont) Your code will have been tested a handful of times against a smattering of different input - if you tested it at all... Your system's getline() implementation has been tested probably trillions of times - if not more - against all kinds of input. Take glibc's implementation, for example. It was developed by an experienced team, formally reviewed by multiple experienced developers, passed through rigorous formal testing, and been open for comment for probably several decades now. \$\endgroup\$ Feb 18 at 20:23
  • 2
    \$\begingroup\$ A weakness to getline( &line, &len, stdin ); is that it allows a nefarious user to overwhelm memory resources with an excessive long line. Defensive programming prevents that. \$\endgroup\$ Feb 18 at 23:35

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.