Trimming fgets(): strlen() or strtok()

Question

After a successful fgets(buffer, ...), it is often desirable to trim a potential End-of-Line '\n'.

Of the following 2 methods, are there any shortcomings?

#include <stdio.h>

char buffer[100];

// Method 1
while (fgets(buffer, sizeof buffer, stdin) != NULL) {
  size_t len = strlen(buffer);
  if (len > 0 && buffer[len - 1] == '\n')
    buffer[--len] = '\0';

  // use buffer
  printf("\"%s\"\n", buffer);
}

// Method 2
while (fgets(buffer, sizeof buffer, stdin) != NULL) {
  strtok(buffer, "\n");

  // use buffer
  printf("\"%s\"\n", buffer);
}

Notes:

1. When reading text files from alternate file systems, strtok(buffer, "\r\n") looks useful.
2. strtok() may incur an issue with another strtok() sequence.

Answer 1

For what it is worth, using strlen is much more efficient than using strtok. I did some tests (omitting the file access) using strlen, strchr, strtok, strstr, strpbrk and strcspn:

It surprised me that to execute this neat looking thing:

s = strchr(s, '\n');
if (s) {
    *s = '\0';
}

takes nearly 50% longer than this ugly looking sucker:

size_t len = strlen(s);
if (len && (s[len-1] == '\n')) {
    s[len-1] = '\0';
}

and the beautiful

strtok(s, "\n\r");

and

strsep(&s, "\n\r");

are 10 to 20 times slower. The latter two do of course look for \r as well as \n. But even adding \r, the strlen approach is still quicker than the strchr and still quicker then strtok and strsep by 10 to 20 times:

size_t len = strlen(s);
if (len && ((s[len-1] == '\n') || (s[len-1] == '\r'))) {
    s[len-1] = '\0';
}

Note that I wasn't exhaustive with string variations. In fact I didn't even break a sweat. And of course this is all quite irrelevant as the stdio call will dominate in your target code :-)

Note also from the comments below that if the first character of the search string (s in the examples above) is one of the characters in the pattern string, strtok does nothing (thanks to @chux).

Answer 2

Why not use the scanf() functionality do the work for you.

char buffer[100];

int  len;
if (fscanf(stdin, "%99[^\n]%n", buffer, &len) != 1)
{
     // No data (or just a '\n')
     len = 0;
}
fscanf(stdin, "%*[^\n]"); /* If the line is longer than 99 this throws away
                           * The rest of the line upto the new line character
                           */
buffer[len] = '\0';
fscanf(stdin, "%*c");     /* read and throw away the newline 
                           */

Answer 3

For completeness, I tried a few alternative ways of doing the file access. I tried your fgets and the suggested alternatives from other answers.

On my system, using fgets, which you used, is about the same speed as using getline. Using fscanf is about 3x slower than these solutions. The fastest method I tested was to use fgetln (available on a Mac from the BSD C library), which was 10-15% faster than fgets/getline.

char *line = NULL;
size_t len = 0;

while ((line = fgetln(f, &len)) != NULL) {
    if (line[len - 1] == '\n') {
        line[len - 1] = '\0';
    }
    //printf("\"%s\"\n", line);
}
if (ferror(f)) {
    perror("fgetln");
}

Another alternative is to use mmap to map the file (only real files though, not pipes, etc) into memory and use string handling directly (strchr etc) to look for line endings. This might be the fastest of all but I didn't test it here.

Generally it is best not to get too hung up about speed unless you know that a piece of code is critical (and normally one doesn't know this). So think more about the advantages and disadvantages of the various solutions:

• fgets is quick but uses a fixed buffer length and you have to handle overflow as a special case.

• getline and fgetln are quick, elegant and capture complete lines. These are attractive if we do not need to worry about attacks by huge strings. fgetln is not in any standard so is rather less portable than getline (which seems to be POSIX). I tried fgetln with a file containing one line > INT_MAX chars long (INT_MAX is the limit that fgetln handles) and although it did slow down the system considerably, it failed cleanly with a memory allocation failure:

  fgetln: Cannot allocate memory
  

  getline has a limit of SSIZE_MAX which is much bigger.

• fscanf - I can't think of a reason to use this for the simple example you pose, but it has its uses if you want to control the input format.

• mmap is (probably) quick and elegant but only handles real files, which restricts the application a bit (eg you can't just pipe input into the application if it wants to map a file).

Answer 4

It seems silly to have to call strlen() or strtok() to find the end-of-line delimiter, when surely the system could have already known how many bytes were successfully read into buffer.

Also consider how you want to handle the case where the input line is too long for the buffer.

To take care of both issues, I suggest using getline() instead:

char *line = NULL;
size_t buflen = 0;
ssize_t len;

while ((len = getline(&line, &buflen, stdin)) > 0) {
    if (line[len - 1] == '\n') {
        line[--len] = '\0';
    }

    // use buffer
    printf("\"%s\"\n", line);
}