1
\$\begingroup\$

I'm in doubts what approach to take here, please have a look on the code below:

private void processBarCodeResponse(String decodedQrCode) {
        JSONObject jsonObject = JsonUtil.composeJsonObject(decodedQrCode);
        SalePoint salePoint = parseSalePoint(jsonObject);
        if (salePoint == null) return; // handle null value here
        saveSalePoint(salePoint);
    }

private SalePoint parseSalePoint(JSONObject jsonObject) {
        SalePoint salePoint = new SalePoint();
        try {
            salePoint.type = jsonObject.getString("type");
            salePoint.url = jsonObject.getString("url");
            salePoint.salePointId = jsonObject.getString("salePointId");
        } catch (JSONException e) {
            Logger.logError(TAG, "jsonObject " + jsonObject.toString() + " wasn't parsed " +
                    "correctly");
            return null; // point of concern
        }

        return salePoint;
    }

For the sake of readability I moved the try/catch block to the parseSalePoint() method, but then if parsing fails I'm enforced to return null from the method and check for null value in processBarCodeResponse() method. Wouldn't it be right to just handle the JSONException in processBarCodeResponse() method and stop its execution once it is thrown?

\$\endgroup\$
  • 2
    \$\begingroup\$ This question is on the very edge of being primarily opinion based. I am not voting to close but I am downvoting because of the way the question is formulated. \$\endgroup\$ – Simon Forsberg Dec 26 '13 at 19:21
5
\$\begingroup\$

A possibly useful pattern here can be the Null Object-pattern.

Instead of returning null, return a "null SalePoint". Perhaps something like new SalePoint();, or new SalePoint("some special url");. Then make your saveSalePoint method do nothing if the SalePoint has a special url indicating that it's null.

Another option is to return null from your parseSalePoint method as you are currently doing and then simply return from the saveSalePoint method if the argument is null which will remove the null check from processBarCodeResponse.

Honestly, I can't tell you which option you should use or not. I can only present you with alternatives. Personally I like Null Object pattern a lot, but it doesn't always makes much sense to use that pattern.

By the way, why isn't SalePoint immutable? Instead of first creating a SalePoint object and then setting it's values one by one, read the values into variables and call a constructor which takes type, url, salePointId as arguments and get rid of the accessible properties (make them private, and also get rid of the setter-methods if you have any)

\$\endgroup\$
  • \$\begingroup\$ Simon, SalePoint object is just a DTO, it encapsulates values parsed from a String, I don't even have getters and setters for it, just public variables in it, that's all. What benefits could I get if I made it immutable? I can't use Null Object pattern because as I said SalePoint does nothing. \$\endgroup\$ – Eugene Dec 26 '13 at 15:25
  • \$\begingroup\$ @Eugene Do you have any benefit from not having it immutable? You could refactor to make a save method inside your SalePoint object so you call salePoint.save(possibleAdditionalParameters). It's up to you of course, I don't know all your code and even if I did I can't tell you how to do it, I'm just saying what you could do. \$\endgroup\$ – Simon Forsberg Dec 26 '13 at 16:27
4
\$\begingroup\$

I think the right thing to do would be to move the exception handler into processBarCodeResponse().

private void processBarCodeResponse(String decodedQrCode) {
    JSONObject jsonObject = JsonUtil.composeJsonObject(decodedQrCode);
    try {
        SalePoint salePoint = parseSalePoint(jsonObject);
        saveSalePoint(salePoint);
    } catch (JSONException e) {
        // You don't need to call .toString() explicitly
        Logger.logError(TAG, "jsonObject " + jsonObject + " wasn't parsed correctly");
    }

}

private SalePoint parseSalePoint(JSONObject jsonObject) throws JSONException {
    SalePoint salePoint = new SalePoint();
    salePoint.type = jsonObject.getString("type");
    salePoint.url = jsonObject.getString("url");
    salePoint.salePointId = jsonObject.getString("salePointId");
    return salePoint;
}

Your brace indentation was a bit weird; I hope that was just a result of pasting the code into this website.

\$\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.