Skip to content

Conversation

Orbiter
Copy link

@Orbiter Orbiter commented Feb 2, 2016

debugging of data from a json api is much easier if the order of the keys is stable.

I added an option to store the JSONObject's properties in a LinkedHashMap to produce a self-defined order of entities when a JSONObject is serialized with toString()

@johnjaylward
Copy link
Contributor

I think a better option may be to modify the toString option to use a specified order. Then you can just call the toString(true) method from logger calls and not mess with the internal representation.

Historically PRs like this have been denied because JSON specifically defines that the keys are not ordered.

@erosb
Copy link
Contributor

erosb commented Feb 2, 2016

JSON specifically defines that the keys are not ordered.

This is true. Also, the access time of a key in a LinkedHashMap is O(N) while it is approximately O(1) for a HashMap, therefore switching to LinkedHashMap may cause performance problems for larger objects.

@Orbiter
Copy link
Author

Orbiter commented Feb 2, 2016

you are both right, json defines that there is no specific order and a LinkedHashMap is less performant. However, the LinkedHashMap is just an option here (not a full replacement!) and having an explicit order is not a contradiction to the requirement, that the order has no logical relevance.

Let me give you an example: I am migrating from com.fasterxml.jackson.databind.ObjectMapper to JSON-java. While I can hand over jackson a LinkedHashSet to pretty-print this in json, I don't have this option with JSON-java.
Here is such a jackson code: https://github.com/loklak/loklak_server/blob/master/src/org/loklak/api/server/GeocodeServlet.java#L111-L118
Here is such a case where the code is converted to JSON-java using the patch: https://github.com/loklak/loklak_server/blob/master/src/org/loklak/api/server/AppsServlet.java#L59-L82

The patch gives the user a convenience to create prettier json strings than just pretty-printed json because it's possible to show humans the same output as given i.e. in documentations.

@johnjaylward
Copy link
Contributor

We aren't arguing about it's prettiness. We are commenting that historically requests like this have been denied for the fact that developers using JSON should not rely on an ordering of the keys. I offered a second option to implement something similar (toString, or maybe call it something like toStringOrdered()).

Personally, I can't imagine ever instantiating a JSONObject and passing it a DEBUG value like your pull request would require. I'd rather see something like this in my code:

JSONObject jo = new JSONObject(someMap);
... do stuff
if(error) {
    LOG.error(jo.toStringOrdered());
}

instead of

JSONObject jo = new JSONObject(inDebugMode);
// copy my map into my json object
... do stuff
if(error) {
    LOG.error(jo.toString());
}

@stleary
Copy link
Owner

stleary commented Feb 3, 2016

Thanks for making the pull request, but it can't be merged for the reasons already stated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants