Prettify JSON class

Question

It's not much, but I tried employing some of the things I learned yet never really got to use, since that kind of code isn't really needed where I work (for the most part). I tried making it as much C++11 as I could. I would like to know if I messed up somewhere, maybe something could have been done better, or maybe you got some tips to improve readability?

#include <string>
#include <regex>
#include <vector>

class JSONPretify : public std::string{

public:

    JSONPretify(std::string j){
        this->assign(j);
        pretify();
    };
    JSONPretify(std::string j, bool colon_space){
        this->assign(j);
        pretify();
        if(colon_space)
            insertColonSpaces();
    };
private:
    void pretify(){
        std::regex var = std::regex(R"((\".+?\".*?(?=\{|\[|\,|\]|\}))|(\d+?))");
        long it = 0;
        int depth = 0;
        while(it < this->size() && it != -1){
            regex_pos pos_tab = findRegexFirstPosition(it, var);
            long pos_comma = this->find(",", it);
            long pos_obj_start = this->find("{", it);
            long pos_obj_end = this->find("}", it);
            long pos_array_start = this->find("[", it);
            long pos_array_end = this->find("]", it);
            long old_it = it;

            unsigned long work_with = find_lowest(std::vector<long>{pos_tab.pos, pos_comma, pos_obj_start, pos_obj_end,pos_array_start,pos_array_end});

            switch(work_with){
                case(TAB):{
                    std::string insert = generateSpaces(depth);
                    this->insert(pos_tab.pos, insert);

                    it = pos_tab.pos+insert.size()+pos_tab.length;
                    break;
                }
                case(COMMA):{
                    std::string insert = "\n";
                    this->insert(pos_comma+1, insert);

                    it = pos_comma+1;
                    break;
                }
                case(OBJ_START):{
                    std::string insert = "\n";
                    this->insert(pos_obj_start+1, insert);
                    it = pos_obj_start+insert.size();
                    depth+=1;
                    if(pos_obj_start-1 < 0 || pos_obj_start > this->size()) continue;

                    if(this->at(pos_obj_start-1) != ':'){
                        std::string extra = generateSpaces(depth-1);
                        this->insert(pos_obj_start, extra);
                        it+=extra.size();
                    }

                    break;
                }
                case(OBJ_END):{
                    std::string insert = "\n"+generateSpaces(depth-1);
                    this->insert(pos_obj_end, insert);
                    depth-=1;
                    it = pos_obj_end+insert.size()+1;
                    break;
                }
                case(ARRAY_START):{
                    depth+=1;
                    std::string insert = "\n";
                    this->insert(pos_array_start+1,insert);
                    it=pos_array_start+insert.size();
                    break;
                }
                case(ARRAY_END):{
                    depth-=1;
                    std::string insert = "\n"+generateSpaces(depth);
                    this->insert(pos_array_end,insert);
                    it=pos_array_end+insert.size()+1;
                    break;
                }
                default:{
                    break;
                }
            };


            if(it == old_it)
                break;
        }
    };

    void insertColonSpaces(){
        long pos = 0;
        while(pos < this->size() && pos != -1){
            pos = this->find(":", pos);
            if(pos == -1 || pos >= this->size()) break;
            this->replace(pos,1, " : ");

            pos+=3;
        }
    }
    struct regex_pos{
        long pos;
        long length;
    };
    std::string generateSpaces(int l){
        std::string r="";
        for(int i = 0; i < l; i++){
            r+= "    ";
        }
        return r;
    }
    regex_pos findRegexFirstPosition(long start_pos, std::regex rx){
        long at = -1;
        long l = 0;

        std::string ss(this->begin()+start_pos, this->end());
        std::smatch m;

        std::regex_search ( ss, m, rx );

        for (unsigned i=0; i<m.size(); ++i) {
            at = m.position(i);
            l = m[i].str().size();
            break;
        }
        if(at != -1) at += start_pos;
        return {at,l};
    }
    template<typename T>
    unsigned long find_lowest(std::vector<T> outof){
        unsigned long lowest_it = 0;
        for(unsigned i = 0; i < outof.size(); i++){
            if((outof[i] < outof[lowest_it] && outof[i] != -1) || (outof[lowest_it] == -1 && outof[i] != -1)){
                lowest_it = i;
            }
        }
        if(outof[lowest_it] == -1)
            lowest_it = outof.size()+1;
        return lowest_it;
    }

    enum positions{
        TAB = 0,
        COMMA = 1,
        OBJ_START = 2,
        OBJ_END = 3,
        ARRAY_START = 4,
        ARRAY_END = 5
    };
};

Github link

Answer 1

Thanks for editing the question. I have few ideas how you might want to polish your code.

std::string base class

It caught my eye that JSONPretify is derived from std::string. Generaly speaking STL containers are not meant for such use case. Specificaly it is a good idea if you are deriving from a base class to check that it has virtual destructor. std::string does not have virtual destructor.

See e. g. https://www.securecoding.cert.org/confluence/display/cplusplus/OOP52-CPP.+Do+not+delete+a+polymorphic+object+without+a+virtual+destructor

interface

I would go even further and suggest that as you don't need to model any state, keep any data or invariant simple function might be better interface.

std::string pretify(const std::string& j, bool colon_space = false);

separation of interface and implementation

In order to be able to hide all hairy details from users of your code you might separate it into interface and implementation. The most common form is iterface only header file (e. g. prettify.hpp) and implementation source file (e. g. prettify.cpp). You then might leave all definitions and implementation details for prettify.cpp. To separate it from the rest of your code (even that you are only linking to) you might use either anonymous namespaces or internal linkage functions (surprisingly this is other meaning of keyword static).

