Will do. The important thing is to add the @throws javadoc for runtime exceptions (especially for log4j-api).
On 23 April 2014 09:59, Ralph Goers <[email protected]> wrote: > I didn’t realize you had changed IAEs I coded to NPEs. Please change them > back. > > Ralph > > On Apr 23, 2014, at 8:53 AM, Ralph Goers <[email protected]> > wrote: > > I agree with Gary on this. > > Ralph > > On Apr 23, 2014, at 8:36 AM, Gary Gregory <[email protected]> wrote: > > On the call site side, it seems cleaner to catch an IAE and log the fact > that some programming error took place. If I catch NPE, I cannot say that > there is a bug or that someone is passing a known bad value, all I can say > is that a null object is where the program does not allow it. > > We'll have to agree to disagree. > > Gary > > > On Wed, Apr 23, 2014 at 10:49 AM, Matt Sicker <[email protected]> wrote: > >> 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]> >> > > > > -- > 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]>
