Imagine the following interface:
interface Service {
addListener(Listener l)
removeListener(Listener l)
}
Should I check for null values while add/remove? Is it a good idea on remove to check if the listener was registered before (e.g. listener B was never registered and should be removed)?
What behaviour helps developers here to identify problems without creating new ones like IllegalArgumentExceptions?
2
First and foremost, you should be consistent with the framework convention. Java is a big world to be sure. I use Java for Android, and here the convention is rather permissive.
For example, setOnClickListener
allows you to pass in null
– this is a crude but nonambiguous way of removing the listener.
From Android source code:
public class View implements Drawable.Callback, KeyEvent.Callback, AccessibilityEventSource {
// [snip]
/**
* Register a callback to be invoked when this view is clicked. If this view is not
* clickable, it becomes clickable.
*
* @param l The callback that will run
*/
public void setOnClickListener(OnClickListener l) {
if (!isClickable()) {
setClickable(true);
}
getListenerInfo().mOnClickListener = l;
}
Null is fine:
/**
* Return whether this view has an attached OnClickListener. Returns
* true if there is a listener, false if there is none.
*/
public boolean hasOnClickListeners() {
ListenerInfo li = mListenerInfo;
return (li != null && li.mOnClickListener != null);
}
Where multiple listeners are allowed, removing a listener is a tolerant operation, too:
/**
* Remove a listener for layout changes.
*
* @param listener The listener for layout bounds change.
*/
public void removeOnLayoutChangeListener(OnLayoutChangeListener listener) {
ListenerInfo li = mListenerInfo;
if (li == null || li.mOnLayoutChangeListeners == null) {
return;
}
li.mOnLayoutChangeListeners.remove(listener);
}
mOnLayoutChangeListeners
is an ArrayList
, and ArrayList.remove(Object object)
doesn’t crash on an inexistent item – it merely returns false
.
The bottom line is the robustness principle – be conservative in what you do, be liberal in what you accept from others.
When in doubt, I’d go with liberal, unless you have a solid reason not to. It’s simplier and less surprising. Consistency is the king, and the principle of astonishment is based on consistency.
Any preconditions should be clearly documented. Your definition of interface Service
doesn’t by itself promise to throw exceptions when null is passed in.
@NonNull
and @Nullable
annotations may help to remove ambiguity, and they are supported by good IDEs such as IntelliJ Idea or Android Studio.
3
Good question. I believe that such methods don’t require its own check simply because:
NullPointerException
is thrown should the event occur, so assuming you tested that case at least once, you would know ifnull
were passed.- If you begin adding checks for
addListener
orremoveListener
, then you really would have to perform basic checks on every method in order to stay consistent throughout your code, which will have a negative impact on performance and readability.
I think the critical point behind checks like these is to ensure that parameters are what they should be within reasonable margins. If you are the only one calling said method and in only a couple places, adding these checks is superfluous. If anything, use junit to test specific methods utilizing these events to ensure that everything goes as expected.
When do you really need to perform these checks?
- There is a serious possibility that the parameter getting passed may be
null
, and for one reason or another, you cannot check prior to passing it ontoService.addListener
. One possibility might be a library using reflection like Spring. - *Your* program is not calling these methods. If you’re writing a library to be used by others, it is a good practice to check all public methods that you intend to be called by the program using your library.
- You’re entering a rather sensitive part of your program where a lot could go wrong. You need to be able to make certain assumptions about your input and you need for them to not be proven wrong, preferably not on a production computer during your lunch hour break. (I call these sanity checks)
I’m mainly referring to addListener
, though to answer your other question about removeListener
, I usually don’t find it necessary to know whether or not a listener was removed. If that information is important, you could optionally return the listener that was removed or null
otherwise, though I would only do so if that information were needed (and it usually isn’t in my experience).
I hope that helps!