Refactor duplicate code for stringToNumber() in JSONObject, JSONArray, and XML #814
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,142 @@ | ||
| package org.json; | ||
|
|
||
| import java.math.BigDecimal; | ||
| import java.math.BigInteger; | ||
|
|
||
| public class NumberConversionUtil { | ||
|
Contributor
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.
Contributor
Author
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.
Contributor
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.
Contributor
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.
Contributor
Author
There was a problem hiding this comment. @stleary @johnjaylward - can this PR be accepted please?
Owner
There was a problem hiding this comment. @rudrajyotib Will get to it as soon as I can after the work of #811 is completed. |
||
|
|
||
| /** | ||
| * Converts a string to a number using the narrowest possible type. Possible | ||
| * returns for this function are BigDecimal, Double, BigInteger, Long, and Integer. | ||
| * When a Double is returned, it should always be a valid Double and not NaN or +-infinity. | ||
| * | ||
| * @param input value to convert | ||
| * @return Number representation of the value. | ||
| * @throws NumberFormatException thrown if the value is not a valid number. A public | ||
| * caller should catch this and wrap it in a {@link JSONException} if applicable. | ||
| */ | ||
| public static Number stringToNumber(final String input) throws NumberFormatException { | ||
| String val = input; | ||
| if (val.startsWith(".")){ | ||
| val = "0"+val; | ||
| } | ||
| if (val.startsWith("-.")){ | ||
| val = "-0."+val.substring(2); | ||
| } | ||
| char initial = val.charAt(0); | ||
| if ((initial >= '0' && initial <= '9') || initial == '-' ) { | ||
| // decimal representation | ||
| if (isDecimalNotation(val)) { | ||
| // Use a BigDecimal all the time so we keep the original | ||
| // representation. BigDecimal doesn't support -0.0, ensure we | ||
| // keep that by forcing a decimal. | ||
| try { | ||
| BigDecimal bd = new BigDecimal(val); | ||
| if(initial == '-' && BigDecimal.ZERO.compareTo(bd)==0) { | ||
| return Double.valueOf(-0.0); | ||
| } | ||
| return bd; | ||
| } catch (NumberFormatException retryAsDouble) { | ||
| // this is to support "Hex Floats" like this: 0x1.0P-1074 | ||
| try { | ||
| Double d = Double.valueOf(val); | ||
| if(d.isNaN() || d.isInfinite()) { | ||
| throw new NumberFormatException("val ["+input+"] is not a valid number."); | ||
| } | ||
| return d; | ||
| } catch (NumberFormatException ignore) { | ||
| throw new NumberFormatException("val ["+input+"] is not a valid number."); | ||
| } | ||
| } | ||
| } | ||
| val = removeLeadingZerosOfNumber(input); | ||
| initial = val.charAt(0); | ||
| if(initial == '0' && val.length() > 1) { | ||
| char at1 = val.charAt(1); | ||
| if(at1 >= '0' && at1 <= '9') { | ||
| throw new NumberFormatException("val ["+input+"] is not a valid number."); | ||
| } | ||
| } else if (initial == '-' && val.length() > 2) { | ||
| char at1 = val.charAt(1); | ||
| char at2 = val.charAt(2); | ||
| if(at1 == '0' && at2 >= '0' && at2 <= '9') { | ||
| throw new NumberFormatException("val ["+input+"] is not a valid number."); | ||
| } | ||
| } | ||
| // integer representation. | ||
| // This will narrow any values to the smallest reasonable Object representation | ||
| // (Integer, Long, or BigInteger) | ||
|
|
||
| // BigInteger down conversion: We use a similar bitLength compare as | ||
| // BigInteger#intValueExact uses. Increases GC, but objects hold | ||
| // only what they need. i.e. Less runtime overhead if the value is | ||
| // long lived. | ||
| BigInteger bi = new BigInteger(val); | ||
| if(bi.bitLength() <= 31){ | ||
| return Integer.valueOf(bi.intValue()); | ||
| } | ||
| if(bi.bitLength() <= 63){ | ||
| return Long.valueOf(bi.longValue()); | ||
| } | ||
| return bi; | ||
| } | ||
| throw new NumberFormatException("val ["+input+"] is not a valid number."); | ||
| } | ||
|
|
||
| /** | ||
| * Checks if the value could be considered a number in decimal number system. | ||
| * @param value | ||
| * @return | ||
| */ | ||
| public static boolean potentialNumber(String value){ | ||
| if (value == null || value.isEmpty()){ | ||
| return false; | ||
| } | ||
| return potentialPositiveNumberStartingAtIndex(value, (value.charAt(0)=='-'?1:0)); | ||
| } | ||
|
|
||
| /** | ||
| * Tests if the value should be tried as a decimal. It makes no test if there are actual digits. | ||
| * | ||
| * @param val value to test | ||
| * @return true if the string is "-0" or if it contains '.', 'e', or 'E', false otherwise. | ||
| */ | ||
| private static boolean isDecimalNotation(final String val) { | ||
| return val.indexOf('.') > -1 || val.indexOf('e') > -1 | ||
| || val.indexOf('E') > -1 || "-0".equals(val); | ||
| } | ||
|
|
||
| private static boolean potentialPositiveNumberStartingAtIndex(String value,int index){ | ||
|
Owner
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.
Contributor
Author
There was a problem hiding this comment. Right, the duplicate method will be removed in the next PR. |
||
| if (index >= value.length()){ | ||
| return false; | ||
| } | ||
| return digitAtIndex(value, (value.charAt(index)=='.'?index+1:index)); | ||
| } | ||
|
|
||
| private static boolean digitAtIndex(String value, int index){ | ||
| if (index >= value.length()){ | ||
| return false; | ||
| } | ||
| return value.charAt(index) >= '0' && value.charAt(index) <= '9'; | ||
| } | ||
|
|
||
| /** | ||
| * For a prospective number, remove the leading zeros | ||
| * @param value prospective number | ||
| * @return number without leading zeros | ||
| */ | ||
| private static String removeLeadingZerosOfNumber(String value){ | ||
| if (value.equals("-")){return value;} | ||
| boolean negativeFirstChar = (value.charAt(0) == '-'); | ||
| int counter = negativeFirstChar ? 1:0; | ||
| while (counter < value.length()){ | ||
| if (value.charAt(counter) != '0'){ | ||
| if (negativeFirstChar) {return "-".concat(value.substring(counter));} | ||
| return value.substring(counter); | ||
| } | ||
| ++counter; | ||
| } | ||
| if (negativeFirstChar) {return "-0";} | ||
| return "0"; | ||
| } | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Just import NumberConversionUtil, not the methods. Someone glancing at the code might not realize it is a static method in another class.