Skip to main content
Code Review

Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link

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 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);
} 

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);
} 

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);
} 
replaced http://codereview.stackexchange.com/ with https://codereview.stackexchange.com/
Source Link

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 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);
} 

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);
} 

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);
} 
Source Link
flakes
  • 1.9k
  • 1
  • 16
  • 28

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);
} 
lang-java

AltStyle によって変換されたページ (->オリジナル) /