2

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.

  • I disagree with being liberal in what you accept from others. It's one thing to accept an InputStream or a PrintWriter and it's quite another to disregard nulls passed when a non-null value is expected. Though I suppose to each his own. In the end, I suppose so long as it is all documented and tested, everything is good. – Neil Dec 2 '14 at 8:46
  • @Neil I don't have a decisive opinion on the subject; it depends on the context. I'm more about "erring on the safe side" in my answer. Does crashing on nulls add any value to the system, or is it just a case of "enterprisey" code where every single method parameter is @NonNull final and a series checkNotNulls follows; just because? – Konrad Morawski Dec 2 '14 at 9:28
  • As you said, it depends heavily on the context, though in my experience, if null is passed to a "addListener", something went wrong. I'd just assume not ignore such an error and pretend everything is fine. Something a lot worse than this could be at play that may affect other areas of the program. That was what I meant. – Neil Dec 2 '14 at 10:39
1

Good question. I believe that such methods don't require its own check simply because:

  1. NullPointerException is thrown should the event occur, so assuming you tested that case at least once, you would know if null were passed.
  2. If you begin adding checks for addListener or removeListener, 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?

  1. 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 onto Service.addListener. One possibility might be a library using reflection like Spring.
  2. *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.
  3. 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!

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.