deleting a structure from file in c++

Question

So, here's the code of the procedure which reads every structure from file, deletes first-found structure which has an AgreementNo that is equal to the inserted int query. It then shortens the array and rewrites the file.

The problem is, it just shortens the array and deletes the last element - as if the searching criterias are not met, even though they should be.

(Before the procedure starts, the file is opened in a+b mode, so in the end, it is reopened that way.)

void deleteClient(int query, FILE *f){ int filesize = ftell(f); int n = filesize/sizeof(Client); Client *c = new Client[n]; Client *c2 = new Client[n-1]; rewind(f); fread(c, sizeof(Client), n, f); for(int i=0; i<n; i++){ if(c[i].agreementNo == query ){ c[i] = c[n]; break; } } for (int i=0; i<n-1; i++){ c2[i] = c[i]; } // reduce the size of the array ( -1 extra element) fclose(f); remove("Client.dat"); f = fopen("Client.dat", "w+b"); for(int i=0;i<n-1; i++) { fwrite(&c2[i], sizeof(Client), 1, f); } fclose(f); f = fopen("Client.dat", "a+b"); }

What could be the cause of the described problem? Did I miss something in the code?

What could be the cause of the described problem? Did I miss something in the code? :[ — Matthew T
– Matthew T, Commented May 29, 2014 at 12:57
For one thing, c[i] = c[n]; is going to fill c[i] with garbage past the end of the array, and not save a copy of c[n - 1], the last element in the array. — Paul Roub
– Paul Roub, Commented May 29, 2014 at 13:03
Also, whatever called you isn't going to get that new f you opened at the end. They'll still have the (now closed) FILE pointer they passed in. — Paul Roub
– Paul Roub, Commented May 29, 2014 at 13:05
They passed in a FILE handle. You closed that handle. It is now useless, and possibly dangerous. You open a new file handle, to a different file, with the same name (even if it was the same file, the old handle is still likely useless). That handle is then leaked and lost as the function ends. — Paul Roub
– Paul Roub, Commented May 29, 2014 at 13:11

John Zwinck · Accepted Answer · 2014-05-29 13:29:19Z

I'd do it this way:

struct MatchAgreementNo { MatchAgreementNo(int agree) : _agree(agree) {} bool operator()(const Client& client) { return client.agreementNo == agree; } }; void deleteClient(int query, FILE *f) { int rc = fseek(f, 0, SEEK_END); assert(rc == 0); long filesize = ftell(f); int n = filesize / sizeof(Client); assert(filesize % sizeof(Client) == 0); Client *begin = mmap(NULL, filesize, PROT_READ|PROT_WRITE, MAP_SHARED, fileno(f), 0); assert(begin != MAP_FAILED); Client *end = std::remove_if(begin, begin + n, MatchAgreementNo(query)); rc = ftruncate(fileno(f), (end - begin) * sizeof(Client)); assert(rc == 0); munmap(begin, filesize); }

That is, define a predicate function which does the query you want. Memory-map the entire file, so that you can apply STL algorithms on what is effectively an array of Clients. remove_if() takes out the element(s) that match (not only the first one), and then we truncate the file (which may be a no-op if nothing was removed).

By writing it this way, the code is a bit higher-level, more idiomatic C++, and hopefully less error-prone. It's probably faster too.

assert is debug-only, so what happens if the code is not run in debug mode and the file is not there?
@Ashalynd: you'd want to replace my asserts with your own error handling function. Since C++ doesn't come with a great built-in one, I assume every application has its own and can plug it in. I put the asserts more as a guide of where to handle errors than how to handle them. To your question of what happens if the file is not there: it is a precondition of the function that f points to a valid FILE. I think that's not unreasonable.
totally agree, and nice trick with working with file via memory map. By the way, are there any size limitations?
Size limitations, let's see. If long on your platform is 32 bits, then 2 GB is the max filesize; this can be solved by using ftello() instead. Otherwise, no, there is no practical limit.
Do you think std::remove_if can tackle collections of arbitrary size without problems?

Ashalynd · Accepted Answer · 2014-05-29 13:43:05Z

one change needed in your code is to save the index of the first found "bad" entry somewhere, and then copy your original array around that entry. Obviously, if no "bad" entry is found, then you aren't supposed to do anything.

One word of warning: the approach of reading the original file as a whole is only applicable for relatively small files. For the larger files, a better approach would be opening another (temporary) file, reading the original file in chunks and then copying it as you go (and after you found the entry which is skipped just copying the rest of the contents). I guess there is even more space for the optimization here, considering that except for that one entry, the rest of file contents is left unchanged.

void deleteClient(int query, FILE *f){ int filesize = ftell(f); int n = filesize/sizeof(Client); int found = -1; Client *c = new Client[n]; Client *c2 = new Client[n-1]; rewind(f); fread(c, sizeof(Client), n, f); for(int i=0; i<n; i++){ if(c[i].agreementNo == query ){ printf("entry No.%d will be deleted\n", i); found = i; break; } } if(found == -1) return; if (i>0) for (int i=0; i<found; i++) { c2[i] = c[i]; } // copy the stuff before the deleted entry if it's >0 for (int i=found+1; i<n; i++){ c2[i-1] = c[i]; } // reduce the size of the array ( -1 extra element) fclose(f); remove("Client.dat"); f = fopen("Client.dat", "w+b"); for(int i=0;i<n-1; i++) { fwrite(&c2[i], sizeof(Client), 1, f); } fclose(f); f = fopen("Client.dat", "a+b"); }

Run it again with the printf statement and see which entry was found - may be it was the last one?
Ahhh, I realized my foolish mistake! My input was scanf("%s",&query);, where it should have been %i. Now it works fine, thanks!
@user3355318: If you enable warnings in your compiler, you won't be caught out by silly mistakes like bad scanf formats.

Collectives™ on Stack Overflow

deleting a structure from file in c++

2 Answers 2

6 Comments

4 Comments

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

6 Comments

4 Comments

Related