-
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
reduces the use of unnecessary exceptions #249
Conversation
I don't have any major issue with this, however, please verify that this test case returns the same value before and after your changes: JSONObject jo = new JSONObject("{\"int\":\"123\",\"true\":\"true\",\"false\":\"false\"}");
assert jo.optBoolean("true",false)==true : "unexpected optBoolean value";
assert jo.optBoolean("false",true)==false : "unexpected optBoolean value";
assert jo.optInt("int",0)==123 : "unexpected optInt value";
assert jo.optLong("int",0)==123 : "unexpected optLong value";
assert jo.optDouble("int",0.0)==123.0 : "unexpected optDouble value";
assert jo.optBigInteger("int",BigInteger.ZERO).compareTo(new BigInteger("123"))==0 : "unexpected optBigInteger value";
assert jo.optBigDecimal("int",BigDecimal.ZERO).compareTo(new BigDecimal("123"))==0 : "unexpected optBigDecimal value";
JSONArray ja = new JSONArray("[\"123\",\"true\",\"false\"]");
assert ja.optBoolean(1,false)==true : "unexpected optBoolean value";
assert ja.optBoolean(2,true)==false : "unexpected optBoolean value";
assert ja.optInt(0,0)==123 : "unexpected optInt value";
assert ja.optLong(0,0)==123 : "unexpected optLong value";
assert ja.optDouble(0,0.0)==123.0 : "unexpected optDouble value";
assert ja.optBigInteger(0,BigInteger.ZERO).compareTo(new BigInteger("123"))==0 : "unexpected optBigInteger value";
assert ja.optBigDecimal(0,BigDecimal.ZERO).compareTo(new BigDecimal("123"))==0 : "unexpected optBigDecimal value"; |
I added the test cases here: from what I can tell your PR should pass them both before and after, but I did not run them on your PR |
I run the tests before and after my changes against the updated test repository including your new test case and they both passes the test successfully. |
@stleary This PR looks good to me. I'm OK with including this in the next release if you are. |
The changes are probably OK. However, it should not be necessary to wade through pages of formatting in order to find the real code changes. It is actually helpful to convert tabs to 4-spaces, so that is OK. And if you find egregious formatting problems, those are OK to fix as well (don't think I saw any of those in this review). Otherwise only lines containing actual code changes should be touched. Please resubmit without the format changes. If someone else wants to do this task, that would be fine as well. |
I reverted my last commit and created the changes to reduce the exceptions again (they are the same changes like in the first commit) without the changes to the formatting. There are just a hand full of "important" formating changes in this commit. |
Thanks, this is helpful and simplifies the review. |
What problem does this code solve? Changes to the API? Will this require a new release? Should the documentation be updated? Change to unit tests? |
Creating an Exception is an expensive operation, because of the collection of the stacktrace.
If the Exception is caught and ignored, this is even an unnecessary operation. Therefore I added null checks und class instance checks to prevent ClassCastExceptions and NullPointerExceptions from being created and caught afterwards.
The changes are passing all test from the test suite as the behavior when a JSONException is thrown or the defaultValue is returned has not been changed.