-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Conversation
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? |
@viveksacademia4git You sure did. It's a pretty obvious design flaw for a Java library. |
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. |
No objection to reopening the PR for a few days. |
There was a problem hiding this 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.
…dditional elements.
…t can hold additional elements.
@johnjaylward Duh! Thanks! Fixed in f1d354c |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good
Please add unit tests that explicitly call the new putAll() methods. |
What problem does this code solve? Risks Changes to the API? Will this require a new release? Should the documentation be updated? Does it break the unit tests? Was any code refactored in this commit? Review status |
Starting 3-day comment window. |
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:
You can now do:
It is also very useful if you need to add elements from multiple arrays/collections. For example:
Instead of:
Simply do:
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.