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 putAll(Collection) and putAll(Array) methods. #529

Merged
merged 4 commits into from Jul 25, 2020

Conversation

ethauvin
Copy link
Contributor

@ethauvin ethauvin commented Jun 16, 2020

I've added two methods to add/put all of the elements of an array/collection to the JSONArray.

Mostly to avoid inefficient code like:

JSONArray jsonArray;
...
if (myArray != null) {
    jsonArray = new JSONArray(myArray);
} else {
    jsonArray = new JSONArray();
}

You can now do:

JSONArray jsonArray = new JSONArray();
...
if (myArray != null && !myArray.isEmpty()) {
    jsonArray.putAll(myArray);
}

It is also very useful if you need to add elements from multiple arrays/collections. For example:

int[] myInts = { 1, 2, 3, 4, 5 };
List<String> myList = Arrays.asList("one", "two", "three", "four", "five");

Instead of:

JSONArray jsonArray = new JSONArray(myInts);

for (String s : myList) {
    jsonArray.put(s);
}

Simply do:

JSONArray jsonArray = new JSONArray(myInts);
jsonArray.putAll(myList);

There's no real code change, just things moved around. I did not implement tests since the constructors are already tested.

Let me know if you have any questions.

@stleary
Copy link
Owner

stleary commented Jun 17, 2020

Thanks for the pull request, but it can't be accepted at the present time. Please see How do you decide which pull requests to accept?

@stleary stleary closed this Jun 17, 2020
@viveksacademia4git
Copy link
Contributor

@ethauvin I wanted to add something similar in issue #528; proposed by you in this issue.

@ethauvin
Copy link
Contributor Author

@viveksacademia4git You sure did. It's a pretty obvious design flaw for a Java library.

@stleary
Copy link
Owner

stleary commented Jul 21, 2020

At the end of the day, this is an uncomplicated reference library with the limited goal of demonstrating how to parse JSON in Java. Being relatively fast and small helps too, I suppose. Currently, this project is in maintenance mode. The main criteria for accepting PRs are to fix bugs and address major user inconveniences (please see the FAQ section). With regard to #529 and #528, neither PR made a compelling case for inclusion, nor did others raise comments to support the need for this change.

@ethauvin
Copy link
Contributor Author

@stleary I completely understand the current motivations. Although as far as #529 is concerned, since it was closed within hours of submission, I don't see how anyone would support it. It would have been interesting to see what others had to say.

@stleary
Copy link
Owner

stleary commented Jul 21, 2020

No objection to reopening the PR for a few days.

@stleary stleary reopened this Jul 21, 2020
Copy link
Contributor

@johnjaylward johnjaylward left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have any issues with the implementation. The refactoring is clean and I think it adds a fairly standard interface among collections that JSONArray has been missing.

The only correction I would make is in the addAll methods, the calls to this.myArrayList.ensureCapacity should include the current length as well as the length of the array or collection that is being added.

@ethauvin
Copy link
Contributor Author

@johnjaylward Duh! Thanks!

Fixed in f1d354c

@ethauvin ethauvin closed this Jul 22, 2020
@ethauvin ethauvin reopened this Jul 22, 2020
Copy link
Contributor

@johnjaylward johnjaylward left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@stleary
Copy link
Owner

stleary commented Jul 22, 2020

Please add unit tests that explicitly call the new putAll() methods.
Please add an example in the description for, "It is also very useful if you need to add elements from multiple arrays/collections."

@ethauvin
Copy link
Contributor Author

@stleary See 0d13e56

@stleary
Copy link
Owner

stleary commented Jul 23, 2020

What problem does this code solve?
Added new methods to JSONArray to make it more convenient to add arrays and collections all at once,
instead of a single element at a time.
This is not a bug fix.
Unit tests were added to exercise the new API methods.

Risks
Low

Changes to the API?
New putAll() methods were added to JSONArray
New private addAll() methods were added to implement the functionality.

Will this require a new release?
No

Should the documentation be updated?
No

Does it break the unit tests?
No

Was any code refactored in this commit?
The JSONArray array and collection constructors were updated to use new private methods addAll()

Review status
APPROVED

@stleary
Copy link
Owner

stleary commented Jul 23, 2020

Starting 3-day comment window.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants