commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Sébastien Brisard <sebastien.bris...@m4x.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 16:07:50 GMT
Hi Gilles,


2012/6/23 Gilles Sadowski <gilles@harfang.homelinux.org>:
> 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
>

Well, the idea is this: if cb is null, fall back to default instead of
throwing an exception, which I find quite unnecessary here. The first
constructor was indeed fine, but the new implementation of the second
constructor (including the test for null) would lead to duplicate
code. So the first constructor, with restricted number of parameters
(assuming default values for unspecified parameters) calls the second
constructor, which is the most general one. I should have thought that
avoiding code duplication in constructors was good practice.

>From your reaction, I assume you do not like the idea of null being a
meaningful value for the parameter cb? If so, I'm sorry. I really
thought that this change was rather minor. If you like, I can cancel
this change, and open a JIRA ticket.

Best regards,
Sébastien


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


Mime
View raw message