0
\$\begingroup\$

This is a follow up to this question. This is an improved version in accordance to comments, suggestions and reviews.

Is the namespace implementation proper? is the code readable? I didn't want to split it into header and source files, because i want to make this single function quick to implement. Maybe i left something out? Maybe the JSONPrettify_specifics should be an anonymous namespace?

namespace boris {

    namespace JSONPrettify_specifics{
        enum class Colons{
            TIGHT,
            SPACED
        };
        enum class Position{
            TAB = 0,
            COMMA = 1,
            OBJ_START = 2,
            OBJ_END = 3,
            ARRAY_START = 4,
            ARRAY_END = 5
        };
        struct RegexPos{
            size_t pos;
            long length;
        };

        std::string generateSpaces( int l ){
            return std::string(l*4, ' ');
        }
        long lowestOf( std::vector<size_t> of ){
            std::vector<size_t>::iterator result = std::min_element(std::begin(of), std::end(of));
            return std::distance(std::begin(of), result);
        }

        void insertColonSpaces( std::string & j ){
            std::regex colon = std::regex(R"(\s*?\:\s*?(?=\S))");
            j.assign(std::regex_replace(j, colon, " : "));
        }
        RegexPos findRegexFirstPosition( const std::string & json, const long start_pos, const std::regex rx ){
            size_t at = -1;
            long l = 0;

            std::string ss( json.begin()+start_pos, json.end() );
            std::smatch m;

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

            if ( m.size() > 0 ) {
                at = m.position(0);
                l = m[0].str().size();
            }
            if( at < json.size() ) at += start_pos;
            return {at,l};
        }
    }

    std::string JSONPrettify( const std::string & json, boris::JSONPrettify_specifics::Colons spacing=boris::JSONPrettify_specifics::Colons::TIGHT ){

        using namespace boris::JSONPrettify_specifics;

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

        long it                 = 0;
        int depth               = 0;

        while( it < pretty.size() ){

            RegexPos pos_tab        = findRegexFirstPosition( pretty,it, var );
            auto pos_comma          = pretty.find( ",", it );
            auto pos_obj_start      = pretty.find( "{", it );
            auto pos_obj_end        = pretty.find( "}", it );
            auto pos_array_start    = pretty.find( "[", it );
            auto pos_array_end      = pretty.find( "]", it );

            long old_it             = it;

            Position work_with;

            {
                std::vector<size_t> _temp = { pos_tab.pos, pos_comma, pos_obj_start, pos_obj_end,pos_array_start,pos_array_end };

                auto at = lowestOf(_temp);


                if(_temp[at] > pretty.size())   break;


                work_with = static_cast<Position>(at);
            }

            switch( work_with ){

                case(Position::TAB):{
                    std::string insert = generateSpaces(depth);

                    pretty.insert( pos_tab.pos, insert );

                    it = pos_tab.pos+insert.size()+pos_tab.length;
                    break;
                }

                case(Position::COMMA):{
                    std::string insert = "\n";

                    pretty.insert( pos_comma+1, insert );

                    it = pos_comma+1;
                    break;
                }

                case(Position::OBJ_START):{
                    std::string insert = "\n";
                    pretty.insert( pos_obj_start+1, insert );

                    it = pos_obj_start+insert.size();

                    depth+=1;

                    if(pos_obj_start-1 > pretty.size()) continue;

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

                    break;
                }

                case(Position::OBJ_END):{
                    std::string insert = "\n"+generateSpaces( depth-1 );
                    pretty.insert( pos_obj_end, insert );

                    it = pos_obj_end+insert.size()+1;

                    depth-=1;
                    break;
                }

                case(Position::ARRAY_START):{
                    std::string insert = "\n";
                    pretty.insert( pos_array_start+1,insert );
                    it=pos_array_start+insert.size();

                    depth+=1;
                    break;
                }

                case(Position::ARRAY_END):{
                    std::string insert = "\n"+generateSpaces( depth-1 );
                    pretty.insert( pos_array_end,insert );
                    it=pos_array_end+insert.size()+1;

                    depth-=1;
                    break;
                }

                default:{
                    break;
                }
            };

            if(it == old_it)
                break;
        }

        if(spacing == Colons::SPACED)
            insertColonSpaces(pretty);

        return pretty;
    }
};
\$\endgroup\$
  • \$\begingroup\$ Have you tested the code? I am getting runtime error. ideone.com/kE6Y4D \$\endgroup\$ – Jan Korous Jun 23 '16 at 12:20
  • \$\begingroup\$ My bad. I fixed it. ideone.com/3C9RjR ; The problem was that I didn't check wether all the positions are bad/impossible ( > size() ) \$\endgroup\$ – Borisas Jun 23 '16 at 12:47
