10

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?

Community
  • 1
  • 1
Raedwald
  • 46,613
  • 43
  • 151
  • 237
  • If you have a specific requirement like this, subclass `Set` and disallow `null` puts. Also, I would not use the `else` here. – Dave Newton Jan 09 '12 at 12:19
  • 2
    I agree this is incredibly annoying. You allow your class to be created with a generic `Set`, but want to ensure it contains no `null`s as per your class contract. And then if someone actually passes in a `Set` that doesn't permit `null`s, your safety `contains` check throws a NPE because it is in the specification. This is design error IMHO, as there is nothing the caller can do about it (ie, no way to ask if the Set permits nulls or not), not to mention that it seems stupid to throw a NPE in this case instead of just returning `false`. – john16384 Mar 26 '19 at 08:42

3 Answers3

5

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");
  }
}
Dan Hardiker
  • 3,013
  • 16
  • 19
  • 1
    Simple, but `O(N)` in complexity, which is bad for a precondition check. – Raedwald Jan 09 '12 at 11:19
  • 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. – Dan Hardiker Jan 09 '12 at 11:34
  • 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. http://programmers.stackexchange.com/questions/79946/what-is-the-best-retort-to-premature-optimization-is-the-root-of-all-evil. – Raedwald Jan 09 '12 at 11:47
3

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");
  }
}
Matt
  • 17,290
  • 7
  • 57
  • 71
  • 1
    Simple, but `O(N)` in complexity and creates a new object, which is bad for a precondition check. – Raedwald Jan 09 '12 at 11:23
  • 1
    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) – Matt Jan 09 '12 at 11:28
  • 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. – Raedwald Jan 09 '12 at 11:34
  • "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 Jan 09 '12 at 11:36
  • 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 ;) – Matt Jan 09 '12 at 13:12
  • Not ideal, but the best so far in that the `copy` variable can be in-lined to yield something neat. – Raedwald Jan 19 '12 at 14:07
2

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.

Bohemian
  • 412,405
  • 93
  • 575
  • 722