Preferred way of checking nulls for defensive copies

Suppose I have the following method:

public void change(Map<String, String> map)

I would like to throw an exception if map is null and make a d开发者_如何学编程efensive copy if it's not.

Would this be preferred:

public void change(Map<String, String> map)
{
    Map<String, String> temp = null;

    synchronized (map) {
            if (map == null)
                throw new NullPointerException("map is null");
        temp = new HashMap<String, Object>(map);
}

or

public void change(Map<String, String> map)
    try {
        Map<String, String> temp = new HashMap<String, Object>(map);
    } 
    catch (NullPointerException e) {
        throw new NullPointerException("map is null");
    }

or is there a better way? why and why not.

EDITS:

revised a few typos I had:

properties should be map

(String, Object) should be (String, String)

Thanks!

---

Well your first way will throw an exception if map is null not due to your code, but due to the synchronized block.

It's not really clear what you're trying to do given that in your second approach you're declaring a local variable, which will be pointless. Is the idea to set an instance variable, or to perform more work on the map after creating the copy?

You also need to know what's going to happen in terms of threading - you can't guarantee that everything will be synchronizing on the map reference itself. Personally I'd isolate threading from other concerns, and handle it more centrally. I'd write something like this, using Guava for both calls:

public void change(Map<String, String> map)
{
    Preconditions.checkNotNull(map);
    Map<String, String> copy = Maps.newHashMap(map);
    // Use copy here
}

The Maps.newHashMap call uses type inference to avoid you having to state the type arguments as you would in a normal constructor call (unless you're using Java 7).

---

I would go with neither.

Your first example doesn't do much if there's no chance of multithreading. (Is properties thread safe?)

Your second example is poor because it's using exceptions to define program flow when it's so easily avoided. Personally, I would use

public void change(Map<String,String> map) {
    if(map == null) throw new NullPointerException("map is null");
    Map<String,String> temp = new HashMap<String,String>(map);
    // continue with temp...
}

---

Definitely the first way, but without the synchronized (unless you're really sure that you need it); if you are able to easily and reliably predict whether an exception will be thrown, it is more readable (and slightly more efficient) to test that condition than using a try/catch. However, you should never throw NullPointerExceptions yourself - throw IllegalArgumentException instead.

---

In Java 7, you can use Objects.requireNonNull to check if a reference is not null and throw a NPE if it is.

public static void change(Map<String, String> map){
    Objects.requireNonNull(map, "map should not be null");
    Map<String, Object> copy = new HashMap<>(map);
}

---

I would do the following:

public void change(/*NonNull*/ Map<String, String> map) {
  if (map == null) {
      throw new IllegalArgumentException("Parameter must be nonnull"); 
  };
  //...
}

Since it's an exported method, clients should get an exception on their abstraction level, so IllegalArgumentException is better than NPE.

The comment /*NonNull*/ can be checked already at compile-time using the nullness checker from the (pluggable type-)checker framework. Something similar is also possible via Findbugs, IDEA or Netbeans, but their checks are not sound, i.e. they can have false positives. Doing a programmatic nullness check still makes sense, because a client might not make the complie-time nullness checks (i.e. use the checker framework/Findbugs/IDEA/Netbeans/...).