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 19:19:43 GMT
Hi Sébastien.

> >> 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 question to ask is: What does it mean for "cb" to be null?
IMO, it means nothing, and I'd tell that to the user by throwing an
exception.

Why would someone call the two-args constructor, whereas the one-arg
constructor performs the exact same thing without ambiguity?


> The first
> constructor was indeed fine, but the new implementation of the second
> constructor (including the test for null) would lead to duplicate
> code.

There is duplicate code only if you allow two ways to do the same thing.
I.e.
  new Incrementor(max);
or
  new Incrementor(max, null);

It's unnecessary and confusing (because it's not obvious that the first call
should behave like the second).

> 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.

That was already the case before.
[You've just added a way to use the second constructor instead of the first
one for doing the exact same thing.]

> I should have thought that
> avoiding code duplication in constructors was good practice.

Yes it is of course.

> 
> From your reaction, I assume you do not like the idea of null being a
> meaningful value for the parameter cb?

Exactly: If a user does not care about the callback, he shouldn't even have
to look at the second constructor, even less wonder about the consequence of
setting it to null.

> 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.

No ticket is necessary (IMHO). I'm fine with just reverting, then adding the
precondition block: it will clearly express that a callback *is* necessary.
[Thanks for spotting that bug.]


Best,
Gilles

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


Mime
View raw message