Critiquing my function to skip k chars from input stream.

Question

Working on (what should be) a simple project, taking the input from stdin and reformatting it to match the output specs. I'm just wanting to see what the experts here think of the following function that is supposed to skip k chars up to the end of the line, unless k < 0, where it will keep skipping chars until it reaches newline.

  1 #include <stdio.h>
 25 int skip(int count){
 26   int i;
 27   int ch;
 28
 29   for(i = 0; count < 0 || i < count; i++){
 30     ch = fgetc(stdin);
 31     if(ch == EOF){
 32       return -1;
 33     }
 34     if(ch == '\n'){
 35       return 0;
 36     }
 37   }
 38   return 1;
 39 }

(including the line numbers for reference purposes)

Answer 1

Not bad. If you worked with me and you wrote that code, I wouldn't ask you to change it.

You could decrement count until it reaches zero, instead of incrementing i until it reaches count, but it's arguable whether that would be an improvement or not.

Answer 2

There's nothing seriously wrong with this code. But to make this more robust, I would:

1. Save the value of fgetc(stdin) to an int variable, then test it to make sure it's a blank using isspace(), reporting an error if not. Use an int not a char because fgetc() returns EOF at end of file, which cannot fit inside a char.

2. Another minor concern is that on modern filesystems, file sizes can be larger than INT_MAX. Instead of "faking" a count value of INT_MAX when k < 0, I would change the loop test to test it explicitly.

In summary, the main loop becomes:

int ch;
for (i = 0; k < 0 || i < k; ++i) {
    ch = fgetc(stdin);
    if (ch == '\n') {
        return 1;
    } else if (ch == EOF) {
        return -1;        /* Or some error code of your choice */
    } else if (!isspace(ch)) {
        return -2;        /* Or some error code of your choice */
    }
}

Answer 3

I would prefer

if (k < 0) count = 0
else       count = k;

do {
  if(fgetc(stdin) == '\n') return 1;
  count--;
} while (count != 0);

return 0;

to avoid INT_MAX usage. Note that the first two lines also simply could be changed to "count = k;" and that you could change the count variable to a 64-bit type for handling inputs > 2 GB.

If you're having large inputs, you could also try to read whole lines or even the whole file into a buffer and passing a pointer to the skip function to increase performance.

Answer 4

Personally, I wouldn't use the comparison with INT_MAX and the separate count variable, and I'd decrement the supplied value rather than use a for loop.

I might even do:

while (k < 0 || k--) {
    ...
}

Note that if (k < 0) the decrement never happens due to the || operator's short-circuit behaviour, so you get an infinite loop unless you break out of the loop per your example for \n

if (k == 0) the loop will terminate immediately (nb: post-decrement rather than pre-decrement)

Answer 5

critic: you formatting:

") {" with a space between is more readable

sometimes you use "if(expression) statement;" and sometimes "if(expression){statement;}". i would always use the "{".

"while (", "if (" on also with a space after the keyword.