I used to think like that, but that was due to the fact that not a single NPE in the JDK includes an error message. Thus, NPE gets the reputation of being a completely useless exception to find in your stack track. In my opinion, IAE makes sense when the argument doesn't follow the rules other than nullity. For example, I changed the isInstanceOf method to not throw an exception if the marker doesn't exist, but if it were kept as an exception, I'd use an IAE since it indicates the given marker is not a valid argument.
This is pretty consistent with the JDK with the added bonus of useful exception messages. On 23 April 2014 08:25, Gary Gregory <[email protected]> wrote: > I like IAE better than NPE because it shows log4j knows the argument is > not allowed, null or otherwise, where NPE says (to me) 'some object was > null and I blew up'. IAE is more of a 'this is not allowed by design, > definitively not a bug in log4j'. > > Gary > > ---------- Forwarded message ---------- > From: <[email protected]> > Date: Wed, Apr 23, 2014 at 10:20 AM > Subject: svn commit: r1589425 - > /logging/log4j/log4j2/trunk/log4j-api/src/main/java/org/apache/logging/log4j/MarkerManager.java > To: [email protected] > > > Author: mattsicker > Date: Wed Apr 23 14:20:56 2014 > New Revision: 1589425 > > URL: http://svn.apache.org/r1589425 > Log: > Update null handling. > > - Don't allow null Marker names. They won't work anyway due to the use > of ConcurrentHashMap. > - Update methods to throw NullPointerException instead of > IllegalArgumentException to match updated javadocs. > - Simplify equals and hashCode based on guaranteed non-nullity of marker > name. > > Modified: > > logging/log4j/log4j2/trunk/log4j-api/src/main/java/org/apache/logging/log4j/MarkerManager.java > > Modified: > logging/log4j/log4j2/trunk/log4j-api/src/main/java/org/apache/logging/log4j/MarkerManager.java > URL: > http://svn.apache.org/viewvc/logging/log4j/log4j2/trunk/log4j-api/src/main/java/org/apache/logging/log4j/MarkerManager.java?rev=1589425&r1=1589424&r2=1589425&view=diff > > ============================================================================== > --- > logging/log4j/log4j2/trunk/log4j-api/src/main/java/org/apache/logging/log4j/MarkerManager.java > (original) > +++ > logging/log4j/log4j2/trunk/log4j-api/src/main/java/org/apache/logging/log4j/MarkerManager.java > Wed Apr 23 14:20:56 2014 > @@ -36,6 +36,7 @@ public final class MarkerManager { > * Retrieve a Marker or create a Marker that has no parent. > * @param name The name of the Marker. > * @return The Marker with the specified name. > + * @throws NullPointerException if the argument is {@code null} > */ > public static Marker getMarker(final String name) { > markerMap.putIfAbsent(name, new Log4jMarker(name)); > @@ -48,6 +49,7 @@ public final class MarkerManager { > * @param parent The name of the parent Marker. > * @return The Marker with the specified name. > * @throws IllegalArgumentException if the parent Marker does not > exist. > + * @throws NullPointerException if the argument is {@code null} > * @deprecated Use the Marker add or set methods to add parent > Markers. Will be removed by final GA release. > */ > @Deprecated > @@ -66,6 +68,7 @@ public final class MarkerManager { > * @param name The name of the Marker. > * @param parent The parent Marker. > * @return The Marker with the specified name. > + * @throws NullPointerException if the argument is {@code null} > * @deprecated Use the Marker add or set methods to add parent > Markers. Will be removed by final GA release. > */ > @Deprecated > @@ -83,15 +86,27 @@ public final class MarkerManager { > private final String name; > private volatile Marker[] parents; > > + /** > + * Constructs a new Marker. > + * @param name the name of the Marker. > + * @throws NullPointerException if the argument is {@code null} > + */ > public Log4jMarker(final String name) { > + if (name == null) { > + // we can't store null references in a ConcurrentHashMap > as it is, not to mention that a null Marker > + // name seems rather pointless. To get an "anonymous" > Marker, just use an empty string. > + throw new NullPointerException("Marker name cannot be > null."); > + } > this.name = name; > this.parents = null; > } > > + // TODO: use java.util.concurrent > + > @Override > public synchronized Marker add(final Marker parent) { > if (parent == null) { > - throw new IllegalArgumentException("A parent marker must > be specified"); > + throw new NullPointerException("A parent marker must be > specified"); > } > // It is not strictly necessary to copy the variable here but > it should perform better than > // Accessing a volatile variable multiple times. > @@ -100,7 +115,6 @@ public final class MarkerManager { > if (localParents != null && (contains(parent, localParents) > || parent.isInstanceOf(this))) { > return this; > } > - // FIXME: should we really be only increasing the array size > by 1 each time? why not something like log(n)? > final int size = localParents == null ? 1 : > localParents.length + 1; > final Marker[] markers = new Marker[size]; > if (localParents != null) { > @@ -113,10 +127,12 @@ public final class MarkerManager { > return this; > } > > + // TODO: add(Marker parent, Marker... moreParents) > + > @Override > public synchronized boolean remove(final Marker parent) { > if (parent == null) { > - throw new IllegalArgumentException("A parent marker must > be specified"); > + throw new NullPointerException("A parent marker must be > specified"); > } > final Marker[] localParents = this.parents; > if (localParents == null) { > @@ -184,7 +200,7 @@ public final class MarkerManager { > @Override > public boolean isInstanceOf(final Marker marker) { > if (marker == null) { > - throw new IllegalArgumentException("A marker parameter is > required"); > + throw new NullPointerException("A marker parameter is > required"); > } > if (this == marker) { > return true; > @@ -213,8 +229,7 @@ public final class MarkerManager { > @Override > public boolean isInstanceOf(final String markerName) { > if (markerName == null) { > - // FIXME: and normally you'd throw an NPE here (ditto for > other cases above) > - throw new IllegalArgumentException("A marker name is > required"); > + throw new NullPointerException("A marker name is > required"); > } > if (markerName.equals(this.getName())) { > return true; > @@ -222,8 +237,7 @@ public final class MarkerManager { > // Use a real marker for child comparisons. It is faster than > comparing the names. > final Marker marker = markerMap.get(markerName); > if (marker == null) { > - // FIXME: shouldn't this just return false? > - throw new IllegalArgumentException("No marker exists with > the name " + markerName); > + return false; > } > final Marker[] localParents = parents; > if (localParents != null) { > @@ -292,19 +306,13 @@ public final class MarkerManager { > if (o == null || !(o instanceof Marker)) { > return false; > } > - > final Marker marker = (Marker) o; > - > - if (name != null ? !name.equals(marker.getName()) : > marker.getName() != null) { > - return false; > - } > - > - return true; > + return name.equals(marker.getName()); > } > > @Override > public int hashCode() { > - return name != null ? name.hashCode() : 0; > + return name.hashCode(); > } > > @Override > > > > > > -- > E-Mail: [email protected] | [email protected] > Java Persistence with Hibernate, Second > Edition<http://www.manning.com/bauer3/> > JUnit in Action, Second Edition <http://www.manning.com/tahchiev/> > Spring Batch in Action <http://www.manning.com/templier/> > Blog: http://garygregory.wordpress.com > Home: http://garygregory.com/ > Tweet! http://twitter.com/GaryGregory > -- Matt Sicker <[email protected]>
