On Tue, 12 May 2020 22:41:14 GMT, Nir Lisker <[email protected]> wrote:
> > I'm fine with doing a fix, but I need to know which one. Avoiding NPE's and
> > silently doing nothing is IMHO not very desirable as this will give the
> > user of the API no feedback that something went wrong.
> > So I would prefer to fix this by documenting that these cases will result
> > in a NPE.
> > The `bind` method has a similar issue -- it doesn't check its array
> > elements for `null`, and will throw a NPE when attempting to add a listener
> > to `null`. Again, I would just document the NPE so what is clearly a
> > mistake doesn't go unnoticed.
>
> I think that checking the array elements doesn't help much because by the
> time you can check them they will already be used, and that will throw an NPE
> implicitly.
(I must have missed this comment)
What I meant here was to document this behavior, not to add a null check. So
for bind:
/**
* Start observing the dependencies for changes. If the value of one of the
* dependencies changes, the binding is marked as invalid.
*
* @param dependencies
* the dependencies to observe
+ * @throws NullPointerException when dependencies is null, or any of its
elements was null
*/
protected final void bind(Observable... dependencies) {
- if ((dependencies != null) && (dependencies.length > 0)) {
+ if (dependencies.length > 0) {
if (observer == null) {
observer = new BindingHelperObserver(this);
}
for (final Observable dep : dependencies) {
dep.addListener(observer);
}
}
}
And if you want to be even more specific, we could add that when a NPE is
thrown, the result is undefined (as some dependencies may have been added
already). I don't think we want to specify that case.
> `bind` is no-op for `null` or 0-length arrays, but should have probably throw
> an NPE on the `null` case. The 0-length check saves creating the `observer`,
> so I think it makes sense. `unbind` should probably only throw an NPE on
> `null` arrays, but that happens implicitly anyway, so maybe there is no
> change needed unless it's for clarity because we can add a custom error
> message.
I don't think we should throw a specific exception (or add a message), only
documentation. IMHO, passing `null` anywhere in any form, is always incorrect
and doesn't require further explanation unless explicitly allowed in the
documentation.
-------------
PR: https://git.openjdk.org/jfx/pull/198
