11
\$\begingroup\$

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

\$\endgroup\$
  • 3
    \$\begingroup\$ @Borisas Do you really need to inherit from 'std::string' ?? OR you just did it to reduce some typing ? Either case, its not a good practice. \$\endgroup\$ – Arunmu Jun 22 '16 at 21:18
  • 3
    \$\begingroup\$ I do see lots of unnecessary copies..in constructor, you can just move the copied string...can you use std::min_element instead of calling find_lowest, which also copies the vector, instead of taking by rvalue reference or const reference.. \$\endgroup\$ – Arunmu Jun 22 '16 at 21:22
  • 2
    \$\begingroup\$ generate_spaces function can be just return std::string(' ', count); \$\endgroup\$ – Arunmu Jun 22 '16 at 21:25
  • 1
    \$\begingroup\$ std::regex var = std::regex(R"((\".+?\".*?(?=\{|\[|\,|\]|\}))|(\d+?))"); XKCD said it well Regular Expressions Which just leads to 100 problems \$\endgroup\$ – Martin York Jun 22 '16 at 22:23
  • 1
    \$\begingroup\$ @LokiAstari I understand that it's not a good way of solving all problems, but in this case it seemed like the most reasonable way to find specific occurrences without writing a ton of code that would literally do the same thing. \$\endgroup\$ – Borisas Jun 23 '16 at 7:51
15
\$\begingroup\$

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).

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();
    }
\$\endgroup\$
  • \$\begingroup\$ Thanks for your answer! I'm currently rewriting and improving the code. Transferring it from a class into a namespace function. But I noticed that this either reduces readability tremendously or the namespace has a bunch of functions that are only used for the JSONPrettify part. That's why I opted to use a class with private and public members, so the user would only see the relevant part - JsonPretify and I inherited std::string so there wouldn't be multiple function calls to "prettify" the json string. So the question - Any tips to put it in a namespace, but also make it readable? \$\endgroup\$ – Borisas Jun 23 '16 at 7:47
  • \$\begingroup\$ 2nd comment, because i ran out of characters. Should i be a namespace within a namespace? (Have the JSONPrettify in a namespace and create a namespace within that namespace that would have the functions that are needed for JSONPrettify function. boy that sounds complicated ) \$\endgroup\$ – Borisas Jun 23 '16 at 7:48
  • \$\begingroup\$ Also - can i add edited (improved?) versions here somehow? \$\endgroup\$ – Borisas Jun 23 '16 at 8:38
  • 1
    \$\begingroup\$ @Borisas You are very welcome. 1. I added new part to the answer "separation of interface and implementation" 2. It would render all answers and discussion chaotic. Creating new question would seem better to me. meta.codereview.stackexchange.com/questions/6800/… meta.codereview.stackexchange.com/questions/1065/… \$\endgroup\$ – Jan Korous Jun 23 '16 at 10:17
  • \$\begingroup\$ So if I understood correctly - you suggest separating it into a header and source file, instead of just single header file. Which is the way i would usually do, but I wanted to make it as easy to implement as possible (which was also the reason for inheriting std::string) and you have to compile .cpp files which is more work than just writing a #include (if i understand things correctly). I added an edited version. I put it in a namespace, got rid of the class, added a sub-namespace that has everything that JSONPrettify needs. This seemed to be in line with your suggestions. \$\endgroup\$ – Borisas Jun 23 '16 at 10:59
3
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Thanks i'm trying to improve the code in accordance to your review/comments/tips! \$\endgroup\$ – Borisas Jun 23 '16 at 7:49
3
\$\begingroup\$

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

     pretify();  
};

And not on switch statement scope either!

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.