Saving an object to file

[masar77]

Hi,

I'm trying to write a register that holds an array of type Person, like this:

class Register {

private:
	Person *pArray;
	int maxElements;
	int elements;
	string fileName;

...
}

I have over loaded the << and >> operators, like this:

ofstream & operator<<(ofstream &of, const Register *r) {
	of.write( reinterpret_cast<const char *>( r), sizeof (Register));	
	return of;
}

ifstream & operator>>(ifstream &ifo, Register *r) {
	ifo.read( reinterpret_cast<char *>( r), sizeof (Register));	
	return ifo;
}

and trying to write to a file with:

ofstream fout(fileName.c_str(), ios_base::out | ios_base::binary);
fout << &reg;

However, it doesn't seem to include the array, i.e., the file doesn't increase in size with more persons in the array. The Person-class contains Name and Address objects and these have also overloaded operators.

Can anyone spot what's wrong?

Thanks.

[laserlight]

You are just writing the value of the pointer to the file.

You have presumably overloaded those operators for Person. You can iterate over the array and print them, presumably with some separator.

Incidentally, you might want to consider the use of Boost.Serialization. Also, consider using a std::vector<Person> instead of a pointer to Person.

[masar77]

Yes, you're right, I will rewrite the definition of the overloaded operator.

Thanks.

[ninja9578]

You need to write a serialization and deserialization methods. Have it print all of members and put them into some sort of standard format. That would usually be JSON or XML. Probably XML in your case because you would also have to serialize the Person objects and save them somewhere too. XML supports referencing, JSON does not.

tinyxml and libxml2 are two good options for xml and libjson is a good option for json.

[itsmeandnobodyelse]

Hi,

I'm trying to write a register that holds an array of type Person, like this:

class Register {

private:
	Person *pArray;
	int maxElements;
	int elements;
	string fileName;

...
}

I have over loaded the << and >> operators, like this:

ofstream & operator<<(ofstream &of, const Register *r) {
	of.write( reinterpret_cast<const char *>( r), sizeof (Register));	
	return of;
}

ifstream & operator>>(ifstream &ifo, Register *r) {
	ifo.read( reinterpret_cast<char *>( r), sizeof (Register));	
	return ifo;
}

and trying to write to a file with:

ofstream fout(fileName.c_str(), ios_base::out | ios_base::binary);
fout << &reg;

However, it doesn't seem to include the array, i.e., the file doesn't increase in size with more persons in the array. The Person-class contains Name and Address objects and these have also overloaded operators.

Can anyone spot what's wrong?

Thanks.

As told you currently were streaming a pointer value which is invalid when you read it back in another session. Instead you need to stream each Person of the Register, e. g. like that:

ofstream & operator<<(ofstream &of, const Register *r) {
      int n = r->getNumElements();
      of << n << "|";
      for (int i = 0; i < ; ++i)
      {
           of << r->getPersonAt(i) << "|";
      }
}

The r->getPersonAt(i) should return Person i from Register by reference or by pointer.

Then you would need a further overload of the ofstream operator<< which takes a Person as right hand operand.

Note the '|' is a seperator if you want text serialization like in

"2|John Smith, 31, New York|Bob Brown, 24, Paris|"

You also can make binary serialization where each attribute put to file has a fixed size. Then the separators were not necessary and you could reread whole structure Person with one binary read without any need to parse for seperators.

Regards, Alex

P.S.

If you can use boost serialization forget the above.

[masar77]

This is an exercise from a class I'm taking so there are constraints for what I can use. I have to use an array, and can't use more advanced methods. It would be much easier if the Register only held an array of persons, then I would just iterate through it and save them one by one. The three other fields, two ints and one string, are giving me headache. Any ideas?

Thanks.

[Lindley]

I don't see why those would be a problem....just write them before or after the Persons are written. Actually, before makes a lot more sense than after, since you need "elements" and "maxElements" (presumably) to define the size of the Person array when reading back out of the file.

[masar77]

I don't see why those would be a problem....just write them before or after the Persons are written. Actually, before makes a lot more sense than after, since you need "elements" and "maxElements" (presumably) to define the size of the Person array when reading back out of the file.

You're right. I'm rewriting the code right now. And yes, before is probably smarter.

