API Issues

[sdoward]

As a follow up from #488 I want to bring up some concerns I have around the Braintree API in this SDK. I have used the braintree sdk for around 5 years. And although there is some improvement from 3.x -> 4.x there is still much to be desired. I am sure there are many concerns that you have that I am not aware of, but I am also aware that similar SDKs (stripe for example) don't suffer from the same problems.

I will be referencing the Paypal flow but I suspect similar issues, concepts exist on the other payment methods.

1. Catch All Callbacks with Nullable Types

---

It seems like the new SDK has adopted a pattern of passing success objects and exceptions in the same function. For me this seems like an anti-pattern and one which I don't really see in the JVM world).

With your current paypalClien.onBrowserSwitchResult API we have to handle the following states...

Nonce	Exception	How I handle them
Not Null	Not Null	Handle Nonce
Not Null	Null	Handle Nonce
Null	Not Null	Handle Error
Null	Null	throw IllegalStateException

Braintree doesn't offer advice or documentation on state 1 and 4 because it is not supposed to happen. But we shouldn't have "dead ends" in our code and we should be handling all eventualities. So in this case your API is giving us these 4 states when it shouldn't.

An obvious solution would be separated functions...

public interface PayPalBrowserSwitchResultCallback {

   void onSuccess(@Notnull PayPalAccountNonce payPalAccountNonce)

   void onError(@NotNull Exception error);
}

In the case we don't have any nullable object and only 2 states. But there are other options available such as an Either Monad (probably overkills in this case). There are other options available.

Also your current pattern results in odd behaviour in the PayPalFlowStartedCallback: to understand if the flow has been started successfully the consumers are given a nullable exception.

Also this approach doesn't scale well. The amount of states increases exponentially with extra parameters. If you were to add another nullable parameter the consumers would have to handle 9 states. Another 2 parameters would mean 16 states.

2. Multiple Callbacks/Results for the Same Flow

---

2.1 paypalClient.tokenizePayPalAccount(activity, callback)

The name suggests that this is a callback when the flow starts but it seems to be mostly used as a way to pass exceptions when an internal call fails. The docs don't handle this successful use case so it's not clear if this is needed at all.

2.2 braintreeClient.deliverBrowserSwitchResult(activity)

This call is confusing for a number of reasons. Firstly, we might not actually be delivering a result, we are just required to call this method on every resume wether or not the flow has been started. We are also providing an activity not giving a result, we are receiving a result.

Secondly, we are passing an activity to Braintree to get an object that we then pass back to Braintree...

    BrowserSwitchResult browserSwitchResult = braintreeClient.deliverBrowserSwitchResult(this);
    if (browserSwitchResult != null && browserSwitchResult.getRequestCode() == BraintreeRequestCodes.PAYPAL) {
      payPalClient.onBrowserSwitchResult(browserSwitchResult, (payPalAccountNonce, error) -> {
        if (payPalAccountNonce != null) {
          // Send nonce to server
          String nonce = payPalNonce.getString();
        } else {
          // handle error
        }
      });
    }

Clients don't really care about this result (we care about the end result). It feels like it shouldn't be exposed to clients at all. As a consumer I don't care if you are switching a browser or not. It's a leaked internal implementation detail

2.3 payPalClient.onBrowserSwitchResult(browserSwitchResult, callback)

This is the actually callback I care about but unfortunately I need to care about a ton of state and usecases from the calls above.

Suggestion

I would love to see 1 listener that can react upon success, exceptions and whatever else..

client.setTokenisationListener(listener)

or even

client.tokenizePayPalAccount(listener)

If the clients needs events from the android system it can accept them..

client.onResume(activity)
client.onActivityResult(requestCode, resultCode, data))

However there are libraries from Android that allow for Lifecycle-Aware Components that might remove that friction.

3. Tight Activity Coupling

---

As a developer I want to be able to write code that isn't coupled to the android system. This way I can write unit tests, make my code modular and reusable. Most of the Android world moved away from writing business logic in Activities however these API's almost require us to write payment related logic there. I understand that Braintree has a dependency on Context and it needs to get data from outside of the system but that doesn't mean that we should be forced to write code in activities. The android team has done a lot of work to help devs and library devs alike to move away from this.

4. Multiple Clients

---

I already touched upon this in 2.2. Essentially is odd that consumers need to pass data from 1 client to the other and check that data. Maybe I could understand this if the clients were independent but PaypalClient has a dependency on BraintreeClient. So it seems that the aforementioned code should be part of your SDK and not part of my source code.

5. Unexplained Nuances

---

Apparently the clients need to do this following...

  @Override
  protected void onNewIntent(Intent newIntent) {
    super.onNewIntent(newIntent);
    // required if your activity's launch mode is "singleTop", "singleTask", or "singleInstance"
    setIntent(newIntent);
  }

Is this something to do with the Braintree sdk? Or something that Android requires?

---

Hopefully I have been constructive in my criticism. I look forward to the response

[sshropshire]

@sdoward also feel free to share details on your current architecture to help us understand the context. We'll take this into consideration and respond to each point to give some feedback from our end on what we've learned maintaining the Android SDK over the years.

[sshropshire]

@sdoward I want to say thanks for your time as well. This is some of the best feedback we've gotten so far. We want to offer the best developer experience possible and we'll take everything into consideration as we evolve the SDK interface.

[sarahkoop]

@sdoward - If you'd like to give more input, please check out the discussion on Developer Experience we created.

[sshropshire]

@sdoward like @sarahkoop mentioned we have a proposed solution that may address some of your concerns and we're gathering feedback to make sure we're headed in the right direction.

And in response to your feedback:

1. Catch All Callbacks with Nullable Types

---

This is something to consider. We would like to guide our merchants to all possible success / error scenarios. On the other hand, we do like the translation of SAM interfaces to Java lambdas / Kotlin functions. The API parameter pairs will be constrained to [success, error] tuples.

We may consider a listener pattern though in cases where an app switch or browser switch occurs. This makes more sense since there are a lot of asynchronous operations that take place in some payment flows, and receiving notifications via listener onSuccess and onError callbacks will improve the developer experience.

2. Multiple Callbacks/Results for the Same Flow

---

This is a good call. Please review our Developer Experience discussion. We may have a solution that allows us to fully encapsulate the browser switch delivery logic and allow the merchant to set a listener to receive updates.

3. Tight Activity Coupling

---

In the past, we leveraged applicationContext to start new activities, but this had the side effect of requiring us to start a new task when calling startActivityForResult. We should be able to remove this restriction as we move to the new registerForActivityResultAPI. Then we can have BraintreeClient constructor overloads for Activity and Fragment.

4. Multiple Clients

---

As a 3rd party SDK, we do have to work around not having ownership of the merchant Activity. Our primary motivation when creating v4 of our library was to make the SDK more modular. Our Feature Clients compose BraintreeClient to access a common set of functionality. We may leverage Jetpack Lifecycle observers in the future to encapsulate more of this behavior and make life easier for our merchants.

5. Unexplained Nuances

---

This is actually included in Google's documentation for Activity#onNewIntent() we may have to keep this around for now until Google decides to deprecate this method in favor of something more flexible like they did with registerForActivityResult.