HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

JSON Serializer

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
jsonserializerstackoverflow

Problem

Carrying on from:

  • Yet another C++ Json Parser



  • Yet another C++ Json Parser (Recursive)



All the code is available from git hub: ThorsSerializer but only reviewing a small section here.

The idea is that using this library it should be easy to define how a class is serialized without writing any code. See example in README

```
#ifndef THORSANVIL_SERIALIZE_JSON_SERIALIZE_H
#define THORSANVIL_SERIALIZE_JSON_SERIALIZE_H

/* Content:
*
* Used to define how a class is imported/exported via the Json Serialized
*
* Conventions:
* T: The type of the base class
* I: The type of the member
* MP: The type of the member pointer
*
* S: Source type (std::ostream output) (ThorsAnvil::Json::ScannerSax input)
*
* Every type that is serializeable has a class JsonSerializeTraits.
* For basic types (int/float etc) no definition is required and the default template version works
* For compound types you need to define the type SerializeInfo.
* SerializeInfo: It is a mpl vector of types.
* Each type (in the vector) defines how to serialize
* a member of the compound type.
*
* Boilerplate code to create the appropriate types for SerializeInfo.
* #define THORSANVIL_SERIALIZE_JsonAttribute(className, member)
*
* Example:
* class MyClass
* {
* // STUFF
*
* // If any members are private and need to be serialized then
* // JsonSerializeTraits must be a friend of the class so it can generate the appropriate code
*
* friend class JsonSerializeTraits;
* };
*
* namespace ThorsAnvil { namespace Serialize { namespace Json {
* template<>
* class JsonSerializeTraits
* {
* static ThorsAnvil::Serialize::Json::JsonSerializeType const type = Map;
*
* THORSANVIL_SERIALIZE_JsonAttribute(MyClass, member1);
* THORSANVIL_SERIALIZE_JsonAttribute(MyClas

Solution

Before even trying to review this by nature incredible complex code I would comment on your {}-religion, code wrap, and vertical spacing.

It is incredible personal what works best for you, me and others. Some of these comments might seem contradictory to each other, but some sacrifices might be made to increase code readability.

The following is all my personal preferences

First {}-religion, '{}' should support the structure of the program and make it easy through visual inspection of the code easier.

template
struct JsonSerializeBrace
{
    static char braces[];
};


The '{' simply doesn't provide more information, the indention already tells you the next line is dependent on the previous.

template
struct JsonSerializeBrace {
    static char braces[];
};


This provides exactly the same visual information namely that braces is part of the struct.

The additional benefit of this is that it avoids some vertical scrolling.

This block hides from a quick visual inspection that there is a control dependency

if (!item.first)
{   stream << ',';
}


If you instead write

if (!item.first) {   
     stream << ',';
}


or

if (!item.first)
    stream << ',';


followed by a blank line, the control flow is more obvious.

Next the vertical spacing, like in texts and papers the spacings are there to make it easier on the eye and improve visual search.

This code looks like a wall of text

template
struct MemberPrinter
{
    // A normal object just prints itself.
    void operator()(std::ostream& stream, T const& source)
    {
        stream 
struct MemberPrinter
{
    void operator()(std::ostream& stream, T const& source)
    {}
};


instead use a vertical spacing between the structs, functions or other distinct entities.

template
struct MemberPrinter {
    // A normal object just prints itself.
    void operator()(std::ostream& stream, T const& source) {
        stream 
struct MemberPrinter {
    void operator()(std::ostream& stream, T const& source) {
    }
};


Now we can easily see that there 2 distinct structs, sacrificing 1 of the saved lines from the '{' move.

* THORSANVIL_SERIALIZE_JsonGenericMapAttributeAccess:  A generic accessor can be used to generate multiple items.
 *                                                      When de-serializing the Json can be applied to multiple elements.
 *                                                      Used manly for container classes like std::map


This might be more readable if there is a break after the ':'

* THORSANVIL_SERIALIZE_JsonGenericMapAttributeAccess:  
 *     A generic accessor can be used to generate multiple items.
 *     When de-serializing the Json can be applied to multiple elements.
 *     Used manly for container classes like std::map


Code wrapping is also a problem when I try to read others code, what does this say? there is actually 2 different problems here, one is the long line length, the 2nd is what does this long line actually do.

boost::mpl::for_each::SerializeInfo>(MPLForEachActivateItem(scanner, destination));


Any horizontal scrolling takes extra time, more than vertical usually. Breaking the line at either '(),=' in order as they bind less strongly. '//' comments after the code might also have to be moved, usually by inserting a new line before and use same indention as the line from which it is moved.

boost::mpl::for_each::SerializeInfo>(
        MPLForEachActivateItem(
            scanner, destination
        ) // optionally combine the 2 ')'
    );


Ah, it is actually 2 function calls, heavily templatized, so this code

template::SerializeInfo>
struct MemberScanner {
    void operator()(ThorsAnvil::Json::ScannerSax& scanner, T& destination) {
        boost::mpl::for_each::SerializeInfo>(
            MPLForEachActivateItem(
                scanner, destination
        ));
    }
};


has a break at the last ',' for the control, ie. the template arguments, before eol, then at the '=' in the template argument.

Now we can see that

typename MemberToSerialize = 
             typename JsonSerializeTraits::SerializeInfo>


might be what you meant for the template argument in the 'for_each', which I couldn't before. If this is not just for SFINAE, then 'MemberToSerialize' should be used.

template::SerializeInfo>
struct MemberScanner {
    void operator()(ThorsAnvil::Json::ScannerSax& scanner, T& destination) {
        boost::mpl::for_each(
            MPLForEachActivateItem(
                scanner, destination
        ));
    }
};


If this is what you actually meant (and not just me misunderstanding what your trying to do) making it a little easier to read.

This leaves me with one eye-sore for this, namely the long namespace+typenames, using 'using', typedef or alias can help here.

```
template::SerializeInfo>
struct MemberScanner {
// only pollute this scope, alternatively use typedef/alias
using ThorsAnvil:

Code Snippets

template<JsonSerializeType>
struct JsonSerializeBrace
{
    static char braces[];
};
template<JsonSerializeType>
struct JsonSerializeBrace {
    static char braces[];
};
if (!item.first)
{   stream << ',';
}
if (!item.first) {   
     stream << ',';
}
if (!item.first)
    stream << ',';

Context

StackExchange Code Review Q#11138, answer score: 3

Revisions (0)

No revisions yet.