Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ordered keys + LinkedHashMap #386

Closed
wants to merge 1 commit into from
Closed

Conversation

hewerlin
Copy link

@hewerlin hewerlin commented Dec 2, 2017

Yes, you have left a comment that you did that on purpose. So you will most likely deny this request.

I just love your JSON implementation! It's so intuitive and performs well.
The only thing that really annoys me is this "random" key order due to the HashMap.
LinkedHashMap stores key in the order they are inserted but still has super fast lookup.

JSON is very often also viewed by developers, so the order of the output keys should make sense.
Also, when you read a JSON string and then write it back to JSON again, the output is not mixed up but the same (not taking whitespace and differences in \u... character encoding into account).

If you deny the pull request, that will be fine.
I prefer the version with LinkedHashMap.

@migueltt
Copy link
Contributor

migueltt commented Dec 3, 2017

[updated - a terrible mistake on my part!!!! - see code snippet below]
@poczone I would strongly suggest to add a new constructors and methods with a boolean flag (keepOrder).
If true, then JSONObject uses a LinkedHashMap. Otherwise, the default (HashMap).
For the toMap() method, the keepOrder flag should be store as an instance variable for future calls on the same JSONObject instance.

Something like:

/* Shame on me! you cannot add IF before calling this or super... so... 
 * yeah, not final to avoid changing the other constructors
 * but more elegant would be with final, and refactor the other constructors - up to you
 */
private keepOrder = false;
public JSONObject(boolean keepOrder) {
    this.keepOrder = keepOrder;
    this.map = this.keepOrder?
        new LinkedHashMap<String, Object>():
        new HashMap<String,Object>();
}
public JSONObject(int initialCapacity, boolean keepOrder) {
    this.keepOrder = keepOrder;
    this.map = this.keepOrder?
        new LinkedHashMap<String, Object>(initialCapacity):
        new HashMap<String,Object>(initialCapacity);
}
public JSONObject(Map<?,?> m, boolean keepOrder) {
    this.keepOrder = keepOrder;
    // same as existing JSONObject(Map<?,?>) but assigning this.map the correct class
} 
public Map<String,Object> toMap() {
    Map<String, Object> results = this.keepOrder?
        new LinkedHashMap<String, Object>(this.length()):
        new HashMap<String, Object>(this.length());
    // do the other stuff
}

By doing this, the current test cases will use the default implementation (no-ordering).
Of course, you will need to add more test cases when keepOrder == true.
For keepOrder==false, copy paste the current test cases but passing false.
BUT, it's up to the maintainers to approve the pull request.

@stleary and @johnjaylward I would say this pull request to be useful - YES, I do agree that as depicted in the JSON specification, ordering is irrelevant, BUT, there are a lot of cases where key order is relevant to business logic due to invalid (but unfortunately, unchangeable) implementations on how JSON data is returned by APIs.
For example, an API may return JSON as:

{
    "line1" : "this is line 1",
    "line2" : "this is line 2",
    "line3" : "this is line 3"
}

...instead of...

[
    {"line" : "this is line 1"},
    {"line" : "this is line 2"},
    {"line" : "this is line 3"}
]

In the first case, if no ordering is preserved, the application will need to parse the key name just to process each "line" properly. This is an incorrect way to specify JSON, but there are a lot (a lot) of APIs being designed like this... unfortunately =_(
In the second case, the application processes each line by iterating the array - this is the correct way to do it. =)

@johnjaylward
Copy link
Contributor

There is no way we can accept this pr as is. It's been provided by many people nearly identically over the years.

The best way to get this accepted would be to make a very thoughtful change like mentioned by @migueltt. Doing a thorough change while maintaining the current unordered logic as default would be the best way to get it accepted.

@stleary
Copy link
Owner

stleary commented Dec 3, 2017

Closed, for the reasons cited by @johnjaylward. Also added an entry to the Wiki FAQ page regarding how pull requests are evaluated:
https://github.com/stleary/JSON-java/wiki/FAQ#how-do-you-decide-which-pull-requests-to-accept

In this case, there are pull requests where @douglascrockford has responded to the question of JSONObject mapping. See for example #91 and #106.

@stleary stleary closed this Dec 3, 2017
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