1
\$\begingroup\$

Regarding your questions

I still think splitting the code between header and implementation is a superior solution but I respect your decision not to do it.

Correctness

I realized that you are probably not expecting characters as ',', ':' or '{'? in json values.

Think about this json example: { "name": "John[]"}

If you want more general json parser you might need to rethink some approaches.

Colons could be in values as well.

if(spacing == Colons::SPACED)
    insertColonSpaces(pretty);

Commas (and others) could be in values as well.

auto pos_comma          = pretty.find( ",", it );

Readability

I would name integer other than l here as it is rather similar to 1.

    std::string generateSpaces( int n ){
        return std::string(n*4, ' ');
    }

Implementation

lowestOf() & Positions

I think I finally understand your lowestOf usage. Sorry it had taken so long! There are corner cases to be addressed (what to do if you don't find next special character). I have some ideas how to simplify it - in the end you probably don't need Positions enum at all.

        // try to find next position of interest
        const RegexPos pos_tab = findRegexFirstPosition( pretty,it, var );
        const std::string::size_t other_pos = pretty.find_first_of(",{}[]", it);

        // if nothing interesting was found we are done
        if(pos_tab.pos == std::string::npos && other_pos == std::string::npos) {
            return pretty;
        }

        // just be carefull that not every position was necessarily found
        const std::string::size_t work_with_idx = std::min(pos_tab.pos, other_pos );

        if(work_with_idx == pos_tab.pos) {
            pretty.insert( pos_tab.pos, generateSpaces(depth) );

            it = pos_tab.pos+insert.size()+pos_tab.length;

        } else {

            switch( pretty.at( work_with_idx ) ){       

                case ','    : {     
                    // ...
                }
                case '['    : {
                    // ...
                }

                // ...
            };
        }

avoiding regex copying

You might avoid copying the regex here

RegexPos findRegexFirstPosition( const std::string & json, const long start_pos, const std::regex rx ){

by using a reference

RegexPos findRegexFirstPosition( const std::string & json, const long start_pos, const std::regex& rx ){

missing headers

#include <algorithm>
#include <regex>
#include <string>
#include <vector>
\$\endgroup\$
  • \$\begingroup\$ ideone.com/YM84RH Here is the changed version. I changed the regex so it would find less common names/values like "John[]". I also added some references (to the regex you offered and to the lowestOf - where i pass a vector. lowestOf basically finds the closest position to the iterator (last position it worked with), so if all the positions are impossible the find returns std::string::npos, which is unsigned long -1, so it's larger then max size of the string (in theory) and lines (81;91) were created to detect if maybe all positions are incorrect-impossible as to avoid range errors. \$\endgroup\$ – Borisas Jun 23 '16 at 21:32
  • \$\begingroup\$ Is my usage of lowestOf still incorrect (considering it manages to avoid range errors)? If I understood you correctly then the biggest problem is readability - the code doesn't explain itself So it's not good enough. Am I right to interpret it this way? \$\endgroup\$ – Borisas Jun 23 '16 at 21:35
  • \$\begingroup\$ @Borisas I am not sure your code is working correctly in case for example pretty.find( ",", it ) returns -1. If I understand it correctly lowestOf then returns 1 (index of pos_comma in params) and that is probably not ok. Is it so? \$\endgroup\$ – Jan Korous Jun 23 '16 at 21:56
  • \$\begingroup\$ But pretty.find( ",", it ) wouldn't return -1, it would return -1 in unsigned long (since .find returns in size_t and size_t is unsigned long) which is... 4294967295 (at least in this case ), and in theory a string shouldn't be that long. I would think that the max value of unsigned long should be too long for event the longest imaginable string, by any stretch of the imagination. sorry. or am I unaware of some case in which size_t would become a non unsigned type? \$\endgroup\$ – Borisas Jun 24 '16 at 0:01
  • \$\begingroup\$ I'll be more precise, since i'm late to edit it. If std::string::find fails to find a value it returns std::string::nposas documented here and std::string::nposis always unsigned and -1 so it's max value as documented here, which in theory should never be more than the length of the string. So lowestOf should never fail. Also if it failed none of my code would work and it would throw an std::out_of_range exception which isn't something I could miss in this case \$\endgroup\$ – Borisas Jun 24 '16 at 0:18

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.