pivot-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Sandro Martini <sandro.mart...@gmail.com>
Subject Fwd: svn commit: r1712423 - in /pivot/trunk: core/src/org/apache/pivot/util/Utils.java wtk/src/org/apache/pivot/wtk/Bounds.java wtk/src/org/apache/pivot/wtk/Component.java wtk/src/org/apache/pivot/wtk/Window.java
Date Fri, 06 Nov 2015 09:11:11 GMT
Hi all,
just look at the commit (from Roger) for doing some cleanup, so not
null checks for arguments are refactored in a utility method like
this:

Utils.checkNull(size, "size");

I'm not against this (refactor common code is always good :-) ), but
just I wonder if in trunk we could use a way for not null checks more
aligned to Java 7 (minimum version required for 2.1.0), see here:

http://docs.oracle.com/javase/7/docs/api/java/util/Objects.html#requireNonNull%28T,%20java.lang.String%29

but in this (standard way) the generated Exception is a
NullPointerException (doesn't look so good in that case, to me) and
not an IllegalArgumentException.
Some time ago I commetted some changes (under the PIVOT-799
issue/improvement) like this:

Objects.requireNonNull(t);
Note that with it in some cases could be useful to have a simpler
check and assignment to a variable in a single line, like:
this.name = Objects.requireNonNull(nameArgument, "name cannot be null");

Even those checks could be updated to the new common method, no
problem, consistency is better. Maybe even an additional version that
returns the value or throws the RuntimeException like in the example
just seen.


Note that we could even change the not null check implementation
inside the general utility method later if/when needed ...

Last, related issue should be
[PIVOT-895](https://issues.apache.org/jira/browse/PIVOT-895), just for
info.

Generally speaking, our checks are mainly preconditions for methods
(something like require in Scala) so IllegalArgumentException are
good, maybe we could even add other methods to do a classical not null
check that throws NullPointerException to use in other cases (like
Java 7 methods, ou even using it in its implementation) ... so maybe
the checkNull method could be renamed in requireNotNull and the new
one as checkNotNull or similar ... and last maybe even a notEmpty for
Strings (if used somewhere).

What do you think ?

Bye,
Sandro




---------- Forwarded message ----------
From:  <rwhitcomb@apache.org>
Date: 2015-11-03 22:44 GMT+01:00
Subject: svn commit: r1712423 - in /pivot/trunk:
core/src/org/apache/pivot/util/Utils.java
wtk/src/org/apache/pivot/wtk/Bounds.java
wtk/src/org/apache/pivot/wtk/Component.java
wtk/src/org/apache/pivot/wtk/Window.java
To: commits@pivot.apache.org


Author: rwhitcomb
Date: Tue Nov  3 21:44:48 2015
New Revision: 1712423

URL: http://svn.apache.org/viewvc?rev=1712423&view=rev
Log:
Code cleanup:  Make a new method in the org.apache.pivot.utils.Utils
class which does the
common operation of checking an argument for null and throwing
"IllegalArgumentException"
if it is.  This will simplify and standardize a lot of code, since
this pattern appears
so often, whenever objects are passed as arguments.  And it will
simplify getting an
understandable message thrown out in every case.  Before, there were
many places that
just threw the plain exception without a detail message, but this
change will simplify
the checking so there can always be a meaningful message.

Change just a few files in the "wtk" area that do a lot of this
checking already.
Of course, there are many more places that could use this treatment...


Modified:
    pivot/trunk/core/src/org/apache/pivot/util/Utils.java
    pivot/trunk/wtk/src/org/apache/pivot/wtk/Bounds.java
    pivot/trunk/wtk/src/org/apache/pivot/wtk/Component.java
    pivot/trunk/wtk/src/org/apache/pivot/wtk/Window.java

Modified: pivot/trunk/core/src/org/apache/pivot/util/Utils.java
URL: http://svn.apache.org/viewvc/pivot/trunk/core/src/org/apache/pivot/util/Utils.java?rev=1712423&r1=1712422&r2=1712423&view=diff
==============================================================================
--- pivot/trunk/core/src/org/apache/pivot/util/Utils.java (original)
+++ pivot/trunk/core/src/org/apache/pivot/util/Utils.java Tue Nov  3
21:44:48 2015
@@ -38,4 +38,26 @@ public class Utils {
         return s1.equals(s2);
     }

+    /**
+     * Check if the input argument is {@code null} and throw an
+     * {@link IllegalArgumentException} if so, with an optional
+     * message derived from the given string.
+     *
+     * @param value The argument value to check for {@code null}.
+     * @param description A description for the value used to
+     * construct a message like {@code "xxx must not be null."}. Can be
+     * {@code null} in which case a plain {@link IllegalArgumentException}
+     * is thrown without any detail message.
+     * @throws IllegalArgumentException if the value is {@code null}.
+     */
+    public static void checkNull(Object value, String description) {
+        if (value == null) {
+            if (description == null) {
+                throw new IllegalArgumentException();
+            } else {
+                throw new IllegalArgumentException(description + "
must not be null.");
+            }
+        }
+    }
+
 }

Modified: pivot/trunk/wtk/src/org/apache/pivot/wtk/Bounds.java
URL: http://svn.apache.org/viewvc/pivot/trunk/wtk/src/org/apache/pivot/wtk/Bounds.java?rev=1712423&r1=1712422&r2=1712423&view=diff
==============================================================================
--- pivot/trunk/wtk/src/org/apache/pivot/wtk/Bounds.java (original)
+++ pivot/trunk/wtk/src/org/apache/pivot/wtk/Bounds.java Tue Nov  3
21:44:48 2015
@@ -21,6 +21,7 @@ import java.io.Serializable;
 import org.apache.pivot.collections.Dictionary;
 import org.apache.pivot.json.JSONSerializer;
 import org.apache.pivot.serialization.SerializationException;
+import org.apache.pivot.util.Utils;

 /**
  * Class representing the bounds of an object.
@@ -46,13 +47,8 @@ public final class Bounds implements Ser
     }

     public Bounds(Point origin, Dimensions size) {
-        if (origin == null) {
-            throw new IllegalArgumentException("origin is null.");
-        }
-
-        if (size == null) {
-            throw new IllegalArgumentException("size is null.");
-        }
+        Utils.checkNull(origin, "origin");
+        Utils.checkNull(size, "size");

         x = origin.x;
         y = origin.y;
@@ -61,9 +57,7 @@ public final class Bounds implements Ser
     }

     public Bounds(Bounds bounds) {
-        if (bounds == null) {
-            throw new IllegalArgumentException("bounds is null.");
-        }
+        Utils.checkNull(bounds, "bounds");

         x = bounds.x;
         y = bounds.y;
@@ -72,9 +66,7 @@ public final class Bounds implements Ser
     }

     public Bounds(Dictionary<String, ?> bounds) {
-        if (bounds == null) {
-            throw new IllegalArgumentException("bounds is null.");
-        }
+        Utils.checkNull(bounds, "bounds");

         if (bounds.containsKey(X_KEY)) {
             x = ((Integer) bounds.get(X_KEY)).intValue();
@@ -102,9 +94,7 @@ public final class Bounds implements Ser
     }

     public Bounds(java.awt.Rectangle rectangle) {
-        if (rectangle == null) {
-            throw new IllegalArgumentException("rectangle is null.");
-        }
+        Utils.checkNull(rectangle, "rectangle");

         x = rectangle.x;
         y = rectangle.y;
@@ -160,9 +150,7 @@ public final class Bounds implements Ser
     }

     public boolean contains(Point point) {
-        if (point == null) {
-            throw new IllegalArgumentException("point is null");
-        }
+        Utils.checkNull(point, "point");

         return contains(point.x, point.y);
     }
@@ -173,9 +161,7 @@ public final class Bounds implements Ser
     }

     public boolean contains(Bounds bounds) {
-        if (bounds == null) {
-            throw new IllegalArgumentException("bounds is null");
-        }
+        Utils.checkNull(bounds, "bounds");

         return contains(bounds.x, bounds.y, bounds.width, bounds.height);
     }
@@ -187,9 +173,7 @@ public final class Bounds implements Ser
     }

     public boolean intersects(Bounds bounds) {
-        if (bounds == null) {
-            throw new IllegalArgumentException("bounds is null");
-        }
+        Utils.checkNull(bounds, "bounds");

         return intersects(bounds.x, bounds.y, bounds.width, bounds.height);
     }
@@ -236,14 +220,12 @@ public final class Bounds implements Ser
         return getClass().getName() + " [" + x + "," + y + ";" +
width + "x" + height + "]";
     }

-    public static Bounds decode(String value) {
-        if (value == null) {
-            throw new IllegalArgumentException();
-        }
+    public static Bounds decode(String boundsValue) {
+        Utils.checkNull(boundsValue, "boundsValue");

         Bounds bounds;
         try {
-            bounds = new Bounds(JSONSerializer.parseMap(value));
+            bounds = new Bounds(JSONSerializer.parseMap(boundsValue));
         } catch (SerializationException exception) {
             throw new IllegalArgumentException(exception);
         }

Modified: pivot/trunk/wtk/src/org/apache/pivot/wtk/Component.java
URL: http://svn.apache.org/viewvc/pivot/trunk/wtk/src/org/apache/pivot/wtk/Component.java?rev=1712423&r1=1712422&r2=1712423&view=diff
==============================================================================
--- pivot/trunk/wtk/src/org/apache/pivot/wtk/Component.java (original)
+++ pivot/trunk/wtk/src/org/apache/pivot/wtk/Component.java Tue Nov  3
21:44:48 2015
@@ -35,6 +35,7 @@ import org.apache.pivot.json.JSONSeriali
 import org.apache.pivot.serialization.SerializationException;
 import org.apache.pivot.util.ImmutableIterator;
 import org.apache.pivot.util.ListenerList;
+import org.apache.pivot.util.Utils;
 import org.apache.pivot.wtk.effects.Decorator;

 /**
@@ -177,9 +178,7 @@ public abstract class Component implemen

         @Override
         public void insert(Decorator decorator, int index) {
-            if (decorator == null) {
-                throw new IllegalArgumentException("decorator is null");
-            }
+            Utils.checkNull(decorator, "decorator");

             // Repaint the the component's previous decorated region
             if (parent != null) {
@@ -198,9 +197,7 @@ public abstract class Component implemen

         @Override
         public Decorator update(int index, Decorator decorator) {
-            if (decorator == null) {
-                throw new IllegalArgumentException("decorator is null.");
-            }
+            Utils.checkNull(decorator, "decorator");

             // Repaint the the component's previous decorated region
             if (parent != null) {
@@ -744,9 +741,7 @@ public abstract class Component implemen
      */
     @SuppressWarnings("unchecked")
     protected void setSkin(Skin skin) {
-        if (skin == null) {
-            throw new IllegalArgumentException("skin is null.");
-        }
+        Utils.checkNull(skin, "skin");

         if (this.skin != null) {
             throw new IllegalStateException("Skin is already installed.");
@@ -868,9 +863,7 @@ public abstract class Component implemen

     @SuppressWarnings("unchecked")
     public Container getAncestor(String ancestorTypeName) throws
ClassNotFoundException {
-        if (ancestorTypeName == null) {
-            throw new IllegalArgumentException();
-        }
+        Utils.checkNull(ancestorTypeName, "ancestorTypeName");

         return getAncestor((Class<? extends Container>)
Class.forName(ancestorTypeName));
     }
@@ -898,9 +891,7 @@ public abstract class Component implemen
     }

     public final void setSize(Dimensions size) {
-        if (size == null) {
-            throw new IllegalArgumentException("size is null.");
-        }
+        Utils.checkNull(size, "size");

         setSize(size.width, size.height);
     }
@@ -1105,9 +1096,7 @@ public abstract class Component implemen
     }

     public final void setPreferredSize(Dimensions preferredSize) {
-        if (preferredSize == null) {
-            throw new IllegalArgumentException("preferredSize is null.");
-        }
+        Utils.checkNull(preferredSize, "preferredSize");

         setPreferredSize(preferredSize.width, preferredSize.height);
     }
@@ -1232,9 +1221,7 @@ public abstract class Component implemen
      * @param widthLimits The new width limits (min and max).
      */
     public final void setWidthLimits(Limits widthLimits) {
-        if (widthLimits == null) {
-            throw new IllegalArgumentException("widthLimits is null.");
-        }
+        Utils.checkNull(widthLimits, "widthLimits");

         setWidthLimits(widthLimits.minimum, widthLimits.maximum);
     }
@@ -1316,9 +1303,7 @@ public abstract class Component implemen
      * @param heightLimits The new height limits (min and max).
      */
     public final void setHeightLimits(Limits heightLimits) {
-        if (heightLimits == null) {
-            throw new IllegalArgumentException("heightLimits is null.");
-        }
+        Utils.checkNull(heightLimits, "heightLimits");

         setHeightLimits(heightLimits.minimum, heightLimits.maximum);
     }
@@ -1415,9 +1400,7 @@ public abstract class Component implemen
      * @see #setLocation(int, int)
      */
     public final void setLocation(Point location) {
-        if (location == null) {
-            throw new IllegalArgumentException("location cannot be null.");
-        }
+        Utils.checkNull(location, "location");

         setLocation(location.x, location.y);
     }
@@ -1568,9 +1551,7 @@ public abstract class Component implemen
      * the component is not a descendant of the specified ancestor.
      */
     public Point mapPointToAncestor(final Container ancestor, int
xArgument, int yArgument) {
-        if (ancestor == null) {
-            throw new IllegalArgumentException("ancestor is null");
-        }
+        Utils.checkNull(ancestor, "ancestor");

         int xArgumentMutable = xArgument;
         int yArgumentMutable = yArgument;
@@ -1603,9 +1584,7 @@ public abstract class Component implemen
      * the component is not a descendant of the specified ancestor.
      */
     public Point mapPointToAncestor(Container ancestor, Point location) {
-        if (location == null) {
-            throw new IllegalArgumentException();
-        }
+        Utils.checkNull(location, "location");

         return mapPointToAncestor(ancestor, location.x, location.y);
     }
@@ -1621,9 +1600,7 @@ public abstract class Component implemen
      * the component is not a descendant of the specified ancestor.
      */
     public Point mapPointFromAncestor(final Container ancestor, int
xArgument, int yArgument) {
-        if (ancestor == null) {
-            throw new IllegalArgumentException("ancestor is null");
-        }
+        Utils.checkNull(ancestor, "ancestor");

         int xArgumentMutable = xArgument;
         int yArgumentMutable = yArgument;
@@ -1646,9 +1623,7 @@ public abstract class Component implemen
     }

     public Point mapPointFromAncestor(Container ancestor, Point location) {
-        if (location == null) {
-            throw new IllegalArgumentException();
-        }
+        Utils.checkNull(location, "location");

         return mapPointFromAncestor(ancestor, location.x, location.y);
     }
@@ -1694,9 +1669,7 @@ public abstract class Component implemen
      * part of the component hierarchy.
      */
     public Bounds getVisibleArea(Bounds area) {
-        if (area == null) {
-            throw new IllegalArgumentException("area is null.");
-        }
+        Utils.checkNull(area, "area");

         return getVisibleArea(area.x, area.y, area.width, area.height);
     }
@@ -1768,9 +1741,7 @@ public abstract class Component implemen
      * @param area The area to be made visible.
      */
     public void scrollAreaToVisible(Bounds area) {
-        if (area == null) {
-            throw new IllegalArgumentException("area is null.");
-        }
+        Utils.checkNull(area, "area");

         scrollAreaToVisible(area.x, area.y, area.width, area.height);
     }
@@ -1951,9 +1922,7 @@ public abstract class Component implemen
      * @param immediate Whether or not the area needs immediate painting.
      */
     public final void repaint(Bounds area, boolean immediate) {
-        if (area == null) {
-            throw new IllegalArgumentException("area is null.");
-        }
+        Utils.checkNull(area, "area");

         repaint(area.x, area.y, area.width, area.height, immediate);
     }
@@ -2512,9 +2481,7 @@ public abstract class Component implemen
      * @param styles A map containing the styles to apply.
      */
     public void setStyles(Map<String, ?> styles) {
-        if (styles == null) {
-            throw new IllegalArgumentException("styles is null.");
-        }
+        Utils.checkNull(styles, "styles");

         for (String key : styles) {
             getStyles().put(key, styles.get(key));
@@ -2528,9 +2495,7 @@ public abstract class Component implemen
      * @throws SerializationException if the string doesn't conform
to JSON standards.
      */
     public void setStyles(String styles) throws SerializationException {
-        if (styles == null) {
-            throw new IllegalArgumentException("styles is null.");
-        }
+        Utils.checkNull(styles, "styles");

         setStyles(JSONSerializer.parseMap(styles));
     }
@@ -2557,9 +2522,7 @@ public abstract class Component implemen
      * @param styleName The name of an already loaded style to apply.
      */
     public void setStyleName(String styleName) {
-        if (styleName == null) {
-            throw new IllegalArgumentException();
-        }
+        Utils.checkNull(styleName, "styleName");

         Map<String, ?> stylesLocal = namedStyles.get(styleName);

@@ -2576,9 +2539,7 @@ public abstract class Component implemen
      * @param styleNames List of style names to apply to this component.
      */
     public void setStyleNames(Sequence<String> styleNames) {
-        if (styleNames == null) {
-            throw new IllegalArgumentException();
-        }
+        Utils.checkNull(styleNames, "styleNames");

         for (int i = 0, n = styleNames.getLength(); i < n; i++) {
             setStyleName(styleNames.get(i));
@@ -2591,9 +2552,7 @@ public abstract class Component implemen
      * @param styleNames Comma-delimited list of style names to apply.
      */
     public void setStyleNames(String styleNames) {
-        if (styleNames == null) {
-            throw new IllegalArgumentException();
-        }
+        Utils.checkNull(styleNames, "styleNames");

         for (String styleName : styleNames.split(",")) {
             setStyleName(styleName.trim());
@@ -2601,7 +2560,6 @@ public abstract class Component implemen
     }

     /**
-     * Returns the user data dictionary.
      * @return The user data dictionary for this component.
      */
     public UserDataDictionary getUserData() {

Modified: pivot/trunk/wtk/src/org/apache/pivot/wtk/Window.java
URL: http://svn.apache.org/viewvc/pivot/trunk/wtk/src/org/apache/pivot/wtk/Window.java?rev=1712423&r1=1712422&r2=1712423&view=diff
==============================================================================
--- pivot/trunk/wtk/src/org/apache/pivot/wtk/Window.java (original)
+++ pivot/trunk/wtk/src/org/apache/pivot/wtk/Window.java Tue Nov  3
21:44:48 2015
@@ -25,6 +25,7 @@ import org.apache.pivot.collections.Hash
 import org.apache.pivot.collections.Sequence;
 import org.apache.pivot.util.ImmutableIterator;
 import org.apache.pivot.util.ListenerList;
+import org.apache.pivot.util.Utils;
 import org.apache.pivot.util.Vote;
 import org.apache.pivot.wtk.media.Image;

@@ -76,9 +77,7 @@ public class Window extends Container {

             if (keyStroke != previousKeyStroke) {
                 if (window != null) {
-                    if (keyStroke == null) {
-                        throw new IllegalStateException();
-                    }
+                    Utils.checkNull(keyStroke, "keyStroke");

                     if (window.actionMap.containsKey(keyStroke)) {
                         throw new IllegalArgumentException("A mapping
for " + keyStroke
@@ -99,9 +98,7 @@ public class Window extends Container {
         }

         public void setKeyStroke(String keyStroke) {
-            if (keyStroke == null) {
-                throw new IllegalArgumentException("keyStroke is null.");
-            }
+            Utils.checkNull(keyStroke, "keyStroke");

             setKeyStroke(Keyboard.KeyStroke.decode(keyStroke));
         }
@@ -115,9 +112,7 @@ public class Window extends Container {

             if (action != previousAction) {
                 if (window != null) {
-                    if (action == null) {
-                        throw new IllegalStateException();
-                    }
+                    Utils.checkNull(action, "action");

                     window.actionMap.put(keyStroke, action);

@@ -129,9 +124,7 @@ public class Window extends Container {
         }

         public void setAction(String actionID) {
-            if (actionID == null) {
-                throw new IllegalArgumentException("actionID is null");
-            }
+            Utils.checkNull(actionID, "actionID");

             Action actionLocal = Action.getNamedActions().get(actionID);
             if (actionLocal == null) {
@@ -550,9 +543,7 @@ public class Window extends Container {
      * window; <tt>false</tt>, otherwise.
      */
     public boolean isOwner(Window window) {
-        if (window == null) {
-            throw new IllegalArgumentException("window is null.");
-        }
+        Utils.checkNull(window, "window");

         Window ownerLocal = window.getOwner();

@@ -599,9 +590,7 @@ public class Window extends Container {
      * @throws IllegalArgumentException if the owner argument is {@code null}.
      */
     public final void open(Window ownerArgument) {
-        if (ownerArgument == null) {
-            throw new IllegalArgumentException();
-        }
+        Utils.checkNull(ownerArgument, "owner");

         open(ownerArgument.getDisplay(), ownerArgument);
     }
@@ -615,9 +604,7 @@ public class Window extends Container {
      * has no owner.
      */
     public void open(Display display, Window ownerArgument) {
-        if (display == null) {
-            throw new IllegalArgumentException("display is null.");
-        }
+        Utils.checkNull(display, "display");

         if (ownerArgument != null) {
             if (!ownerArgument.isOpen()) {
@@ -784,9 +771,7 @@ public class Window extends Container {
      * @param iconURL The location of the icon to set.
      */
     public void setIcon(URL iconURL) {
-        if (iconURL == null) {
-            throw new IllegalArgumentException("iconURL is null.");
-        }
+        Utils.checkNull(iconURL, "iconURL");

         Image icon = Image.loadFromCache(iconURL);

@@ -802,14 +787,12 @@ public class Window extends Container {
      * @see #setIcon(URL)
      */
     public void setIcon(String iconName) {
-        if (iconName == null) {
-            throw new IllegalArgumentException("iconName is null.");
-        }
+        Utils.checkNull(iconName, "iconName");

         ClassLoader classLoader =
Thread.currentThread().getContextClassLoader();
         URL url = classLoader.getResource(iconName.substring(1));
         if (url == null) {
-            throw new IllegalArgumentException("cannot find icon
resource " + iconName);
+            throw new IllegalArgumentException("Cannot find icon
resource " + iconName);
         }
         setIcon(url);
     }

Mime
View raw message