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

Unbounded wildcards in Collections and Maps #112

Closed
wants to merge 3 commits into from

Conversation

mdobrovnik
Copy link

Generics: use unbounded wildcards instead of Object as generic type.

There is a JSONArray constructor with (Collection <Object> c) and another constructor with signature
(Object o) . A call with new JSONArray(new HashSet<Something>()) will not resolve to the
first constructor, but to the second one. This broke our code when we tried to upgrade from
an older JSON-java version.

There is a similar issue which concerns Maps. Maps with <?,?> instead of <String, Object> type parameters should be used. So maximum flexibility is gained for the callers.

For details, please refer to example 4.5.1.1. in the java language reference
http://docs.oracle.com/javase/specs/jls/se7/html/jls-4.html#jls-4.5.1

Very much appreciate your efforts.

P.S.: just saw the very similar pull request (#111 from @domusofsail).
While i do believe that the changes in my request are a little bit more
complete than the ones incorporated in #111, the remarkable timely coincidence of the two
requests signifies that the principal issue is legitimate.

@domusofsail
Copy link

Glad to see the same pull request as mine. I think your changes are better than mine. I see the main difference is related to Map, <?,?> instead of <String,?>: I thought that JSON keys had to be strings, but if you accept any object you can always convert it to string by toString() method as you generate JSON key, much more flexible.

If you agree I'll close my pull request and keep yours.

@klsmith
Copy link

klsmith commented Jan 6, 2015

@domusofsail
I believe toString is meant for human eyes only and is not supposed to be used for anything except visual output. And yes, believe JSON objects are always required to have strings for the key, so allowing any object to be input would be breaking the definition of JSON (feel free to correct anything and everything I will doible check this when I have time).

@mdobrovnik
Copy link
Author

The perception that keys have to be strings in JSON surely is correct.
The opinion that toString() is just for human eyes is somewhat debateable.
My reasons of using Map<?,?> instead of Map<String,?> were the following:

  • Provide a maximum level of flexibility for the API user.
  • Keep it simple. The values in a Map<String,?> could themselves contain (among others) Maps (which recursively would again need to be Map<String,?>)
    I am not aware of any simple and convenient way to express that restriction.
  • Uniformity. E.g. since collections of arbitrary objects are allowed to create a JSONArray, arbitrary maps should be handled in an appropriate way.

Maybe we could settle for something like the following comment/javadoc in the methods accepting a Map<?,?>:

The keys of the Map parameter are usually strings, if not, then the keys toString() implementation will be used to derive a proper JSON key.

@domusofsail
Copy link

I believe there are 2 points to consider:

  • toString() is just for human eyes: if true we'll have to accept <String,?>, but my opinion is that this is mostly a philosophical issue
  • will Map<String,?> break compatibility with older code which used Map only constructor? Yes, I checked the code and before generics a Map<Object,Object> was accepted and the key was already generated by toString() in the JSONObject.write() method (line writer.write(quote(key.toString()));)

So my vote is for @mdobrovnik solution to accept Map<?,?> and put a comment into javadoc regarding toString().

Clarifying comments. Additional Collection<?> occurence.
@mdobrovnik
Copy link
Author

Added the comments.

@douglascrockford
Copy link
Contributor

Please see the README.

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