JSON Serializer

Question

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<MyClass> must be a friend of the class so it can generate the appropriate code
 *
 * friend class JsonSerializeTraits<MyClass>;
 * };
 *
 * namespace ThorsAnvil { namespace Serialize { namespace Json {
 * template<>
 * class JsonSerializeTraits<MyClass>
 * {
 * static ThorsAnvil::Serialize::Json::JsonSerializeType const type = Map;
 *
 * THORSANVIL_SERIALIZE_JsonAttribute(MyClass, member1);
 * THORSANVIL_SERIALIZE_JsonAttribute(MyClass, member2);
 * THORSANVIL_SERIALIZE_JsonAttribute(MyClass, member3);
 * typedef boost::mps::vector<member1, member2, member3> SerializeInfo;
 * };
 * }}}
 *
 * Now we can serialize/deserialize with:
 * std::cout << jsonExport(myClassObj) << "\n";
 * std::cin >> jsonInport(myClassObj);
 *
 * Same for std::vector
 * std::vector<MyClass> vec; // Will serialize any fundamental type or type with JsonSerializeTraits<> specialized for it
 * // If JsonSerializeTraits<> is not specialized for a compound type you get a compile time
 * // error
 * std::cout << jsonExport(vec) << "\n";
 * std::cin >> jsonImport(vec);
 */
#include "json/ScannerSax.h"
#include "json/ParserShiftReduce.tab.hpp"
#include <boost/mpl/at.hpp>
#include <boost/mpl/pop_front.hpp>
#include <boost/mpl/for_each.hpp>
#include <boost/mpl/vector.hpp>
#include <boost/mpl/bool.hpp>
#include <boost/type_traits/is_fundamental.hpp>
#include <boost/typeof/typeof.hpp>
#include <iostream>
/*
 * Helper Macros:
 * 
 * These are macros that will build some boilerplate types needed by the serialization code.
 *
 * THORSANVIL_SERIALIZE_JsonAttribute: This is the main macro used.
 * It identifies a class member that will be serialized
 *
 * THORSANVIL_SERIALIZE_JsonAttribute_1: Used internally (should probably not be used by others).
 * THORSANVIL_SERIALIZE_JsonAttributeAccess: If you want to run some code to as part of the serialization processes
 * this macro allows you to specify a type that will be used during serialization.
 * Examples will be provided in the documentaion.
 *
 * 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
 *THORSANVIL_SERIALIZE_JsonGenericArrAttributeAccess: A generic accessor used by for arrays rather than maps (std::vector)
 * But otherwise identical to THORSANVIL_SERIALIZE_JsonGenericMapAttributeAccess
 */
