1

The code snippet given below is a part of my Customer Billing System Project which I'm writing in C. Here I'm taking input of the Customer Data in a Customer structure and saving the entire structure in "CustomerRecord.dat" file using the writefile() function as defined in my code. After successfully writing the Customer data in my file, I'm trying to traverse the file and assign Customer numbers to each record. For doing this I'm using the setCustomerNo() funtion as defined in my code.

The problem is the while loop inside the setCustomerNo() funtion is running infinitely and reading even after the end of file.

Please help me solving this issue.

I've tried replacing:

  1. while(!feof(fp) { // LOOP BODY }
  2. while(!feof(fp) && !ferror(fp)) { // LOOP BODY }
  3. while(fread(&x, sizeof(Customer), 1, fp) || !feof(fp)) { // LOOP BODY }

But none of these above replacements worked for me.

#define RECORD CustomerRecord.dat
typedef struct
{
    int customerNo;
    unsigned int phoneNo;
    char name[20];
    char address[32];
    float balance;
}Customer;

void writefile(Customer x)
{
    FILE *fp;
    fp = fopen("CustomerRecord.dat", "ab");
    fwrite(&x, sizeof(Customer), 1, fp);
    fclose(fp);
}

Customer setCustomerNo()
{
    FILE *fp;
    Customer x, y;
    int k = 1;

    fp = fopen(RECORD, "rb+");
    while(!feof(fp) && !ferror(fp))
    {
        fread(&x, sizeof(Customer), 1, fp);
        x.customerNo = k++;
        y = x;
        fseek(fp, -sizeof(Customer), SEEK_CUR);
        fwrite(&x, sizeof(Customer), 1, fp);
    }
    fclose(fp);
    return y;
}

The expected result is that the function setCustomerNo() will read all the structures stored in file from the beginning and update customerNo(s) starting from 1 and so on to the structures stored in the file. At the end return the last updated structure.

Kalpadiptya Roy
  • 95
  • 1
  • 11
  • Are you sure the RECORD macro or variable is not referring to a different file other than CustomerRecord.dat? – BlueStrat Jul 17 '19 at 05:57
  • 1
    https://stackoverflow.com/questions/5431941/why-is-while-feoffile-always-wrong – Shawn Jul 17 '19 at 06:11
  • 2
    You must do a seek-like operation each change from reading to writing (and you do one), but also between each change from writing to reading (which you do not do). The standard requires that. You also do not error check any of the file operations, but you should, especially when anything is going wrong (but you should anyway). – Jonathan Leffler Jul 17 '19 at 06:11
  • 1
    The negation of a `size_t` value (such as `-sizeof(Customer)`) is probably not a negative number; it is not clear where the `fseek()` is moving the current position. You don't show what diagnostic printing you've added — such as printing the record that is read. That's a most basic debugging operation. If you don't get sensible input, you know you've got a problem. – Jonathan Leffler Jul 17 '19 at 06:17
  • 1
    Of your three alternatives, (3) is closest. It would be reasonable to use `while (fread(&x, sizeof(Customer), 1, fp) == 1)` as the loop test. (Incidentally, the second seek operation mention previously can be as simple as `fseek(fp, 0, SEEK_CUR);`.) – Jonathan Leffler Jul 17 '19 at 06:23
  • @BlueStrat Yes I have defined RECORD by #define as CustomerRecord.dat – Kalpadiptya Roy Jul 17 '19 at 08:23

1 Answers1

2

You definitely don't want your loop to test feof(fp) -- indeed, that is practically never right -- because feof(fp) will always be false at the end if the loop, so the loop will never end. Why? The fseek manpage spells it out:

A successful call to the fseek() function clears the end-of-file indicator for the stream.

Anyway, if you think about it, you want to terminate the loop exactly when fread reports that it got no data. You certainly don't want to process the non-existent data that fread didn't read. So testing whether there was an EOF after you handle the possibly unread data can't be right, even if feof(fp) actually returned a useful value.

A couple of other notes:

  1. You're better off using ftell to recall the read cursor before the read, and then fseeking to the saved point. That avoids a bunch of corner cases, and is easily upgraded to use the more general fseeko/ftello functions, which work properly with large files. (Or you could just go directly to using fseeko and ftello.

  2. Think about what you want to do if the last read is partial. Of course, that "can't happen" if you always create fixed-length records. But everything that "can't happen" eventually does, for one reason or another. Best to deal with it.

rici
  • 234,347
  • 28
  • 237
  • 341
  • Does the fseeko() and ftello() functions also clears the end-of-file indicator? – Kalpadiptya Roy Jul 17 '19 at 08:54
  • Please read the documentation. [man fseeko](http://man7.org/linux/man-pages/man3/fseeko.3.html) `The fseeko() and ftello() functions are identical to fseek(3) and ftell(3) (see fseek(3)), respectively, except that the offset argument of fseeko() and the return value of ftello() is of type off_t instead of long.` – KamilCuk Jul 17 '19 at 09:07
  • @kalpadiptya: you seem to think that resetting eof is somehow a defect or oddity of fseek, which means that you're not yet thinking clearly about the meaning of the EOF indicator. You should reread the last paragraph in my answer until you understand why checking `eof()` at the beginning of a loop is almost certainly wrong. If you fseek a stream, then it is no longer positioned at the end (probably) so it would be incorrect to indicate that it was. But that's not what eof() is used for. ... – rici Jul 17 '19 at 20:44
  • Eof() is used immediately after some input function reports "error or no more data" (like `fread` returning 0) in order to distinguish between those two cases. Obviously (I would have thought), you need to check this condition before attempting to use the data read because if the error or EOF occurs, **there is no data*". – rici Jul 17 '19 at 20:46