-2

I've some trouble with this pointer in c++ function. I would write my class into binary file, so I write this function member

void Product::saveProducts(){
    fstream file;
    Product temp;
    temp.setId(-1);
    bool flag = false;
    file.open("cigarettes.dat", ios::out | ios::binary);
    if(file.is_open()){
        file.seekg(0, ios::end);
        if(file.tellg()!=0){
            file.seekg(0, ios::beg);
            while(!file.eof()){
                file.read(reinterpret_cast<char *>(&temp), sizeof(temp));
                if(temp.getId() == this->getId()){
                    flag=true;
                    file.seekp(-sizeof(temp),ios::cur);
                    file.write(reinterpret_cast<char *>(this), sizeof(temp));
                    break;
                }
            }
        }
        if(!flag){
            file.write(reinterpret_cast<char *>(this), sizeof(temp));
        }
    }
    file.flush();
    file.close();
}

But when I try to retrieve my stored object with another function member:

list<Product> Product::loadProducsts(){
    fstream file;
    Product temp;
    list<Product> products;
    file.open("cigarettes.dat", ios::in | ios::binary);
    if(file.is_open()){
        while(!file.eof()){
            file.read(reinterpret_cast<char *>(&temp), sizeof(temp));
            products.push_front(temp);
        }
    }
    file.close();
    return products;
}

My array is filled with only one object empty. What's the problem?

