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

add isEmpty and isNotEmpty methods #421

Merged
merged 3 commits into from May 26, 2018
Merged

add isEmpty and isNotEmpty methods #421

merged 3 commits into from May 26, 2018

Conversation

strkkk
Copy link
Contributor

@strkkk strkkk commented May 19, 2018

What problem does this code solve?
It is clearer to check if object is empty or not by calling method rather then comparing length.
Also, it can be used as method reference, see code snippet below.

    JSONObject example = new JSONObject();
    example.length() == 0; // was 
    example.isEmpty(); // become 

    example.length() != 0; // was 
    example.isNotEmpty(); // become 

    List<JSONObject> list = ...;
    list.stream().filter(JSONObject::isEmpty).filter(JSONObject::isNotEmpty).count(); 

Risks
Low

Changes to the API?
Yes

Will this require a new release?
No

Should the documentation be updated?
Methods are added with javadocs

Does it break the unit tests?
No. This methods are new.

Was any code refactored in this commit?
No Internal uses of length() == 0 and similar code were converted to isEmpty()

Review status
APPROVED
(isNotEmpty() is not included, JSONArray.isEmpty() was added)
Starting 3 day comment window

@johnjaylward
Copy link
Contributor

No other collection type in Java has a isNotEmpty method that I know of. I'd rather just have the isEmpty check then in your example, it would look like:

JSONObject example = new JSONObject();
    example.length() == 0; // was 
    example.isEmpty(); // become 

    example.length() != 0; // was 
    example.isNotEmpty(); // become 

    List<JSONObject> list = ...;
    list.stream().filter(JSONObject::isEmpty).filter(item -> !item.isEmpty()).count();

@strkkk
Copy link
Contributor Author

strkkk commented May 20, 2018

@johnjaylward
Yep, java collections does not have it, but JSONObject != java collection, isn't it? :)
Some libs provide such method, e.g. CollectionUtils to make it easier to read.

Arguably, but Scala collections/strings/option have nonEmpty method as part of standard API as well.
Comparing both approaches with lambda, I would prefer method reference way.
list.stream().filter(JSONObject::isNotEmpty).count();

May be we need more opinions.

@stleary
Copy link
Owner

stleary commented May 21, 2018

No objection to adding isEmpty(), but please do it the right way:

  • Add the method to JSONArray as well, unless you have some reason why it should not be included.
  • Update the code in the lib where JSONObject and JSONArray empty condition is checked. I think there are about 4 occurrences.
  • Update those unit tests that check for JSONObject and JSONArray empty condition. There should be about 16 occurrences.
  • If you do the previous 2 steps it won't be necessary to add explicit unit tests.

I don't think there is a valid use case for isNotEmpty().

@strkkk
Copy link
Contributor Author

strkkk commented May 21, 2018

  • Added isEmpty() to JSONArray
  • Removed isNotEmpty() method
  • Refactored some usages of length() method for JSONArray, JSONObject and String

Will update tests later.

@stleary
Copy link
Owner

stleary commented May 22, 2018

Starting 3 day comment window

@johnjaylward
Copy link
Contributor

@stleary If we want to ensure Android compatability, then the other classes should not be updated, only JSONArray and JSONObject.

i.e. if using the CDL class in android, you'll get a no method exception for isEmpty.

@stleary
Copy link
Owner

stleary commented May 23, 2018

My bad, thanks @johnjaylward. @strkkk please revert the changes to CDL, JSONML, and XML. I will need to update the Wiki regarding Android compatibility.

@strkkk
Copy link
Contributor Author

strkkk commented May 23, 2018

Ok, I will change it.
by the way, why changes in this classes will break Android compatibility?

@strkkk
Copy link
Contributor Author

strkkk commented May 25, 2018

@stleary
Tests are updated, changes for mentioned classes are partially reverted.

@stleary stleary merged commit 7a17ae0 into stleary:master May 26, 2018
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.

None yet

3 participants