MJPA/SimpleJSON

Code is very weak

Elmue opened this issue · 6 comments

Elmue commented

There are several issues with your code.
It works but the slightest error in using your library will lead to ugly crashes.

For example:

const JSONArray &JSONValue::AsArray() const
{
    return (*array_value);
}

This is a very dangerous code.
What if the content of the JSONValue is not an array ?
Then you have a crash.

I recommend as the easiest workaround at least to put an assert like this:

JSONArray* JSONValue::AsArray()
{
    if (type == JSONType_Array)
        return array_value;

   assert(false);
   return NULL;
}

By the way:
Your 2 "const" do not make much sense. What if someone wants to modify the JSON data?

A significantly better solution would be to remove the union and replace it with a member variable for each type:

   private:
        JSONType type;

        bool      bool_value;
        double    number_value;
        wstring   string_value;
        JSONArray array_value;
        JSONHash  hash_value;

This would SLIGHTLY use more memory but would avoid any crashes alltogether. Today's computers have at least 4 GB memory.

Is saving a few bytes of memory by using a union really worth the risk of crashes if the server sends unexpected data and the user of your library did not expect that by checking the type each and every time with IsArray() ?

After removing the array and object from the union and converting the pointers into members you can write:

wstring* JSONValue::AsString()
{
    assert (type == JSONType_String) 
    return &string_value;
}

JSONArray* JSONValue::AsArray()
{
    assert(type == JSONType_Array)
    return &array_value;
}

JSONHash* JSONValue::AsHash()
{
    assert(type == JSONType_Hash)
    return &hash_value;
}

and there will never ever be a crash because these new functions will neither ever return NULL nor will they return the wrong object.

If the user calls AsArray() although the JSONValue is for example a string he will get an empty array instead of a crash.

And the construction / destruction will look like this:

JSONValue::JSONValue(JSONArray& m_array_value)
{
    Clear();
    type = JSONType_Array;
    array_value = m_array_value;
}
etc...

JSONValue::~JSONValue()
{
    Clear();
}
void JSONValue::Clear()
{
    type         = JSONType_Null;
    bool_value   = false;
    long_value   = 0;
    double_value = 0;
    string_value = L"";
    FREE_ARRAY(array_value);
    FREE_HASH (hash_value);
}

In my case, many crashes upon calling to
const JSONObject &JSONValue::AsObject() const

return NULL;
in your workaround, won't work.

Elmue commented

You did not understand my code.
You will not use AsObject() anymore.
It is replaced with AsString(), AsArray() and AsHash()

Please give examples of what should be used now, instead of AsObject()

MJPA commented

This was a design decision, there's methods to check if it's an array or object or whatever that should be used before getting the value out.

Elmue commented

Yes, and it was not a good design decision.