Claudio Pomo
  • 2,123
  • 6
  • 38
  • 64
  • file.write(reinterpret_cast(this), sizeof(temp)); - you write only ref to object. if you object contains only POD members then you can write it to binary file and read with same system. You need use sizeof(Product) instead – iOS-programmer Oct 10 '14 at 13:14
  • 1
    You can't just dump an object to a file and read it back. You have to read/write every member individually. Tedious, but that's the only portable solution. – Neil Kirk Oct 10 '14 at 13:15
  • Have you looked at the contents of the binary file to make sure they're correct? Also, what's the declaration of `Product`? – Drew McGowen Oct 10 '14 at 13:16
  • It is not necessary to flush files before closing them. Also see http://stackoverflow.com/questions/5605125/why-is-iostreameof-inside-a-loop-condition-considered-wrong – Neil Kirk Oct 10 '14 at 13:17
  • @iOS-programmer saveProducts() and loadProducsts() are member functions, and I call saveProducts() like this p.saveProducts() to save object itself. – Claudio Pomo Oct 10 '14 at 13:19
  • @ClaudioPomo And what does that change? You still can't read from a file opened for output, and opening for output still truncates the file. Whether you do this in a member function or not doesn't change anything. – James Kanze Oct 10 '14 at 13:39
  • @iOS-programmer The restrictions are a lot more than you state. Just being POD isn't sufficient; you can't write pointers, for example, and expect to do anything with them later. And the same system isn't sufficient, the code has to be compiled with the same version of the same compiler, using the same compiler options. (I've seen such binary files become unusable just because the compiler was upgraded.) – James Kanze Oct 10 '14 at 13:41
  • Please show definition of `Product` class. It probably is not (and should not be, if you are coding C++!) something you can dump to file as single write. – hyde Oct 10 '14 at 13:54
  • @JamesKanze Under "same system" I mean all factors, which you say above. For correct write to file class should have POD type + not have virtual functions. If I write something like: file.write(reinterpret_cast(this), sizeof(Product)); then will write all included members of object, because I put sizeof(Product) - full size of object(not pointer). Of course in topic code many other mistakes. – iOS-programmer Oct 10 '14 at 14:15
  • @iOS-programmer If the class is POD, it doesn't have any virtual functions. But it can contain other pointers (e.g. a `char const*`). (For a `Product`, of course, it's almost inconceivable that there isn't a `std::string` in there somewhere, which would mean that it isn't a POD.) (FWIW: I've done something similar in the past. But we defined a fixed length `char` buffer, and formatted each field as text into a fixed length record. Which was nice, because it meant that when we migrated from Solaris on a Sparc to Linux on an Intel, there were no problems.) – James Kanze Oct 10 '14 at 14:22

1 Answers1

2

There are any number of problems with your code, starting with the fact that just dumping bits to a file generally will not give you anything useful that you can read back. The fact that you need a reinterpret_cast to use it should have tipped you off. In addition:

  • You only open the file for output, and then try to read from it. (Opening a file for output truncates it, so you've already lost all of your previous data.)

  • I'm not sure what you think you're doing with while (!file.eof()), but it's surely not correct. If for some reason, the file.read fails without hitting the end of file, you'll end up in an endless loop, for example.

  • And you're using the results of file.read without verifying that the function worked.

  • You close the file without being at the end. This truncates a file open in output mode.

  • Same problems in the read loop of the second snippet. If the file is empty, you'll still "read" one object, pushing the default constructed temp into products.

Not knowing what Product looks like, nor the initial contents of the file, it's hard to say what is really happening. But most likely:

  • You truncate the file with the open. It now has a length of 0.

  • Since file.tellg() returns 0, you never even try to read it. (If you'd tried to read it, an error would have been set, which would have made all successive operations no-ops.)

  • You then write the single element, and close the file.

The only thing I'm not too sure of: in this scenario, the file actually does contain one element. So when you try to read it, the first file.read succeeds, probably without setting eofbit, since file.read doesn't need any look ahead. And if eofbit isn't set, I would expect you to loop a second time, and push the unmodified bits in temp into products a second time.

EDIT:

FWIW: if we assume that you're in the very restricted case where just writing the data bits to the disk is valid (which normally means that you'll be rereading them later in the same process, but never from a different processs), and that the id can never be -1 in a valid Product, what you probably want to do is:

Product temp;
temp.setId( -1 );   //  This sort of thing should really be handled by a constructor
std::fstream file( "cigartettes.dat", ios::out | ios::in | ios::binary );
while ( file.read( reinterpret_cast<char*>( &temp ), sizeof(temp) && temp.getId() != getId() ) {
}
if ( file.gcount() != 0 ) {
    //      Error somewhere, we ended up reading a partial record
} else if ( temp.getId() == getId() ) {
    file.seekp( -static_cast<int>( sizeof(temp) ) );
} else {
    file.clear();
    file.flush();
}
file.write( reinterpret_cast<char const*>( this ), sizeof(*this) );
file.close();
if ( !file ) {
    //      Something went wrong somewhere...
}

Several comments:

  • Opening in both input and output is necessary. Opening only in output means that 1) the file will be truncated, and 2) any attempt to read it will fail.

  • file.read will fail if it cannot read the correct number of bytes. If it fails, it might have read some bytes anyway (and overwritten the id field in Product, and left the current pointer at some position which isn't a modulo of your object size). So you should check for this using the value from file.gcount() (which returns the number of bytes read by the last unformatted read operation—in the case of the read you're doing, this can only be different from sizeof(Product) if the read failed.

  • When specifying a negative value to seek backwards: you have to convert the results of sizeof to a signed type before doing the -. Otherwise, you'll end up with some astronomical positive value, which will cause you to try to seek beyond the end of file (which will fail).

  • When the read fails, and the number of bytes read is 0, you've reached the end of file. And set the failbit, which will cause all future operations to fail. So we have to clear the error if we're going to write to extend the data. (If we haven't reached end of file, of course, there's nothing to clear.)

  • When doing bidirectional input, after a read, you must execute either a seek or a flush before a write. (Don't ask me why; it's just what the standard says.)

  • Finally, it's good practice to verify the status of the file after closing, when all buffers have been fully flushed and passed to the OS. If for some reason, a write has failed somewhere, you want to know about it, to inform the user that the file that was output is corrupt.

I might add that the usual way of modifying just one record in a file is to copy the file to a new file, replacing or appending the changed record, and then delete the old file and rename the new. Trying to modify a file, as you are doing, can mean that you loose all of the data if something goes wrong.

James Kanze
  • 142,482
  • 15
  • 169
  • 310
  • Yes to all, but my trouble is this `file.write(reinterpret_cast(this), sizeof(this));` Not write my object but only reference to it. – Claudio Pomo Oct 10 '14 at 13:44
  • 1
    @ClaudioPomo No, it doesn't. `sizeof(this)` is the size of a pointer to `Product`, so you're only writing the first few bits of the object. – molbdnilo Oct 10 '14 at 13:51
  • @ClaudioPomo That's also a problem (that I overlooked), but it's far from the only one. – James Kanze Oct 10 '14 at 14:18