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

Numeric enhancements, Refactoring, Fix spelling #336

Merged
merged 3 commits into from
May 20, 2017

Conversation

johnjaylward
Copy link
Contributor

This is mostly cleanup of comments with a few changes to also remove some compile time warnings.

  • JSONObject
    • increment I replaced the autoboxing calls with explicit accessors and also prevented runtime casting by using the proper long/float/double literals.
    • Null I added a hashcode function that explicitly returns 0 everytime, the same as most hash functions when handling null : return someField == null ? 0 : someField.hashCode();
    • Added a put function for float... for some reason we have code that handles floats in multiple places, but no way to directly add them into the JSONObject. Previously these would get converted to double
    • Modified the put(literalNumber) methods to use the valueOf static instead of always creating new instances.
  • JSONPointer
    • Updated class references to be fully qualified with this.
    • Updated constructor parameters to be final.
  • XML
    • toString(String, String) updated parameters to be final

Let me know if I should roll any of the changes back and just keep the comment fixes.

@stleary
Copy link
Owner

stleary commented May 16, 2017

Looks good, but seems to be breaking one of the unit tests:
org.json.junit.JSONObjectTest > jsonObjectIncrement FAILED
java.lang.AssertionError at JSONObjectTest.java:1319
Can you confirm?

@johnjaylward
Copy link
Contributor Author

Yup, I'll take a look.

@johnjaylward
Copy link
Contributor Author

OK, looks like the test is wrong due to that lack of having a float put method I mentioned. I'll update the test and push the change later today.

@stleary
Copy link
Owner

stleary commented May 17, 2017

What problem does this code solve?
This is an enhancement, not a bug fix. Some code is refactored and spelling errors are corrected. Some improvements made to numeric handling.

Risks
Very low

Changes to the API?
No, other than support for JSONObject put(float)

Will this require a new release?
No, can be rolled into the next release

Should the documentation be updated?
No

Does it break the unit tests?
Broke one test due to improved increment handling. The test code fix stleary/JSON-Java-unit-test#70 will be committed in sync with this one.

Was any code refactored in this commit?
Yes

Review status
ACCEPTED, Starting 3 day timer for comments.

@stleary stleary changed the title Fix spelling Numeric enhancements, Refactoring, Fix spelling May 17, 2017
@stleary stleary merged commit f2b642a into stleary:master May 20, 2017
stleary added a commit to stleary/JSON-Java-unit-test that referenced this pull request May 20, 2017
test support for Numeric enhancements, Refactoring, Fix spelling:  stleary/JSON-java/pull/336
@johnjaylward johnjaylward deleted the fixSpelling branch July 7, 2017 16:35
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.

2 participants