search

cginternals / libzeug

deprecated: C++ sanctuary for small but powerful and frequently required, stand alone features.
MIT License
16 stars 13 forks source link

Variant to JSON Serialization #119

Closed apopiak closed 9 years ago

apopiak commented 9 years ago

Changes

Should we delete IniPropertySerializer/IniPropertyDeserializer? Do we want an interface for serialization? Do we want to offer static methods for (de-)serialization?

mjendruk commented 9 years ago

Hi! Thanks for your contribution. Here are some comments:

The Interfaces AbstractSerializer and AbstractDeserializer are in my opinion not useful. The two subclasses have a completely different interface. One handels properties, the other variants.

Please use const parameters wherever possible, e.g., the AbstractSerializer interface does not change the given Variant, but accepts non const refs.

Something that hasn't been done yet, but is intended in the future is that libzeug shouldn't print to the standard output stream for error handling. Right now some of the added methods (e.g., AbstractSerializer::writeToFile) don't communicate possible occurring errors to the calling code. This way the user isn't able to react on errors and, e.g., display messages to the user.

Please don't implement constructors or destructors if they are not necessary. If you have to add empty destructors, use = default;, since this will enable the compiler to do specific optimizations. That means for interfaces, don't implement constructors at all and implement virtual destructors when needed with = default; in the header.

Why don't you use default arguments instead for, e.g., AbstractDeserializer::fromStream()? Moreover, do we really need a successFlag here? Isn't returning a null variant enough?