Json exception handling #16
Conversation
richarddd
commented
Feb 28, 2017
richarddd
commented
- Adding json exception handling allows for subscribers to know if json parsing has failed
- Adding a check if service is bound allows subscribers to be notified if we are unable to bind, i.e billing is not supported
throw error if service fails to bind so it could be handleled by subscriber
added json exception to log method
| purchase = PurchaseParser.parse(data.getStringExtra("INAPP_PURCHASE_DATA")); | ||
| } catch (final JSONException e) { | ||
| ReactiveBilling.log(e, "Cannot parse purchase json"); | ||
| observable.mergeWith(Observable.create(new Observable.OnSubscribe<PurchaseResponse>() { |
There was a problem hiding this comment.
Can this be simplified to observable.mergeWith(Observable.error(e))?
| } catch (SecurityException e) { | ||
| ReactiveBilling.log(e, "Bind service error"); | ||
| subscriber.onError(e); | ||
| } | ||
|
|
||
| //service is not bound, throw exception | ||
| if(!bound){ | ||
| subscriber.onError(new IllegalStateException("Service unable to bind")); |
There was a problem hiding this comment.
If we can't bind to the service, do we need to exit here as opposed to continuing below?
| @@ -33,7 +36,10 @@ protected void onBillingServiceReady(BillingService billingService, Observer<? s | |||
| try { | |||
| observer.onNext(billingService.getPurchases(purchaseType, continuationToken)); | |||
| observer.onCompleted(); | |||
| } catch (RemoteException e) { | |||
| } catch (RemoteException | JSONException e) { | |||
| if(e instanceof JSONException) { | |||
There was a problem hiding this comment.
IMHO catch clauses should only be collapsed if they're identical. If you have to instanceof check in here, they should be separate clauses. That being said, is there a reason not to log all errors here?
There was a problem hiding this comment.
True, however, when we have a JSONException we want to know that the JSON is invalid
There was a problem hiding this comment.
Two things:
-
I'm not suggesting getting rid of the log message, I'm suggesting that if there are different actions in the case of different exception types, then having multiple catch blocks is clearer than collapsing them into one so they can be separated by
instanceofchecks. -
Again, not suggesting removing the logging for
JSONExceptions, rather saying log any exception.
| @@ -31,7 +35,10 @@ protected void onBillingServiceReady(BillingService billingService, Observer<? s | |||
| try { | |||
| observer.onNext(billingService.getSkuDetails(purchaseType, productIds)); | |||
| observer.onCompleted(); | |||
| } catch (RemoteException e) { | |||
| } catch (RemoteException | JSONException e) { | |||
| if(e instanceof JSONException) { | |||
| return PurchaseType.SUBSCRIPTION; | ||
| } else { | ||
| throw new IllegalArgumentException("Unknown purchase type: " + value); | ||
| switch (value) { |
There was a problem hiding this comment.
This can be dangerous if value isn't marked @NotNull. Users will get an NPE if it is.
|
Thanks for the PR @richarddd. |
|
@lukaspili @rharter updated! |
