Skip to content

Conversation

@johnjaylward
Copy link
Contributor

@johnjaylward johnjaylward commented Mar 7, 2018

What problem does this code solve?

this.put(value);
return this;

changes to:

return this.put(value);

Risks
Low. Changes are either documentation only, or semantic to reduce stack by a single dword by reusing the return values. Changes to API methods are not expected to affect existing applications.

Changes to the API?

  • Adds new API methods:
    JSONArray put(float) throws JSONException
    JSONArray put(int, float) throws JSONException
  • Modifies behavior of existing API methods
    JSONArray put(double) : Removes validity check (because put(Object) does)
    JSONArray put(Object): Tests validity before assignment
    JSONArray put(int, Object): tests validity if in-list, add instead of put NULL after end of current list.
    JSONObject(Map): Throws NPE if key is null

Will this require a new release?
No

Should the documentation be updated?
This PR updates the documentation. If some is missed, please note it in the comments or Code Review sections.

Does it break the unit tests?
No. New unit tests were added to reinforce the API

Was any code refactored in this commit?
Yes. As mentioned above some boxing calls were updated to the newer java5+ <Number>.valueOf instead of using the constructors as recommended by java code practices. Also, some returns were optimized which should not affect execution beyond decreasing stack size by a single dword.

Review status
ACCEPTED

@johnjaylward johnjaylward force-pushed the FixNPE branch 3 times, most recently from 942fad5 to 09e48b9 Compare March 7, 2018 17:33
Also optimizes some boxing statements and returns.
@stleary
Copy link
Owner

stleary commented Mar 8, 2018

Looks good, starting 3 day comment window.

@data-enabler
Copy link

data-enabler commented Apr 29, 2022

JSONObject(Map): Throws NPE if key is null

A bit late here, but I would consider throwing an exception for an input that used to be accepted a breaking API change. In my company's codebase we had some tests that verified this behavior that were broken as a result, and a currently unknown amount of code that could actually be depending on this behavior.

Obviously this ship has already sailed (and rejecting null keys makes much more sense), but can we at least point out the breaking change in the release history documentation? Would be very useful as a heads-up for anyone trying to upgrade from an old version.

@stleary
Copy link
Owner

stleary commented Apr 30, 2022

Moving this comment to new issues

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.

3 participants