1
\$\begingroup\$

The program will read the user's input, if it's longer or equal to 10 characters, it will print it.

It's pretty compact and works fine.

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#define MAX_SIZE 1000

int main(void){
    char line[MAX_SIZE];
    int length; /*String length*/

    while((fgets(line, sizeof(line), stdin) != NULL)){
        line[length = (strlen(line)-1)] = '\0'; /*Remove the \n character*/
        if(length >= 10){
            printf("%s\n", line);
        }
    }
    return 0;
}

Not looking for anything specific, method, style, etc..

\$\endgroup\$
3
  • \$\begingroup\$ this line: line[length = (strlen(line)-1)] = '\0'; is not a good way to remove a '\n' character. A much more dependable way is: char *newline = strchr( line, '\n' ); if( newline ) { *newline = '\0'; } \$\endgroup\$ Commented Mar 16, 2017 at 17:39
  • \$\begingroup\$ @user3629249 Why is it not a good way? What are the cases where it would not be '\n' and I'd be removing an important character? \$\endgroup\$
    – user127566
    Commented Mar 16, 2017 at 18:32
  • 1
    \$\begingroup\$ A few cases: 1) windows line endings are two characters, not just one. 2) the user entered a EOF (<ctrl-d> or <ctrl-z> depending on the OS) 3) the input line is longer than the length-1 of the input buffer \$\endgroup\$ Commented Mar 17, 2017 at 19:42

2 Answers 2

1
\$\begingroup\$

Wrong functionality.

line[length = (strlen(line)-1)] always truncates line[] even if it does not end with a '\n', so /*Remove the \n character*/ is not correct.


Avoid a hacker exploit. What happens when line[0] == 0?
Injecting a null character as part of stdin is not easy, but doable.

while((fgets(line, sizeof(line), stdin) != NULL)){
  line[length = (strlen(line)-1)] = '\0';  // danger

fgets() treats a null character just like any non-'\n' characters. So if the first character read is a null character, then strlen(line) is (size_t) 0

line[length = ((size_t) 0 - 1)]
line[length = (SIZE_MAX)] = ... // access outside array bounds --> UB

The solution is to test the length or use strcspn()

size_t length = strlen(line);
if (length > 0 && line[length - 1] == '\n') line[--length] = '\0';
// or 
line[strcspn(line, "\n")] = '\0';

Code does not consume the remainder of the line, should it have more than MAX_SIZE - 1 characters in it. Failure to consume the rest of the line causes the next iteration of the loop to pick-up on the old line resulting in erroneousness reports. Entering a line of MAX_SIZE + 10 characters will demonstrate the problem.

Staying with fgets() (assumed requirement else consider fgetc()) does pose limitations, like trouble detecting null characters, yet aside from that:

while((fgets(line, sizeof line, stdin) != NULL)){
  bool eol_missing = true;
  size_t length = strlen(line);
  if (length > 0 && line[length - 1] == '\n') {
    eol_missing = false;
    line[--length] = '\0';
  }
  if (length >= 10) {
    printf("%s", line);  // or fputs()
  }

  if (eol_missing) {
    // consume rest of the line
    int ch;
    while ((c = fgetc(stdin)) != '\n' && ch != EOF) {
      fputc(ch, stdout);  // or putchar()
    }   
  }

  printf("\n"); // or putchar(), puts(), etc.
}
return 0;

Pedantic code would check if (length >= 10) fputc(ch, stdout); (pesky embedded null characters again.) and once a line was read, look for a rare input error with ferror() before calling fgetc(). If null characters in a line are a real concern for correct functionality, fgets() is not the function to use.

--

Minor: Extra outside ()

//    v----------------------------------------v        
while((fgets(line, sizeof(line), stdin) != NULL)){
// could use 
while (fgets(line, sizeof(line), stdin) != NULL) {
// or 
while (fgets(line, sizeof line, stdin) != NULL) {
// or even
while (fgets(line, sizeof line, stdin)) {
\$\endgroup\$
3
\$\begingroup\$

The magic value 10 is embedded deep in the logic; I would make it an obvious constant (it might one day come from the command-line arguments).

Before overwriting the final \0 with NUL, you will probably want to check that it is actually \n. If not, then you may have received the start of a line that's more than 1000 characters, and you'll want to print it and all the subsequent inputs until you do receive a \n (note that if you receive 1000 chars and then 5 chars, you can't ignore the 5-char input, because it's actually part of a 1005-char line). Also, consider when an input line is read as two separate 6-character chunks - your code would discard both, instead of treating them as a single 12-char unit.

Overwriting the final \n with NUL seems to me pointless here, as you add one back on when writing it out. It's probably better to keep the newline there, and increase your comparison size to take account of it.

Error checking - fgets() can fail, but you treat all failures as end-of-file. Also, we don't normally write != NULL explicitly; all non-NULL values are true, so reduce the clutter.

The scope of length can be reduced, as it's not used outside the loop. Except that I'm going to keep track of the length of lines that are split over more than one read, which moves it back again.

Minor style nit: I don't like writing the sizeof operator to look like a function. It isn't a function, and I prefer it to be distinct. I know there's a spectrum of opinion on that, so I won't push the point.

It gets surprisingly involved when you take into account partial reads and longer lines:

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

#define MAX_SIZE 1000

const int max_len = 10;

int main(void)
{
    char line[MAX_SIZE];
    int length = 0;
    int is_continuation = 0;

    while (fgets(line + length, (sizeof line) - length, stdin)) {
        length += strlen(line + length);
        /* length includes the final newline */
        if (is_continuation || length >= max_len+1) {
            printf("%s", line);
            is_continuation = line[length-1] != '\n';
            length = 0;
        } else {
            is_continuation = line[length-1] != '\n';
            if (!is_continuation)
                /* reset for next read */
                length = 0;
        }
    }

    /* do we have any remaining input?  It may or may not end in newline */
    if (length) {
        if (is_continuation || length >= max_len + (line[length-1] == '\n')) {
            printf("%s", line);
            length = 0;
            is_continuation = line[length-1] != '\n';
        }
    }

    if (feof(stdin))
        return EXIT_SUCCESS;

    perror("stdin");
    return EXIT_FAILURE;
}
\$\endgroup\$
3
  • \$\begingroup\$ the function: fgets() will not stop short of reading the whole line, so the above code makes an invalid assumption. The user 'could' enter an EOF indication rather than a '\n'. the feof(stdin) will never be true unless the code tries to read more from stdin after getting an EOF indication, Strongly suggest re-thinking this code. \$\endgroup\$ Commented Mar 16, 2017 at 17:48
  • \$\begingroup\$ If the call to fgets() does not input a '\n' (which can happen for reasons other than the input being greater than 999 characters) simply call printf() and loop, when the loop exits, all done. No need to be performing all the fiddling. \$\endgroup\$ Commented Mar 16, 2017 at 17:53
  • \$\begingroup\$ fgets() is not documented to read a full line, although stdin is likely to be line-buffered here. \$\endgroup\$ Commented Mar 16, 2017 at 18:34

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.