ArduinoJson icon indicating copy to clipboard operation
ArduinoJson copied to clipboard

Add empty() method

Open FStefanni opened this issue 2 years ago • 5 comments

Is your feature request related to a problem? Please describe.
Hi,

the only way to test if a object is actually empty is to use the size() method, which is actually inefficient, according to the official documentation (it walks a linked list). At the same time, isNull() can return false even if the object is empty (so basically it seems to test something else).

Example code:

DynamicJsonObject doc(256);
auto obj = doc.createNestedObject("test");
if (obj.size() == 0)
{
    // ...
}

In the example code, calling isNull() returns false, so adding an efficient empty() (or isEmpty() if you prefer) would be of great help for performance, in case the object is not actually empty.

Regards

Describe the solution you'd like

Add a support method empty().

Describe alternatives you've considered

As previously stated:

  • Calling size(), as I currently do, but it is inefficient
  • Calling isNull(), but it does not work.

Additional context

FStefanni avatar Jul 14 '22 13:07 FStefanni

Hi @FStefanni,

What I dislike with this feature is that it's ambiguous whether empty() should return true or false when the reference is null or unbound.

As a workaround, I suggest that you compare begin() with end(), like so:

bool isEmpty(JsonObjectConst obj) {
  return !obj.isNull() && obj.begin() == obj.end();
}

// or 

bool isNullOrEmpty(JsonObjectConst obj) {
  return obj.begin() == obj.end();
}

Best regards, Benoit

bblanchon avatar Jul 19 '22 08:07 bblanchon

Hi,

so I will use the isEmpty() snippet you provided thanks.

Nevertheless, I recommend to add an isEmpty() method, since all the containers usually provide this method, and, actually, it is provided since checking for the number of elements can be expensive. For example, think about std::string:

  • originally, the C++ standard did not mandate a size field, so asking for the size was inefficient (equivalent to call strlen())
  • so the empty() method was implemented in an optimized way (by checking the internal pointer for null)
    • so far, it reminds me the situation of ArduinoJson, which unfortunately misses the isEmpty()-optimized method
  • now std::string mandates to store its size, to optimize performance at cost of little more memory usage
    • Other idea: optimize the size() method of ArduinoJson for speed, by tracking internally the number of elements

In the end: it is not a big deal to use the snippet you provided, but it is not obvious that this should be the correct way (a ArduinoJson user must find that the size() method is inefficient and it must figure out that it can use iterators...). So providing a isEmpty() method would increase this library usability.

Regards

FStefanni avatar Jul 19 '22 09:07 FStefanni

I do see the use for this method; I'm just saying it's too ambiguous. If I add this method on JsonArray and JsonObject, I must also add it on JsonDocument and JsonVariant.

What does it mean for a JsonVariant to be empty? Should it return true when the reference is unbound or when it points to null? Should it return true for an empty string? Should it return true for NaN?

The problem is even worse for JsonDocument: what does it mean for a document to be empty? It cannot mean that it contains an empty array, object, or string. On the contrary, it means that it contains nothing which is incompatible with the interface you want for JsonObject.

bblanchon avatar Jul 21 '22 06:07 bblanchon

Hi,

I do not know exactly the relationships between the classes you list nor their internals, but imho and for what I understand, I suggest:

  • JsonArray and JsonObject, seems to me the trivial case:
[ ] // empty array
{ } // empty object
  • JsonVariant: it seems a wrapper to contain anything... so:
    • [container] Contains an object? returns object.isEmpty
    • [container] Contains an array? returns array.isEmpty
    • [literal] Contains something? E.g. a number or a string? return false
    • Contains unbound reference? (if I understand what it means) true
    • [literal] Contains the null or undefined of javascript? (if this case is possible) return false
  • JsonDocument: another wrapper, similar to JsonVariant, but with these differences:
    • [container] Contains an object? returns false
    • [container] Contains an array? returns false
    • Other cases as JsonVariant

So that the question is: is this thing empty from a json point of view? and not asking about the possible stored value. The logic is:

  • [container]s are the obvious case (objects, arrays)
  • [literals]s are values, so they are never empty from a Json point of view
  • Value wrappers (variants) are empty if they are not wrapping anything, otherwise return the wrapped value isEmpty
  • The json root (document) is empty if the document is empty, since it is a container

Does this seems fine to you?

Regards

FStefanni avatar Jul 21 '22 10:07 FStefanni

Hi @FStefanni,

You're suggesting that JsonVariant and JsonObject should behave differently, which contradicts the design of these classes. Look at their interfaces, and you'll see that they are perfectly consistent. The only advantage of JsonObject is that it allows iterating over key-value pairs, whereas JsonVariant doesn't.

Even if I accepted to add this inconsistency (which I won't), you must admit that it's very confusing for the user. No one should have to read the documentation to understand what a function as simple as isEmpty() does, right?

Best regards, Benoit

bblanchon avatar Jul 25 '22 08:07 bblanchon