From 94165caf8cca543c03b432f3bff2839e141db4a5 Mon Sep 17 00:00:00 2001 From: Ben Christensen Date: Mon, 3 Jun 2013 15:48:59 -0700 Subject: [PATCH] Common approach for IllegalArgumentException on subscribe methods As brought up in https://github.com/Netflix/RxJava/issues/278 there were inconsistencies in how arguments were being validated on subscribe methods. All arguments to subscribe are now validated correctly at the beginning of method invocation and IllegalArgumentException thrown if null arguments are injected. According to Rx Guideline 6.5 a null argument is considered "a catastrophic error occurs that should bring down the whole program anyway" and thus we immediately throw. A null argument should be caught immediately in development and has nothing to do with subscribe invocation which is what guideline 6.5 is talking about (since a null Observer can't have onError called on it anyways). --- rxjava-core/src/main/java/rx/Observable.java | 85 ++++++++++++-------- 1 file changed, 53 insertions(+), 32 deletions(-) diff --git a/rxjava-core/src/main/java/rx/Observable.java b/rxjava-core/src/main/java/rx/Observable.java index ffcb100868..6fc645f162 100644 --- a/rxjava-core/src/main/java/rx/Observable.java +++ b/rxjava-core/src/main/java/rx/Observable.java @@ -157,11 +157,16 @@ protected Observable() { * @param observer * @return a {@link Subscription} reference that allows observers * to stop receiving notifications before the provider has finished sending them + * @throws IllegalArgumentException + * if null argument provided */ public Subscription subscribe(Observer observer) { // allow the hook to intercept and/or decorate Func1, Subscription> onSubscribeFunction = hook.onSubscribeStart(this, onSubscribe); // validate and proceed + if (observer == null) { + throw new IllegalArgumentException("observer can not be null"); + } if (onSubscribeFunction == null) { throw new IllegalStateException("onSubscribe function can not be null."); // the subscribe function can also be overridden but generally that's not the appropriate approach so I won't mention that in the exception @@ -224,6 +229,8 @@ public Subscription subscribe(Observer observer) { * The {@link Scheduler} that the sequence is subscribed to on. * @return a {@link Subscription} reference that allows observers * to stop receiving notifications before the provider has finished sending them + * @throws IllegalArgumentException + * if null argument provided */ public Subscription subscribe(Observer observer, Scheduler scheduler) { return subscribeOn(scheduler).subscribe(observer); @@ -241,11 +248,14 @@ private Subscription protectivelyWrapAndSubscribe(Observer o) { @SuppressWarnings({ "rawtypes", "unchecked" }) public Subscription subscribe(final Map callbacks) { - // lookup and memoize onNext + if (callbacks == null) { + throw new RuntimeException("callbacks map can not be null"); + } Object _onNext = callbacks.get("onNext"); if (_onNext == null) { - throw new RuntimeException("onNext must be implemented"); + throw new RuntimeException("'onNext' key must contain an implementation"); } + // lookup and memoize onNext final FuncN onNext = Functions.from(_onNext); /** @@ -291,10 +301,11 @@ public Subscription subscribe(final Object o) { return subscribe((Observer) o); } - // lookup and memoize onNext if (o == null) { - throw new RuntimeException("onNext must be implemented"); + throw new IllegalArgumentException("onNext can not be null"); } + + // lookup and memoize onNext final FuncN onNext = Functions.from(o); /** @@ -328,6 +339,9 @@ public Subscription subscribe(final Object o, Scheduler scheduler) { } public Subscription subscribe(final Action1 onNext) { + if (onNext == null) { + throw new IllegalArgumentException("onNext can not be null"); + } /** * Wrapping since raw functions provided by the user are being invoked. @@ -349,9 +363,6 @@ public void onError(Exception e) { @Override public void onNext(T args) { - if (onNext == null) { - throw new RuntimeException("onNext must be implemented"); - } onNext.call(args); } @@ -364,10 +375,14 @@ public Subscription subscribe(final Action1 onNext, Scheduler scheduler) { @SuppressWarnings({ "rawtypes", "unchecked" }) public Subscription subscribe(final Object onNext, final Object onError) { - // lookup and memoize onNext if (onNext == null) { - throw new RuntimeException("onNext must be implemented"); + throw new IllegalArgumentException("onNext can not be null"); + } + if (onError == null) { + throw new IllegalArgumentException("onError can not be null"); } + + // lookup and memoize onNext final FuncN onNextFunction = Functions.from(onNext); /** @@ -385,9 +400,7 @@ public void onCompleted() { @Override public void onError(Exception e) { handleError(e); - if (onError != null) { - Functions.from(onError).call(e); - } + Functions.from(onError).call(e); } @Override @@ -403,6 +416,12 @@ public Subscription subscribe(final Object onNext, final Object onError, Schedul } public Subscription subscribe(final Action1 onNext, final Action1 onError) { + if (onNext == null) { + throw new IllegalArgumentException("onNext can not be null"); + } + if (onError == null) { + throw new IllegalArgumentException("onError can not be null"); + } /** * Wrapping since raw functions provided by the user are being invoked. @@ -419,16 +438,11 @@ public void onCompleted() { @Override public void onError(Exception e) { handleError(e); - if (onError != null) { - onError.call(e); - } + onError.call(e); } @Override public void onNext(T args) { - if (onNext == null) { - throw new RuntimeException("onNext must be implemented"); - } onNext.call(args); } @@ -441,10 +455,17 @@ public Subscription subscribe(final Action1 onNext, final Action1 @SuppressWarnings({ "rawtypes", "unchecked" }) public Subscription subscribe(final Object onNext, final Object onError, final Object onComplete) { - // lookup and memoize onNext if (onNext == null) { - throw new RuntimeException("onNext must be implemented"); + throw new IllegalArgumentException("onNext can not be null"); + } + if (onError == null) { + throw new IllegalArgumentException("onError can not be null"); } + if (onComplete == null) { + throw new IllegalArgumentException("onComplete can not be null"); + } + + // lookup and memoize onNext final FuncN onNextFunction = Functions.from(onNext); /** @@ -456,17 +477,13 @@ public Subscription subscribe(final Object onNext, final Object onError, final O @Override public void onCompleted() { - if (onComplete != null) { - Functions.from(onComplete).call(); - } + Functions.from(onComplete).call(); } @Override public void onError(Exception e) { handleError(e); - if (onError != null) { - Functions.from(onError).call(e); - } + Functions.from(onError).call(e); } @Override @@ -482,6 +499,15 @@ public Subscription subscribe(final Object onNext, final Object onError, final O } public Subscription subscribe(final Action1 onNext, final Action1 onError, final Action0 onComplete) { + if (onNext == null) { + throw new IllegalArgumentException("onNext can not be null"); + } + if (onError == null) { + throw new IllegalArgumentException("onError can not be null"); + } + if (onComplete == null) { + throw new IllegalArgumentException("onComplete can not be null"); + } /** * Wrapping since raw functions provided by the user are being invoked. @@ -498,16 +524,11 @@ public void onCompleted() { @Override public void onError(Exception e) { handleError(e); - if (onError != null) { - onError.call(e); - } + onError.call(e); } @Override public void onNext(T args) { - if (onNext == null) { - throw new RuntimeException("onNext must be implemented"); - } onNext.call(args); }