What are the pros and cons for different methods of checking a Collection for a null before adding to a Set?

Question

This is a follow up question to "Is there a basic Java Set implementation that does not permit nulls?". (thank you to all of those who helped with the answer)

It seems that the way to do this, since Sun didn't provide something simple like this OOTB, is with a wrapper / proxy. This seems nearly straight forward. What I am wondering is what are the pros/cons of the following two approaches of adding a collection, or is there another better approach?

Approach #1

public boolean addAll( Collection<? extends E> c) {
    if ( null == c ) {
        throw new NullPointerException( "c cannot be null" );
    }

    /*
     * This seems like a terrible abuse of exceptions when
     * all I want to do is check the set for a null.
     * 
     * However, simply running through each element of the
     * Collection may provide much worse performance for large
     * collections. And according to the Collection API, the
     * contains method may throw NullPointerExceptions if the
     * collection implementation does not allow null elements.
     */
    boolean collectionContainsNull = false;

    try {
        collectionContainsNull = c.contains(null);
    } catch (NullPointerException safeToIgnore) {
        /* Safe to ignore since we do not want nulls */
    }

    if (collectionContainsNull) {
        throw new NullPointerException("c cannot contain null elements");
    }

    this.wrapperSet.addAll(c);
}

Approach #2

public boolean addAll( Collection<? extends E> c) {
    if ( null == c ) {
        throw new NullPointerException( "c cannot be null" );
    }

     E[] a = ( E[] )c.toArray();

     /*
     * We have to iterate through the entire collection to check for
     * a null. This won't take advantage of any optimizations that
     * c.contains may be using.
     *
     * We don't call add(e) here because if we hit a null midway through
     * we would violate the condition that the set remains unchanged
     * on error.
     */
     for ( E e : a ) {
         if (null == e) {
              throw new NullPointerException("c cannot contain nulls");
         }
     }

     this.wrapperSet.addAll(a);
}

Thanks in advance!

Answer 1

What's the point of converting to an array first, and then iterating through the array, rather than just iterating the collection? I'd do the second one without the extraneous conversion.

Or maybe do the add to a temporary set:

public boolean addAll( Collection<? extends E> c) {
    if ( null == c ) {
        throw new NullPointerException( "c cannot be null" );
    }

    Set<E> tempSet = new HashSet<E>();

     /*
     * We have to iterate through the entire collection to check for
     * a null. This won't take advantage of any optimizations that
     * c.contains may be using.
     *

     */
     for ( E e : c) {
         if (null == e) {
              throw new NullPointerException("c cannot contain nulls");
         }
         tempSet.add(e);
     }

     this.wrapperSet = tempSet;
}

Answer 2

The second approach is better. Never hide exceptions - you are relying on the assumption that c.contains(null) only throws a NullPointerException in the case that there is a null in the collection. However if the NullPointeException is thrown because of a problem with an equals method you will have a bug in your code - and you will have hidden it.

Edit:

From the JavaDoc for contains, NullPointerException is thrown - if the specified element is null and this collection does not permit null elements (optional).

Given that it is an optional method you may wind up with an UnsupportedOperationException being thrown instead of the NullPointerExcepion (in addition to hiding the an error in equals).

Answer 3

Making a copy of the argument collection in an array is safer. The collection argument may change concurrently (perhaps it is a concurrent collection) (or may be written maliciously).

Also catching runtime exceptions isn't a nice way to do things.

You might want to use Object[] instead of E[] and move the unsafe cast to later. You will also need an Arrays.asList(a) or similar.

Answer 4

You could also add an a short-circuit to not test for nulls if the collection being added subclasses your NonNullCollection parent class.

Answer 5

So it seems that both methods are bad. I really am only posting this answer to consolidate the information into a single post.

TofuBeer pointed out the overlooked flaw in logic for method 1 where there are other exceptions that could be thrown that will not be caught. So in general it is always bad to try to catch exceptions for unexceptional conditions.

Paul pointed out that what I thought was a safe cast really is not. I expected the collection generic to be enforced on the output cast, however it will returned Object[]. As he points out I could use a temp set to store the data while I search for a null.

Also, as Tom confirmed it is possible the collection argument may change concurrently so a defensive copy into a new object is a good idea).

So I guess the desired method is:

public boolean addAll( Collection<? extends E> c ) {
    if ( null == c ) {
        throw new NullPointerException( "c cannot be null" );
    }

     // Create a defensive copy to prevent concurrent modifications of c
     Set<E> tmpSet = new HashSet<E>( c );

     /*
      * We have to iterate through the entire collection to check for
      * a null. This won't take advantage of any optimizations that
      * c.contains may be using.
      */
     for ( E e : tmpSet ) {
         if ( null == e ) {
              throw new NullPointerException( "c cannot contain nulls" );
         }
     }

     return this.wrapperSet.addAll( tmpSet );
}