5
\$\begingroup\$

I'm working on a solution for validating Apple receipts. Here's the repository as whole. I based it mostly on the gdx-pay-server library. In my implementation, one of the classes I want to improve is AppleReceiptValidator.

Some questions I have are as follows. First, should I change the System.out.println statements to instead be logs (like slf4j)? Or omit them entirely? I'm considering seriously developing this as a library. Before my second question, for context, the receipt is sent to Apple's servers for validation. Apple then sends back a response. In the response (see the Parse the Reponse section), there are fields that might be of importance. Second, I'm considering adding a method to return both validate and return the entire response of the receipt validation.

In this method, I'm considering return either an Optional or maybe an Either:

Either<Error, String> isValidReturnReponse(String receiptData, Boolean 
excludeOldTransactions)

OR

Optional<String> isValidReturnReponse(String receiptData, Boolean 
excludeOldTransactions)

Last, but not least, I'm using boxed Booleans in the constructor and isValid so those parameters can be null. Those booleans are used for settings such as whether to use the sandbox or not. Is there a better way to do this?

public class AppleReceiptValidator {

    /** Initial capacity of the StringBuilder */
    private static final int INIT_CAPACITY = 2048;

    /** Sandbox URL */
    private final static String SANDBOX_URL = "https://sandbox.itunes.apple.com/verifyReceipt";

    /** Production URL */
    private final static String PRODUCTION_URL = "https://buy.itunes.apple.com/verifyReceipt";

    /** Maps Strings to JSON */
    private static final ObjectMapper mapper = new ObjectMapper();

    /** True for sandbox mode. */
    private Boolean sandbox;

    /** Your app's shared secret */
    private String password;

    public AppleReceiptValidator() {
        this(false, null);
    }

    /**
     * 
     * @param sandbox
     *            If true, SANDBOX, else production.
     * @param secret
     *            Your
     */
    public AppleReceiptValidator(Boolean sandbox, String password) {
        this.sandbox = sandbox;
        this.password = password;
    }

    /**
     * Checks if a receipt is valid or not.
     * 
     * @param receiptData
     *            The text of the receipt (not base64 encoded).
     * @param excludeOldTransactions
     *            Only used for iOS7 style app receipts that contain auto-renewable
     *            or non-renewing subscriptions; set to null otherwise.
     * @return If the receipt is valid.
     */
    public boolean isValid(String receiptData, Boolean excludeOldTransactions) {
        final AppleReceipt receipt = new AppleReceipt(receiptData, password, excludeOldTransactions);
        final String jsonData = receipt.toJson().toString();
        System.out.println(jsonData);
        try {
            final URL url = new URL(sandbox ? SANDBOX_URL : PRODUCTION_URL);
            final HttpURLConnection conn = (HttpsURLConnection) url.openConnection();
            conn.setRequestMethod("POST");
            conn.setDoOutput(true);
            conn.setRequestProperty("Content-Type", "application/json");
            conn.setRequestProperty("Accept", "application/json");
            final OutputStreamWriter wr = new OutputStreamWriter(conn.getOutputStream());
            wr.write(jsonData);
            wr.flush();

            /** obtain the response */
            final BufferedReader rd = new BufferedReader(new InputStreamReader(conn.getInputStream()));
            StringBuilder builder = new StringBuilder(INIT_CAPACITY);
            String line;
            while ((line = rd.readLine()) != null) {
                builder.append(line);
            }
            wr.close();
            rd.close();
            String response = builder.toString();
            System.out.println(builder.toString());
            JsonNode actualObj = mapper.readTree(response);
            int status = actualObj.get("status").asInt();

            /** verify the response: something like {"status":21004} etc... */
            return mapStatus(status);

        } catch (Exception e) {
            /** I/O-error: let's assume bad news... */
            e.printStackTrace();
            return false;
        }
    }