#define THORSANVIL_SERIALIZE_JsonAttribute(className, member) \
 typedef BOOST_TYPEOF(((className*)01)->member) JsonAttribute ## member ## Type; \
 THORSANVIL_SERIALIZE_JsonAttribute_1(className, member, JsonSerializeTraits<JsonAttribute ## member ## Type>)
#define THORSANVIL_SERIALIZE_JsonAttribute_1(className, member, SerTraits) \
 typedef BOOST_TYPEOF(&className::member) JsonAttribute ## member ## TypePtr; \
 typedef JsonSerialElementAccessor<className, JsonAttribute ## member ## TypePtr, SerTraits> JsonAttribute ## member ## Accessor; \
 struct member: JsonSerializeItem<className, JsonAttribute ## member ## Accessor, std::string> \
 { \
 member() \
 : JsonSerializeItem<className, JsonAttribute ## member ## Accessor, std::string>(#member, &className::member) \
 {} \
 }
#define THORSANVIL_SERIALIZE_JsonAttributeAccess(className, member, accessor) \
 struct member: JsonSerializeItem<className, accessor, std::string> \
 { \
 member() \
 : JsonSerializeItem<className, accessor, std::string>(#member, accessor()) \
 {} \
 }
#define THORSANVIL_SERIALIZE_JsonGenericMapAttributeAccess(className, accessor) \
 struct genericAccessor: JsonSerializeItem<className, accessor, std::string> \
 { \
 genericAccessor() \
 : JsonSerializeItem<className, accessor, int>(-1 , accessor()) \
 {} \
 }
namespace ThorsAnvil
{
 namespace Serialize
 {
 /* External dependencies from the generic Serialization code */
 template<typename T, typename Parser>
 struct Importer;
 template<typename T, typename Printer>
 struct Exporter;
 namespace Json
 {
/* Three basic element types: Invalid (this obejct is not a top level JSON object)
 * Map A JSON object { [<string> : <value> [, <string> : <value>]*]? }
 * Array A JSON array [ [<value> [, <value>]*]? ]
 */
enum JsonSerializeType {Invalid, Map, Array};
/*
 * All objects that want to be serialized by this code must define their own specialization of this class.
 * The default version will cause compilation errors. Which hopefully will bring the reader here.
 */
template<typename T>
struct JsonSerializeTraits
{
 static JsonSerializeType const type = Invalid;
 typedef T SerializeInfo;
};
// Forward declarations
template<typename T, typename A, typename RegisterKey>
struct JsonSerializeItem;
/* Class used by boost::mpl::for_each. Nothing special simple lamba's will replace them in the future */
/*
 * T The object Type
 * S The source type (parser or std::ostream)
 *
 * This is basically trying to templatize and remove the need for multiple action objects that
 * are called from mpl::for_each
 */
template<typename T, typename S>
struct MPLForEachActivateTrait;
template<typename T>
struct MPLForEachActivateTrait<T, std::ostream>
{
 typedef const T ObjectType;
 static int const SerializeActionIndex = 0;
};
template<typename T>
struct MPLForEachActivateTrait<T, ThorsAnvil::Json::ScannerSax>
{
 typedef T ObjectType;
 static int const SerializeActionIndex = 1;
};
template<typename T, typename S>
class MPLForEachActivateItem
{
 typedef MPLForEachActivateTrait<T, S> Traits;
 typedef typename Traits::ObjectType ObjectType;
 S& pump;
 ObjectType& object;
 public:
 MPLForEachActivateItem(S& p, ObjectType& obj)
 : pump(p)
 , object(obj)
 {}
 // Depending on if the pump is a stream or a scanner
 // Calls JsonSerialize::activate()
 // or JsonDeSerialize::activate()
 template<typename SerializeItem>
 void operator()(SerializeItem const& item) const
 {
 typedef typename boost::mpl::at_c<typename SerializeItem::SerializeType, Traits::SerializeActionIndex>::type SerializeAction;
 SerializeAction::activate(item, pump, object);
 }
};
/*
 * Objects of this type get stored in the
 * JsonSerializeTraits::SerializeInfo
 * This is what the user create with the macros above. The default A is JsonSerialElementAccessor
 * But user can define their own action for complex objects This wrapper is merely a vehicle for
 * calling the A methods in a controlled manner.
 *
 * Note: These classes are not designed to be used directly but via the macros:
 * THORSANVIL_SERIALIZE_JsonAttribute
 * THORSANVIL_SERIALIZE_JsonAttributeAccess
 * See the notes by these macros for details
 */
template<typename T, typename A, typename RegisterKey, JsonSerializeType type = JsonSerializeTraits<T>::type>
struct JsonSerialize;
template<typename T, typename A, typename RegisterKey>
struct JsonSerialize<T, A, RegisterKey, Map>
{
 // Generic serialization of a JSON object
 static void activate(JsonSerializeItem<T, A, RegisterKey> const& item, std::ostream& stream, T const& src)
 {
 if (!item.first)
 { stream << ',';
 }
 stream << '"' << item.memberName << '"' << ":";
 item.accessor.serialize(src, stream);
 }
};
template<typename C, typename A, typename RegisterKey>
struct JsonSerialize<C, A, RegisterKey, Array>
{
 // Generic serialization of a JSON array
 static void activate(JsonSerializeItem<C, A, RegisterKey> const& item, std::ostream& stream, C const& src)
 {
 if (!item.first)
 { stream << ',';
 }
 item.accessor.serialize(src, stream);
 }
};
template<typename T, typename A,typename RegisterKey, JsonSerializeType type = JsonSerializeTraits<T>::type>
struct JsonDeSerialize;
template<typename T, typename A,typename RegisterKey>
struct JsonDeSerialize<T, A, RegisterKey, Map>
{
 static void activate(JsonSerializeItem<T, A, RegisterKey> const& item, ThorsAnvil::Json::ScannerSax& parser, T& dst)
 {
 std::auto_ptr<ThorsAnvil::Json::SaxAction> action(item.accessor.action(dst));
 parser.registerAction(item.memberName, action);
 }
};
template<typename T, typename A>
struct JsonDeSerialize<T, A, int, Array>
{
 static void activate(JsonSerializeItem<T, A, int> const& item, ThorsAnvil::Json::ScannerSax& parser, T& dst)
 {
 std::auto_ptr<ThorsAnvil::Json::SaxAction> action(item.accessor.action(dst));
 parser.registerActionOnAllArrItems(action);
 }
};
template<typename T, typename A>
struct JsonDeSerialize<T, A, std::string, Array>
{
 static void activate(JsonSerializeItem<T, A, std::string> const& item, ThorsAnvil::Json::ScannerSax& parser, T& dst)
 {
 std::auto_ptr<ThorsAnvil::Json::SaxAction> action(item.accessor.action(dst));
 parser.registerActionNext(action);
 }
};
/*
 * A type holder object that picks up the correct versions of JsonSerialize and JsonDeSerialize
 * Used by MPLForEachActivateItem to get the correct type
 */
template<typename T, typename A, typename RegisterKey>
struct JsonSerializeItem
{
 RegisterKey memberName;
 A accessor;
 bool first;
 JsonSerializeItem(RegisterKey const& name, A const& ac): memberName(name), accessor(ac), first(false) {}
 JsonSerializeItem& makeFirst() {first = true;return *this;}
 typedef JsonSerialize<T,A,RegisterKey> Serialize;
 typedef JsonDeSerialize<T,A,RegisterKey> DeSerialize;
 typedef boost::mpl::vector<Serialize, DeSerialize> SerializeType;
};
/*
 * Importing
 * ============
 *
 * The JsonImportAction defines a class that is registered with a Json SAX parser
 * so that we can register callback actions for particular attributes.
 *
 * For fundamental types json is read directly into the value.
 * For compound types when the attribute is reached additional callbacks are registered
 * for each of the compound members that needs to be de-serialized (this is done recursively)
 * So we can de-serialize arbitrary json structures.
 */
template<typename SerializeInfo, typename I, bool EnablePod = boost::is_fundamental<I>::value>
class JsonImportAction: public ThorsAnvil::Json::SaxAction
{
 I& memberRef;
 public:
 JsonImportAction(I& mr)
 : memberRef(mr)
 {}
 virtual void doPreAction(ThorsAnvil::Json::ScannerSax&, ThorsAnvil::Json::Key const&){}
 virtual void doAction(ThorsAnvil::Json::ScannerSax&, ThorsAnvil::Json::Key const&, JsonValue const& value)
 {
 // Read fundamental type directly into the member
 memberRef = value.getValue<I>();
 }
};
template<typename SerializeInfo, typename I>
class JsonImportAction<SerializeInfo, I, false>: public ThorsAnvil::Json::SaxAction
{
 I& memberRef;
 public:
 JsonImportAction(I& mr)
 : memberRef(mr)
 {}
 virtual void doAction(ThorsAnvil::Json::ScannerSax&, ThorsAnvil::Json::Key const&, JsonValue const&){}
 virtual void doPreAction(ThorsAnvil::Json::ScannerSax& parser, ThorsAnvil::Json::Key const&)
 {
 // Compound type. Register callback for each member.
 // This is done when the attribute is reached in json not before
 boost::mpl::for_each<SerializeInfo>(MPLForEachActivateItem<I, ThorsAnvil::Json::ScannerSax>(parser, memberRef));
 }
};
/*
 * Need a function template to create the correct JsonImportAction()
 */
template<typename SerializeInfo, typename T, typename I>
ThorsAnvil::Json::SaxAction* new_JsonImportAction(T& dst, I T::* memberPtr)
{
 return new JsonImportAction<SerializeInfo, I>(dst.*memberPtr);
}
/* Default Serialization Traits:
 * Used by all types without their own specific serialization traits.
 */
template<JsonSerializeType>
struct JsonSerializeBrace
{
 static char braces[];
};
/*
 * The MemberScanner is used to register callbacks that will read sub-members from the json object
 */
template<typename T, typename MemberToSerialize = typename JsonSerializeTraits<T>::SerializeInfo>
struct MemberScanner
{
 void operator()(ThorsAnvil::Json::ScannerSax& scanner, T& destination)
 {
 boost::mpl::for_each<typename JsonSerializeTraits<T>::SerializeInfo>(MPLForEachActivateItem<T, ThorsAnvil::Json::ScannerSax>(scanner, destination));
 }
};
template<typename T>
struct MemberScanner<T, T>
{
 // A normal type with no SerializeInfo has no members thus no need to register callbacks.
 void operator()(ThorsAnvil::Json::ScannerSax& scanner, T& destination)
 {}
};
template<typename T>
struct MemberScanner<T, void>
{
 // A normal type with no SerializeInfo has no members thus no need to register callbacks.
 void operator()(ThorsAnvil::Json::ScannerSax& scanner, T& destination)
 {}
};
/*
 * The MemberPrinter prints each member of an object.
 */
template<typename T, typename MemberToSerialize = typename JsonSerializeTraits<T>::SerializeInfo>
struct MemberPrinter
{
 void operator()(std::ostream& stream, T const& source)
 {
 typedef typename boost::mpl::at<typename JsonSerializeTraits<T>::SerializeInfo, boost::integral_constant<int,0> >::type FirstType;
 typedef typename boost::mpl::pop_front<typename JsonSerializeTraits<T>::SerializeInfo>::type AllButFirstType;
 MPLForEachActivateItem<T, std::ostream> itemPrinter(stream, source);
 // Print the first member (Call makeFirst() means no comma is printed)
 itemPrinter(FirstType().makeFirst());
 // For each of the other members use a loop a proceed each object with a comma
 boost::mpl::for_each<AllButFirstType>(itemPrinter);
 }
};
template<typename T>
struct MemberPrinter<T, T>
{
 // A normal object just prints itself.
 void operator()(std::ostream& stream, T const& source)
 {
 stream << source;
 }
};
template<typename T>
struct MemberPrinter<T, void>
{
 void operator()(std::ostream& stream, T const& source)
 {}
};
struct JsonSerializer
{
 template<typename T, JsonSerializeType base = JsonSerializeTraits<T>::type>
 struct Parser: ThorsAnvil::Json::ScannerSax
 {
 typedef boost::mpl::bool_<base != Invalid> Parsable;
 Parser(T& dst)
 : destination(dst)
 {
 MemberScanner<T> memberScanner;
 memberScanner(*this, destination);
 }
 void parse(std::istream& stream)
 {
 ScannerSax::parse<yy::ParserShiftReduce>(stream);
 }
 T& destination;
 };
 template<typename T ,JsonSerializeType base = JsonSerializeTraits<T>::type>
 struct Printer
 {
 typedef boost::mpl::bool_<base != Invalid> Printable;
 Printer(T const& src)
 : source(src)
 {}
 void print(std::ostream& stream)
 {
 stream << JsonSerializeBrace<base>::braces[0];
 MemberPrinter<T> memberPrinter;
 memberPrinter(stream, source);
 stream << JsonSerializeBrace<base>::braces[1];
 }
 T const& source;
 };
};
/*
 * Default accessors for fundamental types std::string
 * The serialize() Recursively calls jsonInternalExport() on the member to serialize the member.
 *
 * The action() Creates a JsonImportAction() that is registered with the SAX parser that just reads the
 * Item directly into the object. If the object is a compound type it uses the SerializeInfo
 * to register subsequent actions recursively so we always read directly into an object
 * not a copy.
 */
template<typename T, typename MP, typename SerTraits>
class JsonSerialElementAccessor
{
 MP memberPtr;
 public:
 JsonSerialElementAccessor(MP mp): memberPtr(mp) {}
 void serialize(T const& src, std::ostream& stream) const
 {
 stream << jsonInternalExport(src.*memberPtr);
 }
 std::auto_ptr<ThorsAnvil::Json::SaxAction> action(T& dst) const
 {
 std::auto_ptr<ThorsAnvil::Json::SaxAction> action(new_JsonImportAction<typename SerTraits::SerializeInfo>(dst, memberPtr));
 return action;
 }
};
/* Helper functions */
template<typename T>
Importer<T, typename JsonSerializer::template Parser<T> > jsonInternalImport(T& object)
{
 return Importer<T, typename JsonSerializer::template Parser<T> >(object);
}
template<typename T>
Exporter<T, typename JsonSerializer::template Printer<T> > jsonInternalExport(T const& object)
{
 return Exporter<T, typename JsonSerializer::template Printer<T> >(object);
}
 }
 }
}
#endif

• Konrad Rudolph: I strongly doubt that Loki would name something after Thor. Unless it was to ingratiate himself for some nefarious scheme.
• Loki Astari: @KonradRudolph: Note: Its 'Thor's Anvil'. The item that Thor beats his frustration out on with his hammer(Mjölnir). Thus in my weird though patterns another pseudonym for Loki. (not the Loki was Thor's only frustration and Nemesis but he was a bit of pain).
• Konrad Rudolph: One level deeper. You win.
• Zeta: Your code is no longer in-sync with your GitHub repository. Given the age of your question, I'm not sure whether you want this version to be reviewed or your new one. But this could be a great opportunity for you to review your own, almost 6 year old code.
• Loki Astari: @zeta. Yes the repository was updated based on the comments it received in the reviews in other posts.

Answer 1

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<JsonSerializeType>
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<JsonSerializeType>
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<typename T>
struct MemberPrinter<T, T>
{
 // A normal object just prints itself.
 void operator()(std::ostream& stream, T const& source)
 {
 stream << source;
 }
};
template<typename T>
struct MemberPrinter<T, void>
{
 void operator()(std::ostream& stream, T const& source)
 {}
};

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

template<typename T>
struct MemberPrinter<T, T> {
 // A normal object just prints itself.
 void operator()(std::ostream& stream, T const& source) {
 stream << source;
 }
};
template<typename T>
struct MemberPrinter<T, void> {
 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<typename JsonSerializeTraits<T>::SerializeInfo>(MPLForEachActivateItem<T, ThorsAnvil::Json::ScannerSax>(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<typename JsonSerializeTraits<T>::SerializeInfo>(
 MPLForEachActivateItem<T, ThorsAnvil::Json::ScannerSax>(
 scanner, destination
 ) // optionally combine the 2 ')'
 ); 

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

template<typename T,
 typename MemberToSerialize =
 typename JsonSerializeTraits<T>::SerializeInfo>
struct MemberScanner {
 void operator()(ThorsAnvil::Json::ScannerSax& scanner, T& destination) {
 boost::mpl::for_each<typename JsonSerializeTraits<T>::SerializeInfo>(
 MPLForEachActivateItem<T, ThorsAnvil::Json::ScannerSax>(
 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<T>::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<typename T,
 typename MemberToSerialize =
 typename JsonSerializeTraits<T>::SerializeInfo>
struct MemberScanner {
 void operator()(ThorsAnvil::Json::ScannerSax& scanner, T& destination) {
 boost::mpl::for_each<MemberToSerialize>(
 MPLForEachActivateItem<T, ThorsAnvil::Json::ScannerSax>(
 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<typename T,
 typename MemberToSerialize =
 typename JsonSerializeTraits<T>::SerializeInfo>
struct MemberScanner {
 // only pollute this scope, alternatively use typedef/alias
 using ThorsAnvil::Json::ScannerSax;
 void operator()(ScannerSax& scanner, T& destination) {
 boost::mpl::for_each<MemberToSerialize>(
 MPLForEachActivateItem<T, ScannerSax>(scanner, destination)
 );
 }
};

2 uses is really on the low side but as they are so long it might be considered.

• Loki Astari: Unfortunately I disagree with nearly every point (apart from shortening names with using). This is more of a style review than a code review. Since my personal style preferences fall into one of the three major style groups (with a couple of tweaks for readability) this is just your opinion differs from mine review. As such not much use, especially since these things are usually covered by local style guides.
• Florian F: There isn't a right style and a wrong style. I thought the page you mention, on indent styles, shows that.
• Surt: @FlorianF, which is why I mentioned it was my preferences, because of the different styles, but also why I think it was relevant.
• Deian: To be fair: @Loki is asking for feedback and Surt is giving it to him. Loki has the right of choice: to agree, to disagree or to learn from a Suri's point of view. Simple.
• Harald Scheirich: While some comment on the style in the scope of a larger review might be helpful, just giving formatting tips which are in most cases completely subjective does not really provide any kind of substantive feedback. This basically amounts to saying "you chose red but i prefer blue."