2
\$\begingroup\$

In my code, there is a chain of functions call where the deepest function may throw an exception. I catch and handle it according to the business logic. The problem is that all other functions in the call chain are dependent on this function, thus, if the deepest function returns null all others function will throw the NullPointerException.

My question is, from the code style/maintainability/best practices point of view, can I care about the exception only at the deepest call chain level and in all higher function in the chain use something like catch (NullPointerException ignored) {}?

Is it a good idea at all to use ignored in a try-catch expression or should I really address to all catches even if they are just the consequences of the execution of another function, where I do the proper exception handling?

The code fragment:

public static JSONObject jsonBuilder(String xmlDataSource) {

    JSONObject xmlJSONObj = null;

    try {
        xmlJSONObj = XML.toJSONObject(xmlDataSource);
    } catch (JSONException e) {
        e.printStackTrace();
    }

    return xmlJSONObj;
}

public static void fetchPushData(DBDriver drv, MongoCollection<Document> dbColl, TrackedEpisode episode) {

    String xmlFeed = JsonHandler.xmlDataLoader(episode.getUrlRSS());

    try {
        JSONObject xmlJSONObj = JsonHandler.jsonBuilder(xmlFeed);

        JsonHandler.docBuilderEvent(xmlJSONObj, episode.getEpisodeID());

        JSONArray itemsArr = JsonHandler.getItemsArr(xmlJSONObj);

        episode.setEpisodeLastUpdate(getLatestPubDate(itemsArr));

        drv.insertToDB(dbColl, itemsArr);

    } catch (NullPointerException ignored) {}
}

Here we can see that in case of failure jsonBuilder() throws exception, which I handle and returns null. Now, the docBuilderEvent() will also throw NullPointerException due to null in xmlJSONObj. Should I care about this NullPointerException or it's enough to handle it only in the jsonBuilder()?

\$\endgroup\$
  • \$\begingroup\$ On Code Review I'm afraid that you have to show us the actual code in order for us to review it and be able to tell you if what you are doing is good or bad. \$\endgroup\$ – Simon Forsberg Feb 20 '16 at 13:44
  • \$\begingroup\$ @SimonForsberg, I added a code example. \$\endgroup\$ – Mike B. Feb 20 '16 at 13:56
  • 1
    \$\begingroup\$ Example code is not appropriate for Code Review. Either show us real code, or remove the "simplified example" disclaimer from your question and accept criticism about your exception handling. \$\endgroup\$ – 200_success Feb 20 '16 at 18:16
  • \$\begingroup\$ @200_success, done. Suppose, I just print stack message. \$\endgroup\$ – Mike B. Feb 20 '16 at 18:56
3
\$\begingroup\$

No, that's not a good design. Catching NullPointerException is almost never the correct thing to do. Effectively ignoring it is worse. You're hiding possible programming errors by using it in this way.

jsonBuilder() should be throwing a known, checked exception up to fetchPushDriver(), which should either handle the exception, rethrow it, or wrap it in a new exception and throw that.

\$\endgroup\$
2
\$\begingroup\$

If you were to keep the original method JSONObject::jsonBuilder as is, then you should document the fact that null values are an expected return type. Because null is a documented possibility, then you should not just go ahead and use it; do a null check first. And I also agree with Eric Stein's point that catching a NullPointerException can possibly mask other programmer errors:

public static void fetchPushData(
        final DBDriver drv, 
        final MongoCollection<Document> dbColl, 
        final TrackedEpisode episode ) {

    final String xmlFeed = JsonHandler.xmlDataLoader(episode.getUrlRSS());

    final JSONObject xmlJSONObj = JsonHandler.jsonBuilder(xmlFeed);

    if (xmlJSONObj != null) {

        JsonHandler.docBuilderEvent(xmlJSONObj, episode.getEpisodeID());

        final JSONArray itemsArr = JsonHandler.getItemsArr(xmlJSONObj);

        episode.setEpisodeLastUpdate(getLatestPubDate(itemsArr));

        drv.insertToDB(dbColl, itemsArr);
    }
}

In certain situations it's okay to return null:

Returning null is usually the best idea if you intend to indicate that no data is available.

An empty object implies data has been returned, whereas returning null clearly indicates that nothing has been returned.

Additionally, returning a null will result in a null exception if you attempt to access members in the object, which can be useful for highlighting buggy code - attempting to access a member of nothing makes no sense. Accessing members of an empty object will not fail meaning bugs can go undiscovered.

However, this post wasn't talking about a situation where there was already an exception being handled internally. In your scenario It would make more sense to let the exception get thrown.

public static JSONObject jsonBuilder(final String xmlDataSource) throws JSONException {
    return XML.toJSONObject(xmlDataSource);
}

You can then choose to handle the exception (by choosing an alternative action, or by throwing a new exception) when you call the method from fetchPushData, or just let the exception bubble up.

public static void fetchPushData(
        final DBDriver drv, 
        final MongoCollection<Document> dbColl, 
        final TrackedEpisode episode ) throws JSONException {

    final String xmlFeed = JsonHandler.xmlDataLoader(episode.getUrlRSS());

    final JSONObject xmlJSONObj = JsonHandler.jsonBuilder(xmlFeed);

    JsonHandler.docBuilderEvent(xmlJSONObj, episode.getEpisodeID());

    final JSONArray itemsArr = JsonHandler.getItemsArr(xmlJSONObj);

    episode.setEpisodeLastUpdate(getLatestPubDate(itemsArr));

    drv.insertToDB(dbColl, itemsArr);
} 
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.