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

Added JsonHelper class to convert Java/JSON #59

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

kristian
Copy link

Hey Ralf. Often I used to work with Java collections / maps and wanted to parse them to JSON. Therefore I added a JsonHelper class, performing a best-guess conversion from Java to JSON and vice-versa. Please let me know if you could imagine merging the request, if so, I would add some documentation.

In jsonValueAsObject, only int numbers are supported, as with your implementation, you can't figure out what kind of number is in the JsonValue object. value.isNumber() is not spcific enough. Similar to asString, a asNumber method would be helpfull returning a Java Number object.

@bernardosulzbach
Copy link
Contributor

See the error. This project supports Java 5.

@kristian
Copy link
Author

Java 5, oh wow. Didn't knew that was still around. I removed the <> operator, which was indeed the only change necesarry to comply to Java 5.

@ralfstx
Copy link
Owner

ralfstx commented Sep 25, 2015

Hi Kristian,

I know this kind of utilities from my own practice and I have to say that I try to eliminate them wherever possible as they're contrary to the idea of minimal-json, that is, to work with an efficient and complete Java representation of JSON data without having to make those "best guesses".

They are indeed only guesses, in particular, you can't be sure that a JSON number is a valid Java Integer (it may be floating point, scientific notification, or exceeding MIN/MAX_VALUE). You can't even safely turn it into a double, as a JSON number can also exceed the double range. The decision what type to convert a number to cannot be made without knowing the expected format. Hence a converter from Json to Java will throw exceptions for some inputs just to turn an efficient data structure into untyped Collections that use up more memory and contain less information.

A Java to Json converter can only handle some basic types, as soon as it gets to an object of any other type, it would fail–or, as in your case, replace it with null or some arbitrary value. So it's also not a general purpose tool. There are object mappers around (such as Gson) that seem more suitable for this task, but minimal-json is, by definition, not an object mapper.

Having that said, I guess that those helpers are sometimes necessary for compatibility with old systems, maybe also for cases that I'm currently missing... What is your typical use case?

@bernardosulzbach
Copy link
Contributor

I don't want this to pass (I can't see it as part of minimal-json). But just in case it does, in line 59 you could pass the array size to the ArrayList's constructor. As this is a library, performance bottlenecks have a multiplicative factor.

@kristian
Copy link
Author

Hi Ralf, Hi Bernardo,

Thanks a lot for your input. First of all, I fully support your opinions. minimal-json is neither a object mapper, nor the perfect "object mapping" library, but it is one of my favorite Java libaries when it comes to JSON parsing! It's no humongous framework, it's nice and simple, as it needs to be.

Nevertheless I see a point in adding such a functionallity to the library, but let me tackle your concerns first.

But just in case it does, in line 59 you could pass the array size to the ArrayList's constructor. As this is a library, performance bottlenecks have a multiplicative factor.

Fully agreed, I added array.size() and object.size() to the list and map constructors.

They are indeed only guesses, in particular, you can't be sure that a JSON number is a valid Java Integer (it may be floating point, scientific notification, or exceeding MIN/MAX_VALUE).

Agreed. Especially for numbers, this is bad, because Java doesn't provide a direct equivalent. My suggestion would be to add a asNumber method to JsonValue and overriding it in JsonNumber returning a Java Number object as a best guess. Java's Number features the full range of numbers (AtomicInteger, AtomicLong, BigDecimal, BigInteger, Byte, Double, Float, Integer, Long, Short) and at least BigDecimal should be compliant to the JSON number definition in any case. Another proposal would be to do the conversion in jsonValueAsObject.

Ok, let me come to the reasons, which in my opition, speak for adding such a functionallity to the library:

  1. Compatibility. There are thousands of libraries out there. You can't assume they are going to adapt, to use JsonArray instead of List in their interfaces in future. If there is no mapping functionallity, the use of your library is very limited, at least for reading JSON.
  2. Doing it right, once. I think there is no doubt, that there are certain use cases (let me come to this in a second), where it makes sense to work with native Java objects. And if I check existing forks of your library, a lot of them add conversions to Java objects already. So there is a general need. In my opinion its better to provide such a functionallity in the library itself, to simply gurantee the correctness of the conversion. A simple example for this would be the comment from Bernardo. Implementing the conversion by yourself, does lead to some problems you didn't think about in the get go. Using a functionallity provided by the library, does always increase code stability and quality.
  3. Guaranteed future. Last, but not least I wanted to mention a certain use case focussing on collections, so JsonArrays. Looking at Java 8, streams provide massive capabilities in data processing. Data mapping, iteration, filtering, etc., but streams require a native Java collection. If you focus your library on writing JSON, I agree, you don't need similar functionalities. But if it comes to reading data, interoperability is key, because this is what you want your library to be: An easy alternative for parsing JSON. If parsing JSON is very simple, but it takes double or tripple the effort to process the data (e.g. because you can't use stream or any other library), your library will have a very limited scope to be usefull for reading JSON data. As simple as that.

