ralfstx / minimal-json

A fast and small JSON parser and writer for Java
MIT License
732 stars 186 forks source link

Use of Optionals in Java 8 #94

Open CLOVIS-AI opened 6 years ago

CLOVIS-AI commented 6 years ago

Since Java 8, Optionals were added to replace null when a method can not return a result, as seen in JsonObject with the methods get, getInt ...

Although the current use of default values is convenient, I think it would be good to allow the use of Optionals, as they allow more control.

I am ready to make the changes myself and submit a pull request, but I'm not sure if it's better, regarding this project's policy, to do it:

  1. replace the current methods' return type by Optionals (can cause problems with dependancies)
  2. add new methods (like getOptional, getIntOptional), which can make the code a bit "messy" (and maybe deprecate the old ones)

Do you think it's worth doing it, and if it is, which would be the best way (I mean, the most user-friendly way) of implementing it?

bernardosulzbach commented 6 years ago

I think this is a welcome change. First of all, and quite evidently, changing existing signatures will cause compatibility problems. So it should be done through new methods.

However, I don't know if this is justified. The problem that having a default value returned does not tell you whether there was a value or not is solved by simply checking for member existence. Additionally, I'd rather read a check for presence with has... rather than a test to see if an optional is empty or not.

CLOVIS-AI commented 6 years ago

What I meant, is the others methods than plain simple Optional#get, like:

All of which are very convenient. Optional official documentation

sroughley commented 6 years ago

It would be great to have these in the API, but retain the original methods too, which has advantages of not breaking existing API and also avoiding object creation when a simple null check will suffice.

CLOVIS-AI commented 6 years ago

That would mean adding methods like getOptional, getOptionalInt etc, right?

sroughley commented 6 years ago

Yes, that was how I understood the suggestion - I would assume in each case the actual method would be something very simple like

Optional<JsonValue> get(String name) {
    return Optional.ofNullable(get(name));
}
CLOVIS-AI commented 6 years ago

Mostly, with some differences for primitives because they cannot be null.

sroughley commented 6 years ago

Interesting point. Most of those methods have a default value as a parameter, so would you just put that into e.g. an OptionalInt instead, in which case you would have something like:

OptionalInt getOptionalInt(String name, int default){
    return OptionalInt.of(getInt(name, default));
}

(which looks a bit pointless?!), or would you provide a new method something like:

OptionalInt getOptionalInt(String name){
    JsonValue value = get(name);
    return value != null ? OptionalInt.of(value.asInt()) : OptionalInt.empty();
}

(which looks more useful!) Or, I suppose you could do both..?

CLOVIS-AI commented 6 years ago

I was thinking about the second version.

CLOVIS-AI commented 6 years ago

Also, because the methods getInt always have 2 parameters (name & default value), we could simply do something like:

int getInt(String name, int default); OptionalInt getInt(String name);

This way, both ways can cohabit nicely in an intuitive manner?

CLOVIS-AI commented 6 years ago

Well, based on this discussion, I'll open a pull request this week with it

CLOVIS-AI commented 6 years ago

I am currently working on it, and I stumbled upon a problem: something changed between Java 7 and Java 8 in the language specifications related to type inference.

For example, if I write:

Optional<Float> getFloat(String name){
  return ... : Optional.empty()
}

the compiler recognizes Optional.empty() as returning Optional<? extends Object>, but since Java 8 it should use type inference to guess that the return type is Optional<Float>. Related question on StackOverflow

I have tried this with the exact same code in a project with only the source code and not the project configuration and it works fine, so I'm sure it comes from the project configuration. NetBeans has hinted that it's related to -source 1.5 that should be -source 1.8, but that is still pretty cryptic to me. Since it's related to project configuration, I thought it would be better to ask the code owners what they think on this issue.

sroughley commented 6 years ago

I think you should be able to use

return ... : Optional.<Float>empty();

based on pg 138 of Joshua Bloch's 'Efficient Java'

CLOVIS-AI commented 6 years ago

It works, thanks!