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

Please don't force the use of checked exceptions when it isn't necessary #75

Closed
wants to merge 1 commit into from
Closed

Conversation

ghost
Copy link

@ghost ghost commented Dec 16, 2012

The JSONObject class forces users to wrap even a simple put operation in
a try-catch block, despite the fact that most times the only situation
that can cause the exception to be thrown is a null key value. I have
changed this to use the standard NullPointerException in the case of
null keys, and the standard IllegalArguementException in the case of
non-finite numbers. The JSONObject class may now be used without an
excessive amount of try/catch code that is often superfluous. Take for
instance this common idiom:

JSONObject jsonObj = new JSONObject();
try {
jsonObj.put("definitelyNotNull", 123);
} catch (JSONException ex) {
/* This will never, ever occur. Yet I'm still forced to catch the
* exception.
*/
}

With this commit, the following can safely be condensed down to:

JSONObject jsonObj = new JSONObject();
jsonObj.put("definitelyNotNull", 123);

The JSONObject class forces users to wrap even a simple put operation in
a try-catch block, despite the fact that most times the only situation
that can cause the exception to be thrown is a null key value. I have
changed this to use the standard NullPointerException in the case of
null keys, and the standard IllegalArguementException in the case of
non-finite numbers. The JSONObject class may now be used without an
excessive amount of try/catch code that is often superfluous. Take for
instance this common idiom:

JSONObject jsonObj = new JSONObject();
try {
	jsonObj.put("definitelyNotNull", 123);
} catch (JSONException ex) {
	/* This will never, ever occur. Yet I'm still forced to catch the
	 * exception.
	 */
}

With this commit, the following can safely be condensed down to:

JSONObject jsonObj = new JSONObject();
jsonObj.put("definitelyNotNull", 123);
@douglascrockford
Copy link
Contributor

This is not true: "The JSONObject class forces users to wrap even a simple put operation in
a try-catch block". You may also add a throws JSONException, which IDEs can add for you automatically.

The JSON package only throws one type of exception in an attempt at simplification. Some people complain about the amount of ceremony that Java requires around exceptions. I suggest you take that up with Gosling.

@ghost
Copy link
Author

ghost commented Dec 18, 2012

Declaring a method that touches a JSONObject as throwing JSONException just pushes the problem further up the chain. Worse, by having all of the exceptions be of the same type, it requires code that wants to catch and handle these situations to dig into the exception state to determine what exactly went wrong. I'm not bringing up an issue with the language, it's an issue with your choice of using checked exceptions for things that the java standard libraries typically consider a runtime problem - i.e., a null pointer or an illegal argument. I can understand your motivation for having the get operations throw checked exceptions, as there are no safe 'default values' for missing keys (though again, returning null is fairly typical in this situation for reference types). How do you envision client code meaningfully dealing with a JSONException from an attempt to insert a null key?

Thanks for your time.

[edit]

If you look at the forked repositories from this one, several have changes in place to avoid this same issue with most choosing to have JSONException extend RuntimeException. The people have spoken! :-)

BGehrels pushed a commit to BGehrels/JSON-java that referenced this pull request Apr 29, 2020
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.

1 participant