Refactor duplicate code for stringToNumber() in JSONObject, JSONArray, and XML #814
Conversation
…ty static method.
| import java.math.BigDecimal; | ||
| import java.math.BigInteger; | ||
|
|
||
| public class NumberConversionUtil { |
There was a problem hiding this comment.
Let's not make this public. We may wish to refactor this class in the future. For now make it private or default (package) visible.
There was a problem hiding this comment.
Static methods meant to be used by functional classes being marked private do not add up. But yes, @stleary - unnecessary making these classes open for used by clients might not be a great idea, there are plenty of scope of improvements in there. So marked them default visibility.
There was a problem hiding this comment.
That is correct. I simply meant that the class itself should be default or private, not the static methods.
There was a problem hiding this comment.
However, I now recall that private outer classes is not a thing in Java. I was thinking inner class scopes. Default visibility is correct here.
There was a problem hiding this comment.
@stleary @johnjaylward - can this PR be accepted please?
There was a problem hiding this comment.
@rudrajyotib Will get to it as soon as I can after the work of #811 is completed.
|
For issue #813 |
|
@stleary - can you accept this PR please? |
stleary
added
Pending response from submitter
and removed
Pending response from submitter
labels
|
@rudrajyotib Please update the PR: From https://github.com/rudrajyotib/JSON-java
|
|
@stleary - PR is now aligned with HEAD. |
|
@stleary - can you please see if this can be accepted, or is it still showing some discrepancy across git branches. |
stleary
added
Approved - 3-day window
hacktoberfest-accepted
and removed
In review
Hacktoberfest
labels
|
What problem does this code solve? Does the code still compile with Java6? 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 Start 3-day comment window |
| @@ -28,6 +28,9 @@ | |||
| import java.util.Set; | |||
| import java.util.regex.Pattern; | |||
|
|
|||
| import static org.json.NumberConversionUtil.potentialNumber; | |||
There was a problem hiding this comment.
Just import NumberConversionUtil, not the methods. Someone glancing at the code might not realize it is a static method in another class.
| || val.indexOf('E') > -1 || "-0".equals(val); | ||
| } | ||
|
|
||
| private static boolean potentialPositiveNumberStartingAtIndex(String value,int index){ |
There was a problem hiding this comment.
There is a duplicate of this method in JSONObject. It should be removed, and the calling code updated to use the NumberConversionUtil method.
There was a problem hiding this comment.
Right, the duplicate method will be removed in the next PR.
| @@ -10,6 +10,9 @@ | |||
| import java.math.BigInteger; | |||
| import java.util.Iterator; | |||
|
|
|||
| import static org.json.NumberConversionUtil.potentialNumber; | |||
There was a problem hiding this comment.
Just import NumberConversionUtil, not the methods.
There was a problem hiding this comment.
In the next PR it would be changed.
|
@rudrajyotib Looks good to me. I left some comments in the code. They are all trivial issues that can wait until this commit is merged. Please do not make any more changes to this PR; if you do, the accept status will be taken down and it won't get committed before the end of Hacktoberfest. I will open an issue to address the notes after Nov 1, but they are low priority. |
stleary
changed the title
stleary
changed the title
|
@stleary sure thing. I will update them in another PR. |
Both XML and JSON parser do convert string to number. Similar code has been duplicated for both parsers, which has been moved to a single utility method.
Fixes #813