• http://en.cppreference.com/w/cpp/language/namespace
• http://en.cppreference.com/w/cpp/language/storage_duration

find_lowest

I would try to avoid implementation of this algorithm and use std::string::find_first_of() and/or std::min_element().

If you decide to stick with it then you still might simplify it by not making it a template as there is just single call to it. You also probably don't want to copy the argument vector so reference might be more appropriate:

unsigned long find_lowest(const std::vector<long>& outof){

work_with

I would recommend using scoped enum (great C++11 extension) and distinquishing between such enum and integers.

See http://en.cppreference.com/w/cpp/language/enum [Scoped enumerations]

All it takes is new positions definition:

enum class positions{

and change to values usage:

case(positions::TAB):{

I am perplexed by this

positions work_with = find_lowest(std::vector<long>{pos_tab.pos, pos_comma, pos_obj_start, pos_obj_end,pos_array_start,pos_array_end});

because you are assigning position to work_with but checking content in switch. Is it correct?

variables

This is kind of subjective opinion but omitting some helper variables might increase readability.

            case(COMMA):{
                std::string insert = "\n";
                this->insert(pos_comma+1, insert);

                it = pos_comma+1;
                break;
            }

shortened to

            case(COMMA):{
                this->insert(pos_comma+1, "\n");

                it = pos_comma+1;
                break;
            }

For those variables that you create and don't intend to change I would definitely use const to let know the compiler about your intention and let it actually check that you don't accidentally violate that.

const std::regex var = std::regex(R"((\".+?\".*?(?=\{|\[|\,|\]|\}))|(\d+?))");

const regex_pos pos_tab = findRegexFirstPosition(it, var);

const std::string insert = generateSpaces(depth);

insertColonSpaces

Basically you are replacing one string with another.

This question might give some hints (e. g. boost::algorithm::replace_all_copy or using std::regex). https://stackoverflow.com/questions/5343190/how-do-i-replace-all-instances-of-a-string-with-another-string

generateSpaces

Unless I have overlooked something it is the same as

return std::string(l * 4, ' ');

Check std::string "fill" constructor here: http://www.cplusplus.com/reference/string/string/string/

for() { break; }

This loop

    for (unsigned i=0; i<m.size(); ++i) {
        at = m.position(i);
        l = m[i].str().size();
        break;
    }

looks more like a simple condition

    if ( m.size() > 0 ) {
        at = m.position(0);
        l = m[0].str().size();
    }

Answer 2

Inheritance

At least in theory, the inheritance you've used here is somewhat dangerous--it's possible to convert a JSONPretify to a std::string implicitly, and probably more importantly, to convert a JSONPretify * to a std::string * implicitly. If a user ends up deleting a JSONPretify via a std::string *, you get undefined behavior.

Since std::string doesn't have any virtual functions, you're not overriding any virtual functions. That means you're not gaining much by using inheritance. If you really want to use inheritance anyway, I'd at least consider using private inheritance. This prevents implicit conversion to the base class, removing (most of) the danger. A user could still convert to the base class using an explicit cast, but at least you're forcing them to do it explicitly.

Use of this->

Although some languages require that you use this-> (or equivalents such as self) to refer to member variables/functions, C++ does not--and doing so adds a great deal of visual noise that distracts and hurts readability. It should be avoided unless truly necessary (which it's not here).

Variable names

That leads directly to looking at some of the variable names, such as insert. Picking good names is often difficult, but there's not such a shortage of names that we need to have a single statement using the same name for both a function and a variable, such as: this->insert(pos_obj_start+1, insert);. I'd rename the variable to something like leader or perhaps prolog.

Consider default arguments to reduce repetition

For example, you have two nearly identical constructors:

JSONPretify(std::string j){
    this->assign(j);
    pretify();
};
JSONPretify(std::string j, bool colon_space){
    this->assign(j);
    pretify();
    if(colon_space)
        insertColonSpaces();
};

I'd at least consider collapsing these down to one with a default argument:

JSONPretify(std::string j, bool colon_space = false){
    this->assign(j);
    pretify();
    if(colon_space)
        insertColonSpaces();
};

This will normally retain the same behavior--you can invoke it as:

JSONPretify(input);
JSONPretify(input, false); // same as above
JSONPretify(input, true);

Add a little safety

Although it's probably not a big deal, I'm guessing you don't normally want to allow implicit conversions from std::string to JSONPretify, so you might as well mark the constructor that can be invoked with only one argument as explicit.

explicit JSONPretify(std::string j, bool colon_space = false) {
    this->assign(j);
    pretify();
    if(colon_space)
        insertColonSpaces();
};

Avoid boolean parameters

When looking at client code, I doubt it's immediately obvious how JSONPretify(foo, true) differs from JSONPretify(foo, false) (i.e., it's not obvious what the Boolean is controlling). I'd consider using an enum instead:

enum class colons { tight, spaced };

explicit JSONPretify(std::string j, colons spacing=colons::tight) {
    this->assign(j);
    pretify();
    if(spacing == spaced)
        insertColonSpaces();
};

This would be invoked something like:

JSONPretify(foo, colons::spaced);

or:

JSONPretify(foo, colons::tight);

...which seems quite a bit clearer, at least to me (though some might prefer a name other than tight).

Spelling

I hesitate to even mention this, but it seems like the class name should have two "t"s: JSONPrettify.

Answer 3

Nit: minor, but a function definition doesn't need a ";" after the "}"

     pretify();  
};

And not on switch statement scope either!