commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gilles Sadowski <gil...@harfang.homelinux.org>
Subject Re: svn commit: r1353140 - /commons/proper/math/trunk/src/main/java/org/apache/commons/math3/util/Incrementor.java
Date Sat, 23 Jun 2012 15:55:20 GMT
Hello.

On Sat, Jun 23, 2012 at 03:09:15PM -0000, celestin@apache.org wrote:
> Author: celestin
> Date: Sat Jun 23 15:09:14 2012
> New Revision: 1353140

Either I don't understand this change, or I don't agree with it.

> 
> URL: http://svn.apache.org/viewvc?rev=1353140&view=rev
> Log:
> In o.a.c.m3.Incrementor, modified constructor to allow for null values of the MaxCountExceededCallback.
Null value was previously not checked wich could lead to a NullPointerException much later
(at exhaustion of the counter).
> 
> Modified:
>     commons/proper/math/trunk/src/main/java/org/apache/commons/math3/util/Incrementor.java
> 
> Modified: commons/proper/math/trunk/src/main/java/org/apache/commons/math3/util/Incrementor.java
> URL: http://svn.apache.org/viewvc/commons/proper/math/trunk/src/main/java/org/apache/commons/math3/util/Incrementor.java?rev=1353140&r1=1353139&r2=1353140&view=diff
> ==============================================================================
> --- commons/proper/math/trunk/src/main/java/org/apache/commons/math3/util/Incrementor.java
(original)
> +++ commons/proper/math/trunk/src/main/java/org/apache/commons/math3/util/Incrementor.java
Sat Jun 23 15:09:14 2012
> @@ -58,13 +58,7 @@ public class Incrementor {
>       * @param max Maximal count.
>       */
>      public Incrementor(int max) {
> -        this(max,
> -             new MaxCountExceededCallback() {
> -                 /** {@inheritDoc} */
> -                 public void trigger(int max) {
> -                     throw new MaxCountExceededException(max);
> -                 }
> -             });
> +        this(max, null);
>      }

You set the callback to "null" here but...

>      /**
> @@ -72,12 +66,22 @@ public class Incrementor {
>       * counter exhaustion.
>       *
>       * @param max Maximal count.
> -     * @param cb Function to be called when the maximal count has been reached.
> +     * @param cb Function to be called when the maximal count has been reached
> +     * (can be {@code null}).
>       */
>      public Incrementor(int max,
>                         MaxCountExceededCallback cb) {
>          maximalCount = max;
> -        maxCountCallback = cb;
> +        if (cb != null) {
> +            maxCountCallback = cb;
> +        } else {
> +            maxCountCallback = new MaxCountExceededCallback() {
> +                /** {@inheritDoc} */
> +                public void trigger(int max) {
> +                    throw new MaxCountExceededException(max);
> +                }
> +            };
> +        }
>      }

... here, when it *is* null, you set it to something not null. So it seems
to be a contorted way of disallowing null while saying the opposite in the
Javadoc!

The first constructor was fine as it was.
In the second, a check for not null was missing:
---
    public Incrementor(int max,
                       MaxCountExceededCallback cb) {
        if (cb == null) {
            throw new NullArgumentException();
        }
        maximalCount = max;
        maxCountCallback = cb;
    }
---

Best regards,
Gilles

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org


Mime
View raw message