Neat way to check whether a Set does not contain a null

Question

I have a method that is given a Set of objects. A method it delegates to requires that the Set does not contain any null elements. I would like to check the precondition that the Set contains no null elements early, in the method before the delegation. The obvious code do do so is this:

public void scan(Set<PlugIn> plugIns) {
   if (plugIns == null) {
      throw new NullPointerException("plugIns");
   } else if (plugIns.contains(null)) {
      throw new NullPointerException("plugIns null element");
   }
   // Body
 }

But this is incorrect, because Set.contains() may throw a NullPointerException if the Set implementation itself does not permit null elements. Catching then ignoring the NullPointerException in that case would work but would be inelegant. Is there a neat way to check this precondition?

---

Is there a design flaw in the Set interface? If a Set implementation may never contain a null, why not instead require Set.contains(null) to always return false? Or have a isNullElementPermitted() predicate?

Solution 1

The simplest way would be to enumerate the Set and check for nulls.

public void scan(Set<PlugIn> plugIns) {
  if (plugIns == null) throw new NullPointerException("plugIns");
  for (PlugIn plugIn : plugIns) {
    if (plugIn == null) throw new NullPointerException("plugIns null element");
  }
}

Solution 2

Create a HashSet from plugIns and check for the existence of null

public void scan(Set<PlugIn> plugIns) {
  if (plugIns == null) throw new NullPointerException("plugIns");
  Set<PlugIn> copy = new HashSet<PlugIn>(plugIns);
  if (copy.contains(null)) {
      throw new NullPointerException("null is not a valid plugin");
  }
}

Solution 3

Just catch the NullPointerException if thrown and ignore it:

public void scan(Set<PlugIn> plugIns) {
    if (plugIns == null) {
        throw new NullPointerException("plugIns");
    }

    NullPointerException e = null;
    try {
        if (plugIns.contains(null)) {
            // If thrown here, the catch would catch this NPE, so just create it
            e = new NullPointerException("plugIns null element");
        }
    } catch (NullPointerException ignore) { }

    if (e != null) {
        throw e;
    }
    // Body
}

This creates only a minor overhead if thrown, but if you don't use the exception (especially the strack trace), it's actually quite lightweight.

Comments

[Raedwald]

Simple, but O(N) in complexity, which is bad for a precondition check.

[Raedwald]

Simple, but O(N) in complexity and creates a new object, which is bad for a precondition check.

[Matt]

How many thousand times per second do you scan for new plug-ins? ;) In my experience, copying or creating new collections is rarely a performance problem. Maybe you should prevent null from being added where plugIns is coming from (to me it sounds like you're fixing someone else's buggy code, unless there is a valid reason for the existence of a null-plugin)

[Raedwald]

I'm not doing this to work-around buggy code. I want to do it because it is better to detect faults early. Imagine this was an API method: we would not control the calling code, but might want to provide good diagnostics.

[Dan Hardiker]

To quote another: "Preoptimisation is the root of all evil". Are you sure this is a performance bottleneck? Given what you're doing I would guess that there's an architectural issue elsewhere.

[Raedwald]

"How many thousand times per second do you scan for new plug-ins?" Good point, your solution would have adequate performance for the specific case that I have. But I'm also wondering how I might do the check in other situatios, where performance would be more important.

[Raedwald]

My concern is not premature optimisation. A neat solution should be general purpose and reasonably efficient. Everytime someone uses a HashSet without first measuring performance, and get the benefit of fast Set.contains(), they are not engaging in premature optimisation. programmers.stackexchange.com/questions/79946/….

[Matt]

Ok, I see your point. But let's suppose that this is part of an actual API: In that case it depends on the interface /contract between the caller and the callee. If null is allowed as part of the set, you can handle it in the //body with a simple if (because we we don't know how to handle Null-Plugins). If null is not allowed to be in the set, then there is simply no need to check for null unless you suspect that the API is buggy. IMHO this has lot to do with personal taste, so there is no wrong or right. Hope that helps ;)

[Raedwald]

Not ideal, but the best so far in that the copy variable can be in-lined to yield something neat.

[unknown_boundaries]

Don't you think your NPE candidates are more of IllegalArgumentException candidates?