I'm open for your feedback and it's fully in your decision if you support my idea or not, but I really think a similar functionallity would only benefit your library, as there is nothing bad about it. I can imagine the JsonHelper class isn't the perfect option of implementing this. I could also imagine having asMap methods in JsonObject and a asList method in JsonArray. Same as for asString in JsonString.

Thanks & regards, Kristian

@bernardosulzbach
Copy link
Contributor

About your 3 reasons:

If there is no mapping functionallity (typo), the use of your library is very limited

Not it is not. But many developers are not good enough to write this properly.

[2 as a whole]

Yes. This is the single solid reason for doing it for me. Peer review and fix it in one place and let everyone use and improve this version.

[3 as a whole]

Too much hype. Vanilla Java will still be around and strong in 2020.

your library will have a very limited scope to be usefull (typo) for reading JSON data.

No it won't.

as there is nothing bad about it.

Adding stuff has a cost. I am not talking about money directly, but if you give me 10 public classes I will take some time to figure out what your library does. If you give me 50 it will take more than that, and my wage is proportional to working time.

Won't stand against you in this. Whatever Ralf decides is OK for me.

@kristian
Copy link
Author

Hello Bernardo,

Thank you for correcting my spelling mistakes. Would you like to comment on my grammar as well? ;)

Not it is not.

But hey, you do some typos as well, so touché.

Too much hype. Vanilla Java will still be around and strong in 2020.

I don’t think that's part of this discussion, but in my opinion there is nothing like “Vanilla Java”, a programming language which doesn’t evolve is a dead language. So Java evolves, so does C++, so does...

No it won't.

What?! How is it on you to judge that? I was asked for my opinion by Ralf, and this was my feedback. Feel free to accept or decline it, but judging about it, is very disrespectful in my eyes.

Adding stuff has a cost. I am not talking about money directly, but if you give me 10 public classes I will take some time to figure out what your library does. If you give me 50 it will take more than that, and my wage is proportional to working time.

I agree, therefore I suggested adding an asMap method in JsonObject and a asList method in JsonArray instead. This is part of the discussion, so feel free to give feedback.

Thanks & regards, Kristian

@bernardosulzbach
Copy link
Contributor

But hey, you do some typos as well, so touché.

Yeah, you got me.

I was asked for my opinion by Ralf, and this was my feedback. Feel free to accept or decline it, but judging about it, is very disrespectful in my eyes.

I didn't mean to disrespect. I just wanted to show my disagreement on a statement that is really strong (implies that the library is not useful for reading JSON data, which is its main - and sole? - purpose).

The asList part works for me.

@ralfstx
Copy link
Owner

ralfstx commented Sep 28, 2015

Without discussing pro and contra of such an API, here are some unsorted thoughts about a possible implementation:

  • I agree with your suggestion to represent JSON numbers as Number. My concerns re footprint remain, but with BigDecimal we could indeed represent all possible inputs.
  • The class Json is already a helper class providing static utility methods. It doesn't feel good to add another helper class next to it. If these methods became part of the minimal-json API, shouldn't they rather be added to Json?
  • Why then should they be static helper methods at all? Why not JsonArray.asList, JsonObject.asMap, etc? Then again, you'd probably expect a method JsonValue.asObject which we already have but with a different meaning (returning JsonObject, not Object). Same with javaToJson, it could be Json.value(Object), Json.value(Collection), etc.
  • How does the Json2Java part look in the light of a streaming API? Shouldn't it be a separate handler instead of reading everything into a JsonObject, and then iterating over this object to create a Map?
  • How to deal with unhandled types in the Java2Json direction? Replacing them with null doesn't seem acceptable. Soon there would be requests to accept custom handers for certain types, and then we'd start creating an object mapper, which I'd like to avoid. Actually, your utility is a poor man's object mapper already, isn't it?
  • I agree that streams are an interesting topic and we should think about if and how to support them in the future. Feel free to open a new issue for this discussion. However, I doubt that transforming Json* types into untyped Collections (<?>) is the appropriate answer.

@nbartels
Copy link
Contributor

nbartels commented Dec 9, 2015

In my implementation there is a similar helper class, but it's a bit different because I map JSON to Pojos. So the user decides (for example) if a JsonValue is a String or Boolean. And therefore the helper is a bit more sophisticated. Perhaps a basic mapper is a nice idea, but I think as soon as you use minimal-json in a special usecase you need a custom helper.

Or you have to go the json.org approach and provide getAsX methods for every JsonValue variation. But then it's not minimal 😄

@ralfstx
Copy link
Owner

ralfstx commented Mar 14, 2016

@kristian With the new streaming API to be published in 0.9.5, wouldn't it be a better option to use a custom JsonHandler to read JSON into Lists and Maps right away instead of first parsing it as JsonObject/JsonArray and then converting it to Lists/Maps? I could provide this custom handler as an example.

@kristian
Copy link
Author

@ralfstx Sure, I see that as an alternative. Especially if such a JsonHandler comes pre-built with the library. This would be an elegant way to handle things, without adding more and more helper functions in future. As a suggestion you could provide a public static inner class CollectionsHandler just like the DefaultHandler.

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