CODEWITHSUNDEEP
c
frklft
frklft

Reputation: 23

What error checking should I do within this function?

I'm a C-programmer in progress just starting to feel that I get some things right. Being used to much higher level languages (like C# and Python), I really miss my exceptions.

The following is a function I wrote and am currently using to peek into streams X number of chars.

What error checking could be done within this function to make sure it doesn't brake a program?

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

#include "speek.h"

void speek(char *peek, size_t len, FILE *stream) {

   int i;

   for (i = 0; i < len; i++) {
      peek[i] = fgetc(stream);
      if (feof(stream) || i >= len - 1) {
         // Null terminate the string if stream is EOF or requested number
         // of characters has already been fetched.
         peek[i] = '\0';
         break;
      }   
   }   

   for (i = strlen(peek); i >= 0; i--) {
      ungetc(peek[i], stream);
   }   
}

Upvotes: 2

Views: 146

Answers (4)

gbulmer
gbulmer

Reputation: 4290

I would always check the FILE* as I read characters.

I think it is a stylistic thing, but I would not put something which I normally intend to happen within the loop.

Finally, a succesfully read string should always be terminated by a '\0'. I am often torn about what to do when there was an error. If I know what the function is for, I could normally choose.

So, using your style of file checking:

void speek(char *peek, size_t len, FILE *stream) {

   int i;
   for (i = 0; i < len-1 && !feof(stream); i++) {
      peek[i] = fgetc(stream);
   }   

   peek[i] = '\0';

   for (i = strlen(peek)-1; i >= 0; i--) {
      if (ungetc(peek[i], stream) != peek[i]) {
         break;  // I'd return an error, but function is void
      }
   }   
}

If this is more than a quick experiment, or I were writing this for others, I might use assert to check parameters (yes, I know, I still do (c=getc(...)), I'm old)

#include <assert.h>

int speek(char *peek, size_t len, FILE *stream) {
   assert(peek != NULL);
   assert(len > 0);    /* Could 0 be okay? */
   assert(stream != NULL && !ferror(stream));

   int i;
   int c;
   for (i = 0; i < len-1 && (c=getc(stream)) != EOF; i++) {
      peek[i] = c;
   }   

   peek[i] = '\0';

   if (ferror(stream)) return -1;

   for (int j = i-1; j >= 0; j--) {
      if (ungetc(peek[j], stream) != peek[j]) {
         return -1;
      }
   }  
   return i;   /* I like to return something useful */ 
}

Edit: I tried to mirror the original but it isn't the way I'd do it, so I finally gave up, and wrote what I'd normally aim to write.

Edit2: ... But I made a mistake. The correct approach is to check there is room for the character before reading it. Doh!

Simple test program:

int main (int argc, const char * argv[]) {
    char line0[100];
    int n0 = speek(line0, 4, stdin);

    fprintf(stderr, "%d '%s'\n", n0, line0);

    char line1[100];
    int n1 = speek(line1, 8, stdin);

    fprintf(stderr, "%d '%s'\n", n1, line1);

    return 0;
}

Given a file containing only abcefghij gives 

3 'abc' 
7 'abcdefg'

Upvotes: 1

Celada
Celada

Reputation: 22261

As Pavan Manjunath indicates, you have bugs in your program. At least one other one that I can see right away is that you do not handle NUL characters in the file: at the end of the function you use strlen, which only measures your sting up to the first NUL byte, which isn't necessarily as many bytes as you actually read.

But that's bugs in general. You asked about error checking specifically. Nobody has mentioned the most important place to check for errors: if there is an error reading the file. You should check the return value of fgetc() for EOF as described in its manpage (preferred) or use the ferror() function afterwards to test if an error has occurred reading the file.

Checking peek and stream for NULL as suggested by Thiruvalluvar is defensive, but I wouldn't say necessary. Those shouldn't be NULL. If they are, it's your caller's fault, not yours.

Upvotes: 1

Vinicius Kamakura
Vinicius Kamakura

Reputation: 7778

Besides checking the pointers for NULL and if (len > 0) I'd advise you to check if the FILE * is a valid one. You can do it by passing it to fileno() and checking if it returns a value different from -1.

Upvotes: 0

Pavan Manjunath
Pavan Manjunath

Reputation: 28525

Adding to what @Thiruvalluvar has noted,

  1. If the file is an empty one peek would just contain a NULL character. i = strlen(peek) will return 0 but you are still entering the loop as you have i>=0 and trying to push back a character into the stream which you never got from there. The same case results even when len is zero!

  2. One more is you are not monitoring the return value of ungetc. It might return EOF on failure.

Upvotes: 0

Related Questions