Thanks.

[masar77]

Should this work?

ofstream & operator<<(ofstream &of, const Register *r) {
	of.write( reinterpret_cast<const char *>(r->getElements()), sizeof (int));	
	of.write( reinterpret_cast<const char *>(r->getMaxElements()), sizeof (int));	
	of.write( reinterpret_cast<const char *>(r->getFileName().c_str()), sizeof (char));	
	cout << endl << "in ofstream..." << endl;
	Person *p = r->getArray();
	for (int i = 0; i < r->getElements(); i++) {
		of.write( reinterpret_cast<const char *>(&p[i]), sizeof (Person));	
	}
	return of;
}

Obviously not, it crashes at the first line, of.write...
Access violation reading location 0x00000001

[Lindley]

Well, if getElements() returns a reference, it might be possible to write it that way. However, it's more typical to make getters return by value, in which case the above would not work. I suggest making the operator<< be a friend so that you can operate on the elements field directly.

Also, you're writing the string the wrong way. You're currect to write the c_str(), but you need to write all of the characters, not just 1. You also need to store the number of characters somewhere, or else make sure you're also storing the trailing NULL.

[masar77]

Well, if getElements() returns a reference, it might be possible to write it that way. However, it's more typical to make getters return by value, in which case the above would not work. I suggest making the operator<< be a friend so that you can operate on the elements field directly.

Also, you're writing the string the wrong way. You're currect to write the c_str(), but you need to write all of the characters, not just 1. You also need to store the number of characters somewhere, or else make sure you're also storing the trailing NULL.

Thanks!

[Paul McKenzie]

Can anyone spot what's wrong?
Thanks.

ofstream & operator<<(ofstream &of, const Register *r) {
	of.write( reinterpret_cast<const char *>( r), sizeof (Register));	
	return of;
}

Similar, if not the exact code has been posted here many times here by students/new programmers attempting to save an object to a file. The code is, of course, no good, as saving pointers to a file, as well as non-POD objects like this is invalid.

So the question is where did this code (or teaching of this type of coding) come from? It has to be the same place, since a lot of new programmers seem to be using the same erroneous reference material on this topic.

Regards,

Paul McKenzie

[masar77]

const Register &Register::operator=(const Register &r) {
	if (&r != this)
	{
		delete [] this->pArray;
		this->maxElements = r.getMaxElements();
		this->elements = r.getElements();
		this->fileName = r.getFileName();
		this->pArray = new Person[maxElements];
		Person *arrayToCopy = r.getArray();
		for (int i = 0; i < elements; i++) {
			pArray[i] = arrayToCopy[i];
		}
	}
	return *this;
}

This crashes when 'this' is NULL, which it is when starting the program. How can I fix that?

Thanks.

[Paul McKenzie]

This crashes when 'this' is NULL, which it is when starting the program. How can I fix that?

By not making mistakes when you write the code.

Seriously, how can we answer your question if we don't know what your main() program looks like? Also, "this" should never be NULL, unless you've written something that is totally invalid (which we need to see).

Also, why not use std::vector instead of all of this hand-coded memory management? Then none of this code you've written would be necessary, as it is wrong in many ways, one being that your object is totally messed up if new[] throws an exception.

Regards,

Paul McKenzie

[itsmeandnobodyelse]

const Register &Register::operator=(const Register &r) {
	if (&r != this)
	{
		delete [] this->pArray;
		this->maxElements = r.getMaxElements();
		this->elements = r.getElements();
		this->fileName = r.getFileName();
		this->pArray = new Person[maxElements];
		Person *arrayToCopy = r.getArray();
		for (int i = 0; i < elements; i++) {
			pArray[i] = arrayToCopy[i];
		}
	}
	return *this;
}

This crashes when 'this' is NULL, which it is when starting the program. How can I fix that?

Thanks.

When this == NULL you do something like

Register * pr1 = NULL;
Register * pr2 = new Register;
pr2->AddPerson(Person("John Smith");
*pr1 = *pr2;   // kaboom and this is NULL

The first advice is "don't use pointers to Register" if you can avoid it.

The second advice is "if using pointers check them for not being NULL" prior to using them.

Regards, Alex