11

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?

Improve this question
2
  • If you have a specific requirement like this, subclass Set and disallow null puts. Also, I would not use the else here. Commented Jan 9, 2012 at 12:19
  • 3
    I agree this is incredibly annoying. You allow your class to be created with a generic Set, but want to ensure it contains no nulls as per your class contract. And then if someone actually passes in a Set that doesn't permit nulls, 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. Commented Mar 26, 2019 at 8:42

3 Answers 3

Reset to default
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"); } }
Improve this answer
Sign up to request clarification or add additional context in comments.

3 Comments

Simple, but O(N) in complexity, which is bad for a precondition check.
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.
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/….
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"); } }
Improve this answer

6 Comments

Simple, but O(N) in complexity and creates a new object, which is bad for a precondition check.
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)
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.
"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.
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 ;)
|
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.

Improve this answer

1 Comment

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.