Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

##Binary Serialization

Binary Serialization

##Design

Design

##Never make assumptions about a class

Never make assumptions about a class

##Binary Serialization

##Design

##Never make assumptions about a class

Binary Serialization

Design

Never make assumptions about a class

Source Link
Loki Astari
  • 97.7k
  • 5
  • 126
  • 341

##Binary Serialization

Is a bit more complex that what you have implemented.
You have serialized the content of the memory that is occupied by an object of type Building. Which is not quite the same as binary serialization of an object.

Problem 1: Integers are not represented the same on all platforms.
Problem 2: Floats are not represented the same on all platforms.

Those are the obvious two. The not so obvious ones are packing added by the compiler to the object. Changing the compiler or even just the compiler flags can change both the packing (and thus alignment) and the size of the object.

Assuming all the above is handled and does not change. The above code is very fragile to any change. It assumes all the data is in the object. This is usually not the case (think of a string, most of the data is at the other end of a pointer). So just because you serialize the memory occupied by an object does not always mean you get all the data that belongs to an object.

To use an optimal binary serialization. I would serialize each member individually. That way at least you avoid the issues with packing introduced by the compiler.

##Design

Not sure what m_buildingQualityDecimals is used for or why it is useful. If you are worried about the exactness of floating point then you should retain the data as a string or integer (depending on the situation).

##Never make assumptions about a class

 file.write((char*) &b, sizeof(b)); // writing file to binary by casting to a char*

Here the external user is serializing the class. But has no idea what is inside the class. This makes this code impossibly brittle to change. It will almost certainly break over time.

What you should do is ask the object to serialize itself to the stream. The object understands itself and its members. So it can optimally serialize itself to the stream.

 b.write(file); // b can serialize itself to the stream.
 class Building
 {
 // Stuff
 void write(std::ostream& out)
 {
 char version = 1;
 out.write(&version, 1);
 out.write(&m_age, sizeof(m_age));
 out.write(m_locationCode, 7);
 out.write(&m_buildingQuality, sizeof(m_buildingQuality));
 }

The same applies to the read.

 file2.read((char *) b2, sizeof(*b2));
 // I would write...
 b2->read(file);

Normal serialization

// Testing process
std::cout << "Age: " << b2->getAge() << "; Loc: " << b2->getLoc() <<
 "; Building Quality:" << std::setprecision(b2->getDecimals()) << b2->getQuality() <<std::endl;

Again you are providing geter methods to access the member and retrieve values that you then use to perform an action. Geter methods break encapsulation and expose the internal types of the object. You should never use them like this. What you should do is ask the class to serialize itself to the stream. There is even a standard idiom for this in C++ and it is called the output operator.

 class Building
 {
 friend std::ostream& operator<<(std::ostream& out, Building const& value)
 {
 return out << "Age: " << value.m_age 
 << "; Loc: " << value.m_locationCode 
 << "; Building Quality:" << value.m_buildingQuality
 << "\n";
 }

Prefer "\n" over std::endl

The only difference between the two is that std::endl also flushes the stream. The stream will flush itself automatically when it needs to and thus forcing a flush is only going to make it less efficient. Humans are notoriously bad at making this decision; the code is actually very good at. So let the code decide when to flush.

Don't use pointers in C++

 Building* b2 = new Building(0, "000-000", "0.0");
 // what happens if an exception happens between these two points?
 delete b2;

Using pointers is very C like and its not done much in modern C++ (unless you are implementing a container class or smart pointer or trying to do some tricky low level optimization).

In normal code you very rarely see them. The problem is that they do not show ownership semantics (the owner of a pointer is responsible for calling delete on it). So it is not clear who should delete them. So in most cases normal variables are a preferred and if you must dynamically create an object then you should be using a smart pointer so that the ownership semantics are specified.

In this case a normal object is best.

lang-cpp

AltStyle によって変換されたページ (->オリジナル) /