    private boolean mapStatus(int status) {
        switch (status) {
        case 0:
            return true;
        case 21000:
            System.out.println(status + ": App store could not read");
            return false;
        case 21002:
            System.out.println(status + ": Data was malformed");
            return false;
        case 21003:
            System.out.println(status + ": Receipt not authenticated");
            return false;
        case 21004:
            System.out.println(status + ": Shared secret does not match");
            return false;
        case 21005:
            System.out.println(status + ": Receipt server unavailable");
            return false;
        case 21006:
            System.out.println(status + ": Receipt valid but sub expired");
            return false;
        case 21007:
            System.out.println(status + ": Sandbox receipt sent to Production environment");
            return false;
        case 21008:
            System.out.println(status + ": Production receipt sent to Sandbox environment");
            return false;
        default:
            /** unknown error code (nevertheless a problem) */
            System.out.println("Unknown error: status code = " + status);
            return false;
        }
    }
}
\$\endgroup\$
4
\$\begingroup\$

Instead of returning false 10+ times, just return false at the end since the only case you'll return true is if the case is 0. Also, you could reduce some repetition by putting the message in a string and printing it at the end instead/

private boolean mapStatus(int status) {
    String message = status + ": ";
    switch (status) {
        case 0: return true;
        case 21000: message += "App store could not read"; break;
        case 21002: message += "Data was malformed"; break;
        case 21003: message += "Receipt not authenticated"; break;
        case 21004: message += "Shared secret does not match"; break;
        case 21005: message += "Receipt server unavailable"; break;
        case 21006: message += "Receipt valid but sub expired"; break;
        case 21007: message += "Sandbox receipt sent to Production environment"; break;
        case 21008: message += "Production receipt sent to Sandbox environment"; break;
        default:
            /** unknown error code (nevertheless a problem) */
            message = "Unknown error: status code = " + status;
    }
    System.out.println(message);
    return false;
}

As for using slf4j instead of System.out.println(), yes. You definitely should use a logging library instead of using a sysout.

If the developer that uses your library don't want to see the logging messages, he can control it with slf4j, but he can't control it if you're using a System.out.println().

\$\endgroup\$
3
\$\begingroup\$

System.out.println vs logging framework

as TwiN pointed out, a logging library is preferred to system.out.println as it will allow the user to control the logging. Mind that SLF4J itself is only a layer on top of other logging frameworks like log4j(2), logback, ... When creating a library you will need to be aware that you will be introducing this as a dependency.

Validate and return value

You have a few options here according to me:

  • throw an exception when you determine that it is not valid, including any error code / message you want. Otherwise return the message when things are ok, no need for 'Optional' in this case.
  • wrap the return in an object where you have both the exceptions and the return value (eg. ValidationResult). This will allow you to keep a list of validation problems (codes / message / ...) and then you can do whatever formatting you want to do as part of that object
  • you can split it out in multiple methods in which one of them actually checks if it is valid and the other one to take action on it. (potentially throwing an exception)

Constructor parameters:

  • using constructor arguments is fine for me. When you don't want values to be null, you can specify it in the documentation and do some checks on it. eg. throw an exception when password is null.
  • You use 'Boolean' to pass the environment. Suppose someone introduces an extra environment, what will you do with it? eg. sandbox, test, production. Perhaps you can use an enum and then fetch the url accordingly

  • do you need the default constructor?

Environment switching

Does your class really need to know it is operating in a sandbox?
If possible keep that external to this class, as it will allow easier portability cross environments

Responsability

I would consider splitting out some of the responsibilities this class is taking up right now:

  • contacting the webservice (with the password handling)
  • parsing the response

Return codes

I would also consider returning an indication of OK to the calling code (next to avoiding the code duplication as TwiN pointed out)

Initialization

Parts of the class variables get initialized in the constructor, part of them when they are defined. Personally I like to have consistency in this (and look in 1 place), so I would initialize all these values in the constructor.

\$\endgroup\$
  • \$\begingroup\$ Static variables shouldn't be defined in the constructor. But otherwise, good comments! \$\endgroup\$ – Daniel May 30 '18 at 16:30
  • \$\begingroup\$ thank you :-). I was referring to the ObjectMapper, but missed the static keyword over there (I guess because the variable is in lower casing). To me it shouldn't even be static, as when you would test, you can easily mock that dependency \$\endgroup\$ – Manuel May 31 '18 at 